-
Notifications
You must be signed in to change notification settings - Fork 3
Enable report sharing via URL #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
df837a0 to
6146c82
Compare
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
anth-volk
left a comment
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
… law policy; reorder edit/share icons
This PR enables -
Changes