release: Share lists by id, copy them atomically, and make usernames unique - #159
Merged
Conversation
Changes: - Switch main from overflow-clip to overflow-x-clip in app/layout.tsx main has px-4 pt-4 and no bottom padding, so clipping both axes cut the bottom border and shadow off whichever element ended the page. The shopping list cards showed it most clearly: every card rounded except the last, which ended in a flat sliced edge. Only the x-axis clip was load-bearing, to contain the TextGlow bleed that c4b674d added so the page stops scrolling sideways on narrow viewports. Notes: adding pb-4 back to main would have fixed the shadow by reverting 809441b, which removed that padding on purpose so the footer supplies the spacing. Narrowing the clip axis leaves layout geometry byte-identical: main keeps padding-bottom 0 and the footer stays flush. Verified at 390px that the landing page still gains no horizontal scrollbar.
Changes: - Name overflow-x-clip and explain why the y axis must stay unclipped The glow-containment paragraph still said main uses overflow-clip and that it cost nothing visible. Clipping both axes did cost something: main has no bottom padding, so the last child's bottom border and shadow were sliced off. Record why restoring pb-4 is the wrong fix, so the next reader does not revert 809441b to chase the shadow.
Changes: - Remove the in-flight paragraph tracking the shopping_list.is_public retirement Step 3 has now been run against dev and prod, so the paragraph described work that was already finished. It told the reader to delete it once the column was gone. Notes: - The generic three-step recipe above the paragraph stays, since it is the reusable pattern for any future NOT NULL column removal under ddl-auto=update.
Changes: - Replace the switch, level select, URL row and revoke dialog with one four-value select and a Google-style access row - Add share-access-row.tsx, link-access-copy.ts and optimistic-list.ts; delete share-link-row.tsx - Give useUpdateShoppingList optimistic cache writes and drop the modal's pendingAccess mirror - Add a "Podijeli poveznicu" button that shares the URL, alongside the text share - Make the switch visible, fix the outline variant in dark mode, colour selected rows in Select and MultiSelect - Give LabeledSelect srOnlyLabel and triggerClassName The modal had grown a switch, a select, a URL row, a confirm dialog and three paragraphs for what is one decision. Private is now a level rather than an off switch, so there is one control and one sentence describing what it means. Two defects fell out of the rework rather than being cosmetic. There was no way to share the link itself: the only button sent the formatted text and the URL was display-and-copy only. And every save flickered new, old, new, because invalidateQueries only starts a refetch while the modal cleared its pending mirror on settle, so the control fell back to the pre-save value for a whole round trip. The mutation now writes the cache in onMutate and applies the response in onSuccess, which is also what makes a freshly minted token available immediately. The switch, outline button and select ticks are shared primitives, so those fixes reach the whole app: the switch was 1.8% in lightness from the background with a transparent border, and outline buttons were white-on-near-white in dark mode. Notes: components/ui/ is edited here despite AGENTS.md saying it is shadcn output. Practice already differs (9db907c added ScrollFade to select.tsx), and the switch, outline variant and select tick all live there. Worth settling the rule either way. The EDIT hint states that adding items is not possible, which is real: /api/shared has no endpoint for creating one. Adding and renaming from a link are both recorded on the v2 roadmap card.
…slot Changes: - Stop Banner hardcoding text-xs over the text-sm its md and lg sizes set - Blur and alpha-fill the soft Banner variants, matching the header - Add an optional icon prop to SelectItem, rendered outside ItemText Banner's size variant scales padding, text and icon, but the body paragraph carried its own text-xs, so every banner in the app rendered its text at the smallest size no matter which size was asked for. The soft variants filled opaquely, so they covered what they sat on instead of tinting it and never matched the header. Alpha fills also mean one value works in both themes, so their dark background and border overrides are gone; the text keeps its overrides, since contrast has to be chosen per theme. SelectValue re-renders the selected item's ItemText, so an icon passed as a child appeared twice: once in the list and again in the trigger. The icon prop renders outside ItemText, and takes text-current so it tracks the row's own colour, including turning primary when checked. Notes: The Banner changes reach seven other call sites: products, scanner, install and auth.
Changes: - Give the banner level-specific text instead of one generic sentence - Add a next step: sign in when signed out, ask the owner otherwise - Drop the sign-in button and fold its hint into the same sentence - Move to primarySoft at size md, so the icon matches the rest of the app - Add link-access-icons.ts, one icon per level, shared with the modal The banner said the same thing whatever the owner had granted, so a recipient could not tell shopping access from editing, and the controls they could not use had no stated reason. The sign-in button was also a dead end: anonymous callers are capped at VIEW whatever the link grants, so it could not promise the button would gain them anything. The next step is suppressed on EDIT, which is the most a link can give, so there is nothing left to ask for. Notes: The icon map is deliberately not used by the list card's visibility indicator, which stays binary Lock and Globe: there it answers "is this shared at all" at a glance, with the level in its tooltip.
Changes: - Give every level in the access select its own icon, matching the banner - Swap the trigger icon for the loading spinner while a save is in flight - Toast on success and keep the error toast, dropping the sr-only success message - Reword the modal description The select offered four bare labels, and the only sign a save had happened was an sr-only message nobody sighted would see. Both icons and spinner occupy 16px in the same slot, so the swap shifts no layout. The toast carries its own live region, so announcing success in the modal as well would make a screen reader say it twice. The sr-only region now covers only the in-flight state, which the toast does not describe.
Changes: - Wrap the trigger's icon and SelectValue in one flex group SelectTrigger is justify-between, so the icon, the label and the chevron were three loose children spread across the full width. Grouping the first two keeps them gap-2 apart, matching the spacing the options already use, and leaves the chevron on the right.
Changes: - Give Select and MultiSelect chevrons size-5 and text-muted-foreground - Swap the search bar's text-gray-400 for the same token - Keep the share select's spinner primary Both chevrons were size-4 at opacity-50 over an inherited colour, which left the one affordance saying "this opens" barely visible. They now match the search bar's leading icon in size and weight. muted-foreground rather than gray-400: the token adapts per theme, where the fixed grey stayed mid-grey against a near-black background. Aligning the search bar to it removes one instance of the class issue #81 exists to clear, and is what makes the two genuinely match.
Changes: - Point the visibility indicator at LINK_ACCESS_ICONS instead of a binary Lock and Globe The card showed one Globe for view, shop and edit alike, so the level was only reachable by hovering the tooltip. It now shows the same glyph the share modal and the shared-list banner use, so a list looks the same wherever you meet it. This reverses the earlier binary reading, that a glance only needs to answer "is this shared". Carrying the level costs nothing once the same glyphs are already taught in the modal, and the tooltip still spells it out for anyone who needs the words.
Changes: - Default unsized svgs in SelectTrigger and SelectItem to size-5 - Size the option icon slot and the share select's trigger glyph to match The chevron moved to size-5 to match the search bar's leading icon, which left every other glyph in a select at size-4 and looking undersized beside it. One size now runs across the trigger, the options and the chevron. Notes: The checked tick stays size-4. It sits in a fixed slot on the right as a state marker rather than a leading glyph, and growing it would push against the row's reserved padding.
Changes: - Add createPenBadgeIcon, which insets any base icon and stamps lucide's own pen path over the bottom-right corner - Add EyePen, ListPen and Share2Pen built on it Lucide ships FilePen, UserPen, SquarePen, NotebookPen, ClipboardPen and FilePenLine, but no eye, list or share equivalent, so these three had to be drawn. Each marks a control whose target already exists, meaning the control edits it rather than creating it. Notes: Lucide's own composites clear the pen's corner by redrawing the base shape shorter, once per icon. These shrink the base by 0.7 instead, with a pre-divided stroke width so it renders back at lucide's 2. That is one rule for any base rather than a bespoke outline each time, and 0.7 is the largest scale at which all three bases still clear the pen by a full stroke.
…ct rows Changes: - Extract sortShoppingListsByRecency out of use-selected-shopping-list into lib/api/shopping-lists - Add useIsOnNewestShoppingList, which answers whether a product already sits on that list The add-to-list modal preselects the newest list. A product row that wants to say so has to agree with the modal on which list that is, so the sort moved next to the service instead of being copied. The hook reads the cached me query rather than fetching a list by id: that payload already carries each list's items, so it costs a subscription and no request. Signed-out visitors skip it, since the endpoint only answers 401.
Changes: - Swap EyeOff for EyePen on the watchlist button, and drop the eye-off entirely - Show ListPen instead of ListPlus once the product is on your newest list - Give the quick-actions sheet the same two reads, so it agrees with the buttons behind it - Point both modal submit icons at the pen when they are editing an existing entry The watchlist button showed a slashed eye once you were tracking something, which promises "stop watching". It opens the tracking settings, so it now carries a pen, and the add-to-list button gained the same distinction it never had. Every label moved with its icon, so the accessible name says which of the two the control will do rather than always saying "add".
…hare control Changes: - Return isShared from useShoppingListActions and thread it through the action group props - Show Share2Pen and "Uredi dijeljenje popisa" on the desktop button, the mobile menu and the actions sheet The three share controls all said "Podijeli popis" whether or not the list was already shared, even though the owner's button opens the settings panel rather than starting a share. Notes: linkAccess comes back null for anyone but the owner, so a recipient passing on a link they hold keeps the plain share icon. That is correct rather than a gap: their button really does start a share, it does not edit one.
…label Changes: - Add ml-1 to the addable-product count in CreateDiscountedListButton The expandIcon effect sets gap-0 on the button, because its icon wrapper animates its own width and padding from zero and a flex gap would leave a permanent hole beside the collapsed icon. That left the count rendering flush against the label, so this button spaces its own count instead. Notes: the three other expandIcon buttons carry a single label child, so gap-0 does not affect them, and no other parenthesised count in the app relies on the button gap.
…ist cards Changes: - Add PriceStack, owning the two-tier figures-divider-figures layout - Add PriceInfoButton, a padding-free ghost trigger hugging its 24px glyph - Render ProductUnitPriceDetails and WatchlistItemDiscountInfo through PriceStack - Drop the undefined text-md classes and a commented-out separator sketch The watchlist card sat noticeably taller than the product card in the same kind of list. Its info trigger was a size-10 button against the product card's size-7, and its stack put a gap on both sides of the divider where the product card had only a 4px margin. Both cards now take their spacing and sizing from one component, and the trigger adds no padding, so a tier is as tall as its icons and nothing more. Notes: text-md is not a Tailwind utility and is not defined in globals.css, so the sm:text-md steps on the product card were already inert. PriceInfoButton sizes its glyph on the element rather than through a [&_svg] variant, which Button's own :not([class*='size-']) rule would outrank. At 24px square it sits exactly on the WCAG 2.5.8 AA minimum, with no expanded hit area by choice. The divider carries margin on both sides because a 24px icon overhangs the 20px line box it sits in.
Changes: - Require a fine pointer and an sm viewport for the card's action cluster The four buttons stayed visible in a resized desktop window, which is the same crowded row a phone gets and made the emulator disagree with the device. Stacked variants and together, so the cluster now needs both. Notes: with a mouse under 640px the card's actions become unreachable, since the long-press fallback is touch only. The product page carries the same four buttons, and the quick-actions sheet passes no visibility class, so it is unaffected.
Changes: - Note that the product card mounts its four actions behind pointer-fine and sm The doc said the desktop card mounts them without saying what counts as desktop, which is now a pointer and a width rather than a pointer alone.
Changes: - Split thresholdValue into percentageValue and absoluteValue, validating only the selected mode, so switching modes keeps an in-progress edit - Seed both fields with form.reset and keepDirtyValues, which installs the baselines as defaults regardless of whether the field is registered - Derive isEdited by comparing each number to its baseline instead of reading RHF's dirtyFields, and gate Spremi on the number differing from what is saved - Draft one number per mode, exclude watchType, and clear only the saved mode's entry on save, so the other mode's unsaved number survives - Fall back to a 1 EUR suggestion when the product has no price data, and skeleton the control until the tracked-threshold check resolves - Skip draft fields the form no longer has, in use-form-draft One field shared by both modes meant the toggle and the number wrote to the same place, so the form could not tell a mode switch from an edit: with both modes tracked the prefill never landed, switching mode enabled Resetiraj, and an untouched prefill was offered for saving and written to localStorage as a draft the user never typed. Notes: the prefill was written with resetField, which does nothing for a field that is not registered, and neither number is while the product loads or for the mode that is off screen. Existing watchlist.* drafts carry the old thresholdValue key; they are now ignored on restore and dropped on the next open and close.
… modes Changes: - Note that the watchlist modal drafts one number per watch mode and excludes watchType - Document that restore skips draft keys the form no longer has - Add a gotcha on seeding a prefill as the form's defaultValue, the resetField registration trap, and why keepDirtyValues means buttons cannot be gated on dirtyFields The draft engine's contract changed with the watchlist fix, and the prefill rules are the part that has been easiest to get wrong.
✅ Deploy Preview for disscount canceled.
|
Contributor
Summary by CodeRabbit
WalkthroughThe pull request updates shopping-list sharing, optimistic cache handling, watchlist threshold forms and drafts, product action states, price layouts, shared UI primitives, responsive behavior, and related documentation. ChangesSharing access and cache updates
Watchlist thresholds and drafts
Product actions and prices
Shared UI and layout
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Owner
participant ShareListModal
participant useShareListModal
participant useUpdateShoppingList
participant ShoppingListCache
Owner->>ShareListModal: Select access level
ShareListModal->>useShareListModal: Request access update
useShareListModal->>useUpdateShoppingList: Submit list mutation
useUpdateShoppingList->>ShoppingListCache: Apply optimistic update
ShoppingListCache-->>Owner: Render updated sharing state
useUpdateShoppingList-->>ShoppingListCache: Apply server result or rollback
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ters Changes: - Product actions now read add to list, watchlist, share, image search - Shopping list actions now read edit, copy, share, delete - Applied the same order to the long press quick action sheets, and to the shopping list's mobile dropdown The two clusters had grown opposite conventions: the product row ended on its most useful action while the shopping list row started with sharing. Leading with the actions that write to the user's own data puts the common case under the thumb first, and keeps the destructive one last. Notes: the shopping list card's sharing indicator sits outside the cluster and is unchanged.
Changes: - Updated the shared-order rule to the new product and shopping list sequences - Noted that the sharing indicator is not part of the shopping list cluster The doc spelled out both orders verbatim, so the reorder made it wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/lib/api/shopping-lists/hooks.ts (1)
77-102: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize sharing updates before persisting and replaying them.
setLinkAccesswriteslinkAccessoptimistically on each change, while offline queued mutations use one shared key per list and pause until reconnect. If two access changes for the same list pause, replay can restore or apply them out of order and leave the wrong access level in the cache/server state. Queue sharing writes by list, or track a per-list mutation/replay order and ignore stale outcomes. Add a test that changes access twice while offline and replays them in reverse order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/api/shopping-lists/hooks.ts` around lines 77 - 102, Serialize linkAccess mutations per shopping list so offline changes cannot replay or apply out of order. Update the mutation flow around onMutate, onSuccess, and onSettled (and the setLinkAccess caller if needed) to preserve per-list submission order and ignore stale outcomes; add coverage for changing access twice offline and replaying the mutations in reverse order, ensuring the latest access level remains in both cache and server state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/LANDING.md`:
- Line 264: Update the sentence in the documentation to say “Clip the x-axis
only.”, adding the missing hyphen while preserving the surrounding explanation.
In `@frontend/src/app/s/`[token]/components/shared-list-access-banner.tsx:
- Around line 21-27: Define an exported ISharedListAccessBannerProps interface
in SharedListAccessBanner’s file containing myAccess and isSignedIn, then
replace the inline props object in the component signature with that interface.
- Around line 37-41: Update the unauthenticated branch of the nextStep
expression in shared-list access banner so it uses neutral sign-in guidance that
does not promise edit access, while preserving the existing myAccess-based
messaging for signed-in users.
In `@frontend/src/components/custom/price/price-info-button.tsx`:
- Around line 3-47: Update PriceInfoButton to use named React imports for
forwardRef and the ComponentProps type instead of the React namespace, and
extract the inline forwardRef render callback into a named render function while
preserving its props, ref forwarding, and button behavior.
In `@frontend/src/components/ui/select.tsx`:
- Around line 120-152: The shadcn UI primitives must remain generator-managed;
move all application-specific changes into custom wrappers or the generator
source, then regenerate. In frontend/src/components/ui/select.tsx lines 120-152,
relocate SelectItem.icon and selected-item styling; in
frontend/src/components/ui/select.tsx lines 38-48, relocate trigger-specific
styling; in frontend/src/components/ui/button.tsx lines 32-37, relocate the
outline variant customization; and in frontend/src/components/ui/switch.tsx
lines 16-37, relocate track and thumb customization. Preserve the current
behavior through the custom wrappers or regenerated output.
In `@frontend/src/utils/browser/storage/drafts.ts`:
- Around line 26-33: Update removeFormDraftField so the setFormDraft call
preserves the existing draft.savedAt expiry timestamp when replacing
draft.values; ensure removing one field does not reset or extend the untouched
draft’s expiration.
---
Outside diff comments:
In `@frontend/src/lib/api/shopping-lists/hooks.ts`:
- Around line 77-102: Serialize linkAccess mutations per shopping list so
offline changes cannot replay or apply out of order. Update the mutation flow
around onMutate, onSuccess, and onSettled (and the setLinkAccess caller if
needed) to preserve per-list submission order and ignore stale outcomes; add
coverage for changing access twice offline and replaying the mutations in
reverse order, ensuring the latest access level remains in both cache and server
state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b428c1fe-2063-4a30-b1f9-0b6e93857ca3
📒 Files selected for processing (59)
docs/DEPLOYMENT.mddocs/LANDING.mddocs/MOBILE-NAV.mddocs/SHARING.mddocs/STATE-PERSISTENCE.mdfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-buttons.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-desktop-actions.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-mobile-actions.tsxfrontend/src/app/(user)/shopping-lists/[id]/hooks/use-shopping-list-actions.tsfrontend/src/app/(user)/shopping-lists/components/forms/share-access-row.tsxfrontend/src/app/(user)/shopping-lists/components/forms/share-link-row.tsxfrontend/src/app/(user)/shopping-lists/components/forms/share-list-modal.tsxfrontend/src/app/(user)/shopping-lists/components/shopping-list-actions-sheet.tsxfrontend/src/app/(user)/shopping-lists/components/shopping-list-quick-actions-list.tsxfrontend/src/app/(user)/shopping-lists/components/shopping-list-visibility-indicator.tsxfrontend/src/app/(user)/shopping-lists/hooks/use-share-list-modal.tsfrontend/src/app/(user)/shopping-lists/utils/link-access-copy.tsfrontend/src/app/(user)/shopping-lists/utils/link-access-icons.tsfrontend/src/app/(user)/watchlist/components/create-discounted-list-button.tsxfrontend/src/app/(user)/watchlist/components/watchlist-discount-row.tsxfrontend/src/app/(user)/watchlist/components/watchlist-item-discount-info.tsxfrontend/src/app/layout.tsxfrontend/src/app/products/components/forms/add-to-shopping-list-form.tsxfrontend/src/app/products/components/forms/watchlist-item-modal.tsxfrontend/src/app/products/components/forms/watchlist-threshold-input.tsxfrontend/src/app/products/components/product-action-buttons.tsxfrontend/src/app/products/components/product-item/product-item.tsxfrontend/src/app/products/components/product-item/product-price-scope-info.tsxfrontend/src/app/products/components/product-item/product-price.tsxfrontend/src/app/products/components/watchlist-action-button.tsxfrontend/src/app/products/hooks/use-selected-shopping-list.tsfrontend/src/app/products/hooks/use-watchlist-item-form.tsfrontend/src/app/products/typings/watchlist-form.tsfrontend/src/app/products/utils/watchlist-thresholds.tsfrontend/src/app/s/[token]/components/shared-list-access-banner.tsxfrontend/src/components/custom/common/banner.tsxfrontend/src/components/custom/common/labeled-select.tsxfrontend/src/components/custom/form/multi-select-item.tsxfrontend/src/components/custom/form/multi-select-trigger.tsxfrontend/src/components/custom/icons/eye-pen.tsfrontend/src/components/custom/icons/list-pen.tsfrontend/src/components/custom/icons/pen-badge.tsfrontend/src/components/custom/icons/share-2-pen.tsfrontend/src/components/custom/price/price-info-button.tsxfrontend/src/components/custom/price/price-stack.tsxfrontend/src/components/custom/price/store-price-popover.tsxfrontend/src/components/custom/product/product-quick-actions-list.tsxfrontend/src/components/custom/product/product-summary.tsxfrontend/src/components/custom/search/search-bar.tsxfrontend/src/components/ui/button.tsxfrontend/src/components/ui/select.tsxfrontend/src/components/ui/switch.tsxfrontend/src/hooks/use-form-draft.tsfrontend/src/lib/api/schemas/shopping-list.tsfrontend/src/lib/api/shopping-lists/hooks.tsfrontend/src/lib/api/shopping-lists/optimistic-list.tsfrontend/src/lib/api/shopping-lists/sort-lists.tsfrontend/src/lib/api/shopping-lists/use-newest-list-membership.tsfrontend/src/utils/browser/storage/drafts.ts
💤 Files with no reviewable changes (3)
- frontend/src/lib/api/schemas/shopping-list.ts
- frontend/src/app/(user)/shopping-lists/components/forms/share-link-row.tsx
- docs/DEPLOYMENT.md
| Mixing an `animate` prop with `style` MotionValues on one element makes them fight over the same transforms. `HeroCart` and `CartChaser` both nest two `motion.div`s instead: the outer one owns `animate` (opacity/scale/rotateY enter-exit states), the inner one owns the continuous `style` springs (`scaleX`, `rotate`). | ||
|
|
||
| Decorative overflow becomes a horizontal scrollbar unless it is clipped. `TextGlow` is positioned with negative insets (`-inset-6`/`-inset-8`) so its halo intentionally sits ~25px outside its text box; on a narrow viewport a near-full-width heading pushes that transparent overhang past the right edge (~13px at 390px). The doodle glows do the same to a smaller degree. This surfaces as a scrollbar because of a CSS rule: when one overflow axis is `hidden` (not `visible`/`clip`), the other axis computes to `auto`. `<main>` in `app/layout.tsx` therefore uses `overflow-clip` (both axes clip, no scroll container) rather than `overflow-y-hidden`, which had silently made the x-axis scrollable. `overflow-clip` clips the transparent glow edge with zero visible loss and does not affect page (body) vertical scroll. Prefer `overflow-*-clip` over `overflow-*-hidden` for containing decorative bleed. | ||
| Decorative overflow becomes a horizontal scrollbar unless it is clipped. `TextGlow` is positioned with negative insets (`-inset-6`/`-inset-8`) so its halo intentionally sits ~25px outside its text box; on a narrow viewport a near-full-width heading pushes that transparent overhang past the right edge (~13px at 390px). The doodle glows do the same to a smaller degree. This surfaces as a scrollbar because of a CSS rule: when one overflow axis is `hidden` (not `visible`/`clip`), the other axis computes to `auto`. `<main>` in `app/layout.tsx` therefore uses `overflow-x-clip` (clips, no scroll container) rather than `overflow-y-hidden`, which had silently made the x-axis scrollable. Clip the x axis only. This started as `overflow-clip`, which clips both axes, and the vertical half was not free: `main` carries `px-4 pt-4` and no bottom padding, so the last child sits flush against the content edge and its bottom border and shadow were sliced off. Restoring `pb-4` is the wrong fix, since 809441bb694ec5ecfc4e57c4aa2358ce2aab6637 removed that padding deliberately to let the footer supply the spacing. Prefer `overflow-*-clip` over `overflow-*-hidden` for containing decorative bleed, and clip only the axis that actually bleeds. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use “x-axis” in the sentence.
Line 264 should say “Clip the x-axis only.” The missing hyphen is a user-facing documentation typo.
🧰 Tools
🪛 LanguageTool
[grammar] ~264-~264: Use a hyphen to join words.
Context: ...y made the x-axis scrollable. Clip the x axis only. This started as `overflow-cli...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/LANDING.md` at line 264, Update the sentence in the documentation to say
“Clip the x-axis only.”, adding the missing hyphen while preserving the
surrounding explanation.
Source: Linters/SAST tools
Comment on lines
+37
to
+41
| const nextStep = !isSignedIn | ||
| ? "Prijavi se za uređivanje." | ||
| : myAccess === "EDIT" | ||
| ? null | ||
| : "Za više ovlasti obrati se vlasniku popisa."; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not promise edit access after sign-in.
A visitor on a VIEW link remains at VIEW after sign-in. Line 38 can therefore promise edit access that the link does not grant. Use neutral guidance, such as Prijavi se da koristiš ovlasti ove poveznice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/app/s/`[token]/components/shared-list-access-banner.tsx around
lines 37 - 41, Update the unauthenticated branch of the nextStep expression in
shared-list access banner so it uses neutral sign-in guidance that does not
promise edit access, while preserving the existing myAccess-based messaging for
signed-in users.
Comment on lines
+3
to
+47
| import * as React from "react"; | ||
| import { Info } from "lucide-react"; | ||
|
|
||
| import { Button } from "@/components/ui/button"; | ||
| import { cn } from "@/lib/utils"; | ||
|
|
||
| // Plain button props rather than Button's own: its icon props are a union that | ||
| // collapses once they are spread through a wrapper, and this trigger never | ||
| // takes a leading icon anyway. | ||
| interface IPriceInfoButtonProps extends Omit< | ||
| React.ComponentProps<"button">, | ||
| "children" | ||
| > { | ||
| label: string; | ||
| } | ||
|
|
||
| /** | ||
| * The info affordance that sits beside a price, on a product card and on a | ||
| * watchlist card alike. Sized here rather than per caller because it is the | ||
| * tallest thing in either row, so a caller picking its own size silently | ||
| * changes how tall that card is. | ||
| * | ||
| * The box hugs the glyph, which leaves it at 24px square: the WCAG 2.5.8 AA | ||
| * floor exactly, with nothing to spare. Do not shrink it further. The size goes | ||
| * on the icon rather than through a [&_svg] variant, because the button's own | ||
| * icon rule guards on :not([class*='size-']) and would otherwise outrank it. | ||
| * | ||
| * Forwards its ref, since both callers hand it to a Radix trigger as a child. | ||
| */ | ||
| const PriceInfoButton = React.forwardRef< | ||
| HTMLButtonElement, | ||
| IPriceInfoButtonProps | ||
| >(({ label, className, ...props }, ref) => ( | ||
| <Button | ||
| ref={ref} | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon-sm" | ||
| aria-label={label} | ||
| className={cn("size-6 p-0", className)} | ||
| {...props} | ||
| > | ||
| <Info aria-hidden="true" className="size-6" /> | ||
| </Button> | ||
| )); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
git ls-files | grep -F 'frontend/src/components/custom/price/price-info-button.tsx' || true
echo
echo "File contents with line numbers:"
cat -n frontend/src/components/custom/price/price-info-button.tsx
echo
echo "Check React namespace usages and export style in frontend/src component files:"
python3 - <<'PY'
from pathlib import Path
import re
p = Path('frontend/src/components/custom/price/price-info-button.tsx')
text = p.read_text()
print("uses_namespace_react_dot:", bool(re.search(r'\bReact\.', text)))
print("uses_star_import_from_react:", re.search(r'^import\s*\*\s+as\s+\w+\s+from\s+["\']react["\']', text, flags=re.M) is not None)
print("has_named_function_export:", bool(re.search(r'\bconst\s+PriceInfoButton\s*=\s*forwardRef\s*<[^>]*>[^=]*function\s+PriceInfoButton\b', text, flags=re.S)))
print("has_return_statement:", re.search(r'\breturn\b', text) is not None)
PYRepository: OffCrazyFreak/Disscount
Length of output: 2422
Use named React imports and a named render function.
Import forwardRef and type ComponentProps from react, use them instead of the React. namespace, and move the render callback to a named function so it follows the TypeScript component guidelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/custom/price/price-info-button.tsx` around lines 3 -
47, Update PriceInfoButton to use named React imports for forwardRef and the
ComponentProps type instead of the React namespace, and extract the inline
forwardRef render callback into a named render function while preserving its
props, ref forwarding, and button behavior.
Source: Coding guidelines
Comment on lines
+120
to
+152
| icon: Icon, | ||
| ...props | ||
| }: React.ComponentProps<typeof SelectPrimitive.Item>) { | ||
| }: React.ComponentProps<typeof SelectPrimitive.Item> & { | ||
| /** | ||
| * Leading glyph for the option. Rendered outside `ItemText` on purpose: `SelectValue` | ||
| * re-renders the selected item's `ItemText`, so an icon inside it would appear a second | ||
| * time in the trigger. | ||
| */ | ||
| icon?: React.ElementType; | ||
| }) { | ||
| return ( | ||
| <SelectPrimitive.Item | ||
| data-slot="select-item" | ||
| className={cn( | ||
| "focus:bg-accent focus:text-accent-foreground [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-default items-center gap-2 rounded-sm py-1.5 pr-8 pl-2 text-sm outline-hidden select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2", | ||
| "focus:bg-accent focus:text-accent-foreground [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-default items-center gap-2 rounded-sm py-1.5 pr-8 pl-2 text-sm outline-hidden select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-5 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2", | ||
| // The chosen row said so with a grey tick and nothing else, so it looked the same | ||
| // as the rest until you compared ticks. | ||
| "data-[state=checked]:text-primary data-[state=checked]:font-medium", | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| <span className="absolute right-2 flex size-3.5 items-center justify-center"> | ||
| <SelectPrimitive.ItemIndicator> | ||
| <CheckIcon className="size-4" /> | ||
| {/* text-primary explicitly: the item's [&_svg:not([class*='text-'])] rule would | ||
| otherwise paint it muted. */} | ||
| <CheckIcon className="size-4 text-primary" /> | ||
| </SelectPrimitive.ItemIndicator> | ||
| </span> | ||
| {/* text-current, not a colour of its own: the class still contains "text-", which is | ||
| what keeps the muted rule above off it, so the glyph tracks whatever the row's | ||
| text is doing, including turning primary when checked. */} | ||
| {Icon && <Icon className="size-5 text-current" aria-hidden="true" />} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep shadcn UI primitives generated.
The same path-rule violation appears in all three files. Move application-specific styling and behavior into custom wrappers, or update the generator source and regenerate.
frontend/src/components/ui/select.tsx#L120-L152: moveSelectItem.iconand selected-item styling into a custom item wrapper.frontend/src/components/ui/select.tsx#L38-L48: move trigger-specific styling into a custom wrapper or generator source.frontend/src/components/ui/button.tsx#L32-L37: move the outline variant customization into a custom button wrapper or generator source.frontend/src/components/ui/switch.tsx#L16-L37: move the track and thumb customization into a custom switch wrapper or generator source.
As per coding guidelines, frontend/src/components/ui/**/*.{ts,tsx} says: “Do not hand-edit shadcn UI primitives.”
📍 Affects 3 files
frontend/src/components/ui/select.tsx#L120-L152(this comment)frontend/src/components/ui/select.tsx#L38-L48frontend/src/components/ui/button.tsx#L32-L37frontend/src/components/ui/switch.tsx#L16-L37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/ui/select.tsx` around lines 120 - 152, The shadcn UI
primitives must remain generator-managed; move all application-specific changes
into custom wrappers or the generator source, then regenerate. In
frontend/src/components/ui/select.tsx lines 120-152, relocate SelectItem.icon
and selected-item styling; in frontend/src/components/ui/select.tsx lines 38-48,
relocate trigger-specific styling; in frontend/src/components/ui/button.tsx
lines 32-37, relocate the outline variant customization; and in
frontend/src/components/ui/switch.tsx lines 16-37, relocate track and thumb
customization. Preserve the current behavior through the custom wrappers or
regenerated output.
Source: Coding guidelines
…draft Changes: - Clamp the absolute watch suggestion to the schema's range, so a tenth off a sub-1 EUR product no longer opens the modal below the 0.1 EUR minimum - Key WatchlistItemModal on the ean in the modal outlet - Resolve the product row's list membership against the list the add-to-list modal will actually preselect, renaming the hook to useIsOnPreselectedShoppingList A suggestion under the minimum opened the modal already invalid, with the save button dead and no message saying why until the field was touched, because the seeding path clears errors rather than validating. The outlet stays mounted between openings, so an unkeyed watchlist modal handed product B the threshold edited for product A. Add-to-list was already keyed for this reason. The row's pen icon claimed the newest list, but the modal restores a drafted list choice and opens on that instead, so the icon could describe an entry on a list the modal was never going to show.
Changes: - Deleted shopping-list-mobile-actions.tsx - Narrowed mobilePresentation to "buttons" | "none" and defaulted it to "none" The dropdown only rendered under mobilePresentation="menu", which was the default but which neither call site ever passed: the header asks for "buttons" and the card for "none". It had no reachable path in the app, so the reorder it just received was invisible either way. Notes: use-tap-to-open.ts was the dropdown's only consumer and now has none. It is left in place, since it documents a Radix touch bug worth keeping, but it is dead until something mounts a menu on touch again.
Changes: - Replaced "all three of its surfaces" with the two that remain - Recorded why the third one went The mobile dropdown was removed as unreachable, so the count was wrong.
Changes: - Deleted use-tap-to-open.ts, which the dropdown was the only consumer of - Renamed shopping-list-desktop-actions.tsx to shopping-list-action-row.tsx - Replaced the mobilePresentation union with a showOnMobile boolean The dropdown left three loose ends behind it. The hook had no callers at all. The row was still called "desktop" while rendering on mobile for the detail header, which is one of its two callers. And mobilePresentation was a three-value union with only two values left, which is a boolean. Behaviour is unchanged: the header still shows the row at every width and the card still hides it below sm. Notes: HOLD_CANCEL_PX stays, since use-long-press.ts and use-bottom-nav-pointer.ts still read it. The Radix pointerdown bug the deleted hook worked around is now recorded in docs/MOBILE-NAV.md.
Changes: - Added the pointerdown-opens-on-touch gotcha the deleted hook carried - Updated the action cluster rule for showOnMobile and the renamed row The workaround was documented only in the file that got deleted, so the reason it existed would have gone with it.
* docs(agents): Record how to deliver review artifacts
Changes:
- Require artifacts to be handed back as [name](file:///abs/path) links, and
list the forms that were tested and do not work
- Add the HTML layout rules: full browser width, a wide Finding column, a
narrow Where column, and Now/Expected/Test split into separate blocks
Tested five path forms against the terminal and only the Markdown link to a
file:// URI is clickable, so the skill now names it as the only option rather
than leaving it to judgement. The layout rules each record the way the page had
become unreadable, so they are not mistaken for style preferences later.
* refactor(shopping-lists): Share a list by its own id, not a token
Changes:
- Delete SharedShoppingListController, SharedShoppingListService, the share_token
column and findActiveByShareToken
- Move the by-id list and item routes onto the optional-bearer chain, matched by
method plus a UUID-shaped id so /me and /items stay authenticated
- Resolve the caller's access in ShoppingListService and ShoppingListItemService
instead of hardcoding OWNER, and port the SHOP versus EDIT field mask across
- Answer not-found rather than forbidden for a list the caller may not see
- Add NotFoundException and a DataIntegrityViolationException handler
The token bought rotation, which protects against a scenario that does not
happen: a grocery list is shared once, used for a week and abandoned. Sharing on
the list's own id means the URL in the owner's address bar is the one worth
sending, which is what people already expect from Google Docs.
Notes:
The matcher is an allowlist of method plus UUID-shaped id rather than a wildcard
minus exceptions. A denylist would silently expose any future literal route such
as /api/shopping-lists/archived, since it would match the wildcard and not be
listed as an exception.
The chain's own rule is permitAll, so the access checks in the services are now
an authentication boundary rather than a convenience. Relaxing one removes
authentication. This is stated in the SecurityConfig javadoc as well.
Adding items stays owner-only through isOwner() rather than canManageShare(),
which agrees today, so that a later decision to let a link level manage sharing
cannot quietly grant item creation or deletion too.
* refactor(shopping-lists): Serve owners and link visitors from one route
Changes:
- Delete /s/[token] and read every list through /shopping-lists/[id]
- Move the access banner and the unavailable state into the list feature folder
- Take the shareToken parameter out of the data, item-mutation and action hooks,
the header, the action buttons and the item list
- Build the share URL from the list id, and drop the sharedShoppingList query
root, its two offline mutation keys and the shared query functions
- Gate the share and edit modals on ownership, and the share-link button on the
save being in flight
- Bump CACHE_BUSTER so a queued write with a retired key is not dropped in silence
The detail client used to hardcode isSignedIn and say in a comment that the route
was behind the auth gate. It is not any more, so it reads the session and every
control gates on the access the server resolved.
Notes:
The access banner renders an empty element carrying its id rather than null when
it has nothing to say. The disabled item controls point at that id with
aria-describedby, and a list DTO persisted before myAccess existed replays with
it undefined, which makes the controls disabled and the description absent at
once.
The by-id query inherits the 30 second staleTime and focus refetch the shared
query owned. Two people shopping off one list need each other's ticks, and that
read is now the only one there is.
The lost-access toast moves onto the by-id item writes, which link visitors now
use. An owner never reaches its branch, since you cannot lose access to your own
list.
* chore(pwa): Retire the share-token infrastructure
Changes:
- Delete scrub-share-token.ts and its Sentry wiring, including the Replay
exclusion for /s/ pages
- Move the page cache bucket from shared-list-pages to shopping-list-pages and
match /shopping-lists/ instead of /s/
- Move Referrer-Policy no-referrer onto the shopping-lists header rule
- Stop disallowing /shopping-lists in robots.ts
A list id now travels wherever a share token used to, and it is not scrubbed.
Unlike the token it replaced, the id is the app's ordinary identifier and appears
in paths, query keys and offline storage, so redacting it would blind every
shopping-list trace rather than protect one route. It is inert once the list is
not shared, and the exposure is Sentry and our own proxy log.
Notes:
robots.ts no longer derives from PROTECTED_ROUTE_PREFIXES, because the two
consumers want opposite answers now. A disallowed URL is never fetched, so a
crawler would never read the noindex header, and a shared list is exactly the
kind of URL that gets pasted somewhere crawlable. The prefix stays in the list
for the logout redirect, where the index behind it is still auth-only.
* feat(auth): Make usernames unique
Changes:
- Mark app_user.username unique, and add existsByUsername
- Refuse a taken name in updateProfile with a 409 carrying fieldErrors
- De-duplicate seedUsername with a numeric suffix
- Say in the settings form that the name has to be unique
Version 2 invites people by username, which only means one person if the column
says so. The frontend already required a name; uniqueness is the half only the
server can enforce, since two requests can pass any client-side check at once.
Notes:
The column stays nullable. deleteAccount nulls the username to free the name for
somebody else, and Postgres lets a unique index hold as many nulls as it likes.
Presence stays the frontend's rule.
Seeding suffixes rather than refusing, because nobody is present to choose on a
first login and the sources collide readily: two Google accounts called the same
thing, or two addresses with the same local part. The user-facing edit refuses
instead, where there is somebody to ask.
* fix(ui): Keep the select label still while it saves
Changes:
- Give the spinner's svg a text-inherit class so a container's descendant rule
stops overriding its colour
- Drop the spinner to 16px in the share access select and the list action row,
matching the icons it stands in for
Two separate faults. The spinner rendered muted rather than primary because
ui/select styles descendant svgs with
[&_svg:not([class*='text-'])]:text-muted-foreground, which outranks the colour
sitting on the spinner's wrapper div. Carrying its own text- class opts out of
that rule, which fixes every spinner inside a styled container at once.
And it was 4px wider than the icon it replaced, which shunted the label sideways
for the length of a save. size-5 is 16px in this project, not 20px, because
--spacing is 0.2rem rather than Tailwind's stock 0.25rem.
* feat(shopping-lists): Choose what a copy carries
Changes:
- Add a shopping-list/copy modal target and mount it in the entity outlet
- Offer three options: products on by default, ticks and captured prices off,
sharing settings off
- Disable the progress option without products, and the sharing option unless
the viewer owns the original
- Point the existing copy button at the modal instead of copying on the spot
Copying used to guess, and it guessed wrong for the common case. The usual
reason to copy is "same shop, next week", so inheriting last week's ticks and
prices makes the copy wrong the moment it opens, while a copy that silently
drops a list's sharing surprises the other direction.
Notes:
The sharing option is owner-only because a recipient could otherwise copy a
shared list and hand the owner's people a link at a level the owner never chose.
The server refuses linkAccess from a non-owner regardless; the checkbox state is
honesty, not the guard.
Ticks, chain and both captured prices travel together as one option. A tick
without its price reads as a bargain nobody recorded.
The copy button loses its pending state everywhere, because opening a modal is
instant. The spinner now belongs to the modal's submit.
* fix: Apply the 2026-08-06 review findings
Changes:
- Parse thresholds with the whole string, so "10abc" is not read as 10
- Scope the unsaved marker to the visible watch mode, and keep reset on both
- Subscribe to draft changes instead of snapshotting the drafted list at mount
- Adopt the shared recency comparator on the lists page
- Hoist the four product action labels so the card row and the hold sheet agree
- Move the threshold skeleton out of FormControl and drop the aria-label that
contradicted the visible one
- Guard the watchlist read on a session, announce the discounted count, and keep
the disabled share button focusable so its explanation is reachable
- Render a zero secondary price, constrain the SelectItem icon prop, delete two
LabeledSelect props with no callers, and give the badged icons kebab-case names
- Size the switch in explicit rem, since size-5 is 16px under this project's
--spacing, and fit the thumb inside the track
- Replace forwardRef in PriceInfoButton, which React 19 does not need
- Move the preselected-list hook out of the data layer into hooks/
Everything except rows 1, 2, 6, 7, 20, 31 and 33, which were skipped by request.
Notes:
Rows 3, 22 and 25 are resolved by the sharing rework rather than fixed here: the
shareToken they concerned no longer exists, the banner moved into the feature
folder it used to reach across into, and its props interface came back with it.
Row 8 needed the drafts store to become observable. Product rows outlive the
modal that writes the draft, so a mount-time snapshot went stale the moment
somebody changed the list, which is the contradiction the hook exists to stop.
* docs: Record sharing by list id and unique usernames
Changes:
- Rewrite SHARING.md sections 3 to 10 for the id-as-link model, including the
accepted trade that a shared list's id is a capability and is not scrubbed
- Update AUTH.md for the new optional-bearer chain and the username rule
- Repoint PWA.md at the shopping-list-pages cache bucket
- Correct MOBILE-NAV.md: three Radix menus still mount on touch unguarded, and
the list card leaves a mouse under 640px without actions
- Document per-field draft clearing and the draft subscription
- Hyphenate x-axis in LANDING.md
Notes:
Section 3 stops being "the token, and why it is not the list id" and becomes the
record of why that reasoning was reversed, so the argument survives rather than
just its conclusion.
MOBILE-NAV.md previously said nothing mounts a Radix menu on touch, which was
the stated justification for deleting use-tap-to-open. It is false, and it is
the kind of claim a reader trusts instead of checking.
* fix(shopping-lists): Answer the first review pass
Changes:
- Keep list pages out of Session Replay, which records the DOM and so would
capture someone else's list contents
- Report a partial copy honestly instead of claiming the whole thing failed
- Stop reading a replayed delete's 404 as lost access, since an already-deleted
item is the desired end state
- Say so when the access level is still unknown, rather than pointing the
disabled controls at an empty description
- Correct the staleTime comment, which promised a liveness the config does not
deliver
Notes:
Accepting the list id in telemetry was a deliberate trade, recorded in
SHARING.md. Replay is a different question and was not part of it: it captures
the page rather than its URL, so a recording of a shared list carries the list
itself. Those pages were excluded when they lived under /s/ and stay excluded.
The copy still is not atomic. There is no server-side copy endpoint and adding
one was out of scope, so a failed item now says which count did not make it
rather than reporting failure while a partial list sits on the server.
* fix: Answer the multi-agent review
Changes:
- Flush the profile insert so a first-login username collision is caught where
it is handled, and retry once without a username instead of a 500
- Drop bare 32-hex from the id matcher, since Spring cannot bind it and it only
bought an anonymous caller a logged 500
- Add HEAD to the matcher, so a link unfurler does not get 401 on a shared list
- Answer 400, not a logged 500, for a path variable that will not convert
- Keep CACHE_BUSTER at "3" and tombstone the two retired mutation keys instead
- Call the write-failure handler from the hooks' own onError, since a hook-level
onError replaces the mutation default rather than running beside it
- Add peekFormDraft, so reading a draft during render cannot write localStorage
- Carry savedAt through removeFormDraftField
- Guard the second watchlist read on a session, not just the first
- Name the copy checkboxes by their label alone, and keep a disabled one focusable
- Route the edit gate through resolveShoppingListAccess
Notes:
The CACHE_BUSTER bump was the worst of these and was my own. A mismatch makes
persistQueryClient call removeClient(), which discards the entire persisted
client including queued writes under every key, not just the two retired ones.
It would have thrown away somebody's ticks under keys this branch never touched,
which is the exact thing the comment claimed it was preventing.
Several comments claimed guarantees that were not there: that the server refuses
linkAccess from a non-owner (a create runs as the new list's owner, so there is
nothing to refuse), that removeFormDraftField preserved savedAt (it did not,
until now), and that both product surfaces guarded the watchlist read (only one
did). Each is now either true or gone.
* feat(shopping-lists): Copy a list in one transaction
Changes:
- Add POST /api/shopping-lists/{id}/copy and a ShoppingListCopyService, so the
copy and its items commit together
- Enforce owner-only sharing carry-over on the server, not just in the modal
- Narrow the 409 to duplicate keys, and log the constraint name rather than the
driver message, which carries the colliding value
- Replace the modal entry instead of closing it and pushing, and pass replace
from the actions sheet
- Stop building the share URL during render, where a misconfigured app URL would
take the whole modal down instead of one button
- Skip a boolean secondary price rather than drawing an empty tier
- Require the ask-first items to actually be asked, in AGENTS.md
- Cut the comment bloat this branch introduced, from 51% of added lines to 39%
Copying was a create followed by one add per item, which are separate
transactions: a failure partway left a half-populated copy on the server that no
retry could tidy up, and pressing the button again made another one.
Notes:
I should have asked about the endpoint rather than working around its absence.
AGENTS.md now says so in as many words, because the workaround shipped worse and
the question would have taken a sentence.
The comments were the other half of that. Several restated what the code already
said. The ones left record a decision the code cannot: why not-found rather than
forbidden, why the matcher is an allowlist, why saveAndFlush rather than save.
* style: Cut the comments that restate the code
Changes:
- Trim the comment blocks in link-access-copy, instrumentation-client, purge and
persister down to the decision each records
- Move the CACHE_BUSTER version history into docs/PWA.md, where it belongs
Swept the whole dev-to-main range, not just this branch. Of 53 files changed
there only one was over the line, so the older work was fine and the bloat was
recent and mine.
Notes:
The rule I was breaking: a comment earns its place by recording something the
code cannot say, such as why not-found rather than forbidden, or why bumping the
cache buster is heavier than it looks. Restating what the next line already says
is noise, and history belongs in the docs.
* fix(auth): Let a username collision reach its 409
Changes:
- Catch DataIntegrityViolationException, not DuplicateKeyException, and narrow on SQLState 23505
- Move the provisioning insert into UserProfileCreator, a REQUIRES_NEW bean
- Re-read by id before the nameless retry, since a concurrent first login is the usual cause
Neither half of the collision path worked. Hibernate only raises DuplicateKeyException
for a session-level id clash, so a database unique violation arrived as the general
type and fell through to the 500 the 409 was written to replace. The seeding retry had
the same shape of problem: the failed insert aborted the transaction it shared with its
caller, so the retry inside that transaction could never commit.
Notes:
The handler answers the non-conflict case itself rather than rethrowing, because an
exception thrown out of an @ExceptionHandler bypasses the advice and surfaces a
container 500 instead of a Problem Detail.
* fix(pwa): Purge both offline stores even when one fails
Changes:
- Run removePersistedCacheFor and purgeServiceWorkerCaches through Promise.allSettled
- Report each rejection separately
The two deletes were sequential awaits inside one try, so a failing IndexedDB purge
skipped the service worker buckets entirely and left the departing account's data in
them across a logout.
* fix(ui): Address the reader in the title validation copy
Changes:
- Replace "Naziv mora/može imati" with "Upiši naziv s" in all five title schemas
The messages described the field rather than addressing the person filling it in.
Changed everywhere the phrasing appears, not only in the copy request, so the same
field does not word its error two ways depending on which form it sits in.
* docs: Record the copy route and the provisioning transaction
Changes:
- SHARING.md: add POST /{id}/copy to the route table, correct "six routes" to seven plus the HEAD matcher
- AUTH.md: record why provisioning inserts through a REQUIRES_NEW bean and why the handler narrows on SQLState
- AGENTS.md: allow editing components/ui/ where the primitive is the natural home for the change
The route table had not caught up with the copy endpoint. The AGENTS.md rule was
costing more than it bought: a wrapper cannot fix a sizing rule our --spacing override
breaks without duplicating the class string it is wrapping.
OffCrazyFreak
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Production release. 29 commits, 130 files, frontend and backend.
Follows the sharing release merged as #152. Two things dominate: the sharing model was rebuilt twice over, first onto a single access select (#159's original scope) and then off tokens entirely onto the list's own id (#160). Around that sits a sweep of card, icon and watchlist consistency work.
This release needs two manual database migrations. See the Database section, and read it before merging.
Sharing moved off capability tokens
A shared list used to live at
/s/<token>, behind a rotatingshopping_list.share_token. It now lives at its own canonical URL,/shopping-lists/<id>, andlink_accessalone decides what holding that URL grants.The reasoning is recorded in
docs/SHARING.md§3. Rotation protected against a scenario that does not happen: a grocery list is shared once, used for a week and abandoned. What the change buys is that the owner can copy the URL out of the address bar and it works, which it previously did not. The trade is accepted and stated explicitly: while a list is shared, its id is an effective capability, it appears in Sentry, proxy logs and browser history, and unlike the token it cannot be scrubbed, because it is the application's identifier everywhere.frontend/src/app/s/is deleted.HEAD) take an optional bearer token.UuidScopedRequestMatcherallowlists them by method and UUID-shaped id, so/meand/itemsstay authenticated by shape rather than by being named as exceptions.ShoppingListService. A list the caller may not see is a 404, never a 403, so the response cannot confirm that an id is real./s/<token>links break. No grace-period redirect, by decision.Copying a list is now server-side and atomic
Copy used to be a create followed by one item POST per product, from the client. A failure partway left a half-populated copy that no retry could tidy up, and pressing the button again made another one.
POST /api/shopping-lists/{id}/copynow does it in one transaction, behind a modal with three independent options: products, shopping progress, and sharing settings. Progress and sharing are off by default; sharing is owner-only, enforced on the server.Usernames are unique
app_user.usernameis unique as of this release, so version 2 can invite somebody by name and mean one person. It stays nullable, because deleting an account nulls it to free the name. Seeding suffixes a digit on collision; a user-facing edit is refused with a field-level 409.Everything else
Bannerhonours its ownsize,SelectItemgained an icon slot, a hardcoded grey is gone.Related issue
None linked. Closes nothing automatically.
Type of change
Database
Two manual changes, in this order. Neither is something
ddl-auto=updatewill do: Hibernate never drops a column, and it will not add a unique index to a populated table (it logs the failure and starts anyway).1. Unique index on
app_user.username. Ideally before the deploy, but safe after: until it exists, theexistsByUsernamepre-check still returns the 409 for the ordinary case, and only a genuine concurrent collision goes unguarded. Duplicates are expected in existing data, because the old seeding took the provider display name verbatim. De-duplicate with a numeric suffix ordered bycreated_at, id, repeat until clean, thenCREATE UNIQUE INDEX CONCURRENTLY ux_app_user_username. Notedocs/AUTH.mdrecords that prod may still carry a stale unique constraint from beforec61a1a7e, so inspect before creating.2.
ALTER TABLE shopping_list DROP COLUMN share_token. After the deploy has settled, because the currently-running image still selects it. The column's unique constraint drops with it.Both against dev and prod.
Screenshots / recordings
Not attached. Best seen on the deploy preview: a shared list opened signed out, the copy modal at
?modal=shopping-list/copy, and the share modal at?modal=shopping-list/share.Verification
mvn -B verifypasses. It only compiles and packages: there is still nobackend/src/test, so nothing here proves behaviour.pnpm exec tsc --noEmit,pnpm lint,pnpm buildall clean.Manual checks worth doing on the preview before merge, since no test suite covers them: a private list opened by id while signed out must 404 rather than show a login wall, and a non-owner must see no share, delete, edit or store-optimize control.
Notes
The multi-tool
devvsmainreviewAGENTS.mdcalls for was run, over commits1ee63d40..b6368d2d. Artifacts are inreviews/REVIEW-2026-08-06-DEV-VS-MAIN-BY-AREA.mdand its HTML twin, and the findings landed across this range. The share-by-id work in #160 arrived after that review and was covered by its own review pass on the PR instead.CodeRabbit did not review the final four commits on #160 (
d11d7209through9c4f4db0); its check reported "Review rate limited". Those four fix a dead 409 handler, an unreachable provisioning retry, a cancelled offline purge and validation copy. They passed local gates only.