Skip to content

Conversation

@SakshiKekre
Copy link
Collaborator

This PR enables -

  • Users to share reports via URL. From an owned (created) report, clicking the share button copies a shareable link to clipboard.
  • Recipients to open the shared-report link and see the full report in read-only mode with a "Shared Report" badge.
  • Recipients to save shared reports to their own reports with one click, which creates a local copy and redirects to the owned view.

Changes

  • URL encoding/decoding utils for share data
  • Hook to fetch shared report data from URL params
  • Share button (copy link) and save button (bookmark icon with tooltip)
  • Shared view routing and detection
  • ReportActionButtons component extraction for cleaner code

@vercel
Copy link

vercel bot commented Dec 19, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
policyengine-app-v2 Ready Ready Preview, Comment Dec 24, 2025 10:23am
policyengine-calculator Ready Ready Preview, Comment Dec 24, 2025 10:23am
policyengine-website Ready Ready Preview, Comment Dec 24, 2025 10:23am

@SakshiKekre SakshiKekre marked this pull request as ready for review December 19, 2025 17:54
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @SakshiKekre. This is in the right direction, but I don't see anywhere in this code that actually encodes the user-associated data, which was the original point of using the less ideal URL-based encoding instead of just passing the base report ID as part of the URL.

I'd recommend modifying this code such that, when a user clicks to share, the code takes the user-associated report and encodes all the user-associated records (but not the base ingredient records, since the user-associated records contain the base ingredient IDs)

Then, when a user accesses the shared link, the report page should fetch the entire user report, taking the encoded data to fill in user-associated records that don't exist in the database, then fetching the base ingredients out of the database.

enabled?: boolean;
}

interface UseSharedReportDataResult {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Why not use the existing normalized report hook that also handles user-associated data and assembles it all into a tree structure? And again, is this code not meant to implement user-associated structures?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing hook starts from localStorage associations. For shared reports, we don't have any associations yet - we're fetching directly from API using IDs from the URL-encoded ShareData, then applying the sharer's labels. Only after the user clicks "Save" do associations get created.

There's some duplication in the fetching logic between the two hooks (~50-70 LOC). Could potentially extract a shared "fetch ingredients by IDs" utility that both hooks use, with each handling ID sourcing and label application differently. Happy to refactor if you think it's worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants