refactor(shopping-lists): Share a list by its own id - #160
Merged
Conversation
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
✅ Deploy Preview for disscount ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Summary by CodeRabbit
WalkthroughThis change replaces token-based shopping-list sharing with UUID-scoped list URLs and service-layer access checks. It adds configurable list copying and updates frontend loading, offline handling, caching, username conflicts, watchlist drafts, accessibility, and review artifact instructions. ChangesShopping-list sharing and frontend updates
Shopping-list copy flow
Watchlist and interface updates
Review artifact documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Viewer
participant ShoppingListController
participant ShoppingListService
participant ShoppingListItemService
Viewer->>ShoppingListController: Request a UUID-scoped list or item route
ShoppingListController->>ShoppingListService: Resolve nullable user ID
ShoppingListService->>ShoppingListItemService: Apply visibility and access checks
ShoppingListItemService-->>Viewer: Return authorized result or problem detail
sequenceDiagram
participant Owner
participant ShoppingListAction
participant CopyListModal
participant useCopyListModal
participant ShoppingListCopyService
Owner->>ShoppingListAction: Select copy
ShoppingListAction->>CopyListModal: Open with list ID
CopyListModal->>useCopyListModal: Submit copy options
useCopyListModal->>ShoppingListCopyService: Create copied list
ShoppingListCopyService-->>CopyListModal: Return copied list DTO
CopyListModal-->>Owner: Navigate to copied list
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/main/java/disscount/user/service/UserService.java (1)
95-128: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove the collision retry out of the failed transaction.
UserService.ensureActiveProfileruns under the class-level@Transactional, and asaveAndFlushuniqueness violation marks that transaction rollback-only while PostgreSQL aborts the database transaction. The fallbacksaveAndFlushinside the same method cannot commit. Run the retry in a separateREQUIRES_NEWtransaction through another bean, catch the first failure outside that transaction, then re-read byid; return if the concurrent request created the profile, or create the username-null profile in the new transaction.🤖 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 `@backend/src/main/java/disscount/user/service/UserService.java` around lines 95 - 128, Update UserService.ensureActiveProfile so the initial saveAndFlush failure is caught after its transaction ends, then re-read the profile by id and return if a concurrent request already created it. Move the username-null fallback saveAndFlush into a separate bean method using REQUIRES_NEW, and invoke that method through the bean proxy so it runs in a new transaction rather than the rollback-only transaction.backend/src/main/java/disscount/config/SecurityConfig.java (1)
93-116: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle CORS preflight before the authenticated chain.
The by-id matcher only includes GET, HEAD, PUT, DELETE, and item POST/PUT/DELETE;
OPTIONSpreflight falls through tofilterChain, which appliesanyRequest().authenticated()with no CORS configuration before authorization. Cross-origin browser calls receive 401 from the preflight and do not send the actual request. Add CORS handling before authorization while keeping OPTIONS covered.🤖 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 `@backend/src/main/java/disscount/config/SecurityConfig.java` around lines 93 - 116, Update optionalAuthShoppingListChain to enable CORS processing before authorization, and ensure its security matcher includes OPTIONS requests for the shopping-list by-id route. Preserve the existing stateless session policy, permit-all authorization, and filter ordering while allowing preflight requests to reach CORS handling instead of the authenticated fallback chain.Source: Linters/SAST tools
🤖 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 `@backend/src/main/java/disscount/config/SecurityConfig.java`:
- Around line 118-134: Update the Javadoc for shoppingListByIdMatcher to state
seven routes instead of six, matching the seven UuidScopedRequestMatcher entries
while leaving the allowlist unchanged.
In `@backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java`:
- Around line 38-47: Update UuidScopedRequestMatcher’s delegate used by matches
so it uses a variable-aware matcher such as PathPatternRequestMatcher, allowing
result.getVariables().get("id") to resolve for UUID paths. Preserve rejection of
non-matching and invalid IDs, and add tests covering both a valid UUID path and
a literal path.
In `@backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java`:
- Line 66: Update the warning log in GlobalExceptionHandler so it does not pass
the full constraint-violation exception or its cause message. Log only a
non-sensitive identifier such as the exception class or constraint name, while
preserving the existing data-integrity handling behavior.
- Around line 64-72: Update handleDataIntegrityViolation to detect and handle
only genuine duplicate-key constraint violations as HTTP 409 conflicts; for
not-null, check-constraint, foreign-key, and other integrity failures, rethrow
or otherwise delegate to the existing catch-all 500 handler. Preserve the
current username-specific conflict message for duplicate collisions.
In `@docs/AUTH.md`:
- Around line 127-130: Update the username uniqueness statement near
UserProvisioningFilter to distinguish entity and local-database declarations
from production enforcement. State that production uniqueness remains pending
the manual migration, and avoid claiming concurrent provisioning is protected
before that migration completes.
In
`@frontend/src/app/`(user)/shopping-lists/[id]/hooks/use-shopping-list-mutations.ts:
- Around line 63-69: Update the ShoppingListActionsSheet caller of handleCopy to
pass { replace: true } when opening the copy modal, matching the existing edit
and owner-sharing modal transitions. Keep handleCopy’s URL and options handling
unchanged.
In `@frontend/src/app/`(user)/shopping-lists/hooks/use-copy-list-modal.ts:
- Around line 117-125: Remove the closeModalUrl() call from the successful copy
flow in the list-copy handler, since it starts an asynchronous history
traversal. Navigate directly to shoppingListPath(copy.id) with router.push after
displaying the existing toast, replacing the modal entry through destination
navigation.
In `@frontend/src/app/`(user)/shopping-lists/hooks/use-share-list-modal.ts:
- Around line 29-32: Move the shareUrl computation into a useMemo in the
share-list modal, importing useMemo as a named React hook, so shareListUrl is
evaluated only within the existing share action flow and its try/catch toast
handling remains effective; preserve the null result when shoppingList is
absent.
In `@frontend/src/components/custom/price/price-stack.tsx`:
- Around line 38-40: Update the secondary-tier render condition in PriceStack to
exclude boolean React nodes while still allowing valid numeric values such as 0.
Preserve the existing null/undefined filtering and only render the separator and
secondary price when secondary is a renderable non-boolean value.
In `@frontend/src/components/ui/select.tsx`:
- Around line 128-132: The generated shadcn primitives must remain unmodified:
in frontend/src/components/ui/select.tsx lines 128-132, remove the
project-specific SelectItem icon contract and provide it through an owned
wrapper or generated override; likewise, in
frontend/src/components/ui/switch.tsx lines 23-40, move the custom switch sizing
rule to an owned wrapper or generated override, preserving both behaviors
without editing the primitives directly.
In `@frontend/src/constants/product-action-labels.ts`:
- Around line 9-14: Change PRODUCT_ACTION_LABELS from a named export to the
module’s default export, then update every import of PRODUCT_ACTION_LABELS to
use default-import syntax while preserving the existing constant name and
values.
---
Outside diff comments:
In `@backend/src/main/java/disscount/config/SecurityConfig.java`:
- Around line 93-116: Update optionalAuthShoppingListChain to enable CORS
processing before authorization, and ensure its security matcher includes
OPTIONS requests for the shopping-list by-id route. Preserve the existing
stateless session policy, permit-all authorization, and filter ordering while
allowing preflight requests to reach CORS handling instead of the authenticated
fallback chain.
In `@backend/src/main/java/disscount/user/service/UserService.java`:
- Around line 95-128: Update UserService.ensureActiveProfile so the initial
saveAndFlush failure is caught after its transaction ends, then re-read the
profile by id and return if a concurrent request already created it. Move the
username-null fallback saveAndFlush into a separate bean method using
REQUIRES_NEW, and invoke that method through the bean proxy so it runs in a new
transaction rather than the rollback-only transaction.
🪄 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: a289b3f4-2804-4ff5-8e03-f3ffb6ed25d3
📒 Files selected for processing (98)
.claude/skills/multi-tool-code-review/SKILL.md.claude/skills/multi-tool-code-review/references/03-triage-doc-format.mdbackend/src/main/java/disscount/config/SecurityConfig.javabackend/src/main/java/disscount/config/UuidScopedRequestMatcher.javabackend/src/main/java/disscount/exceptions/GlobalExceptionHandler.javabackend/src/main/java/disscount/exceptions/NotFoundException.javabackend/src/main/java/disscount/shoppingList/dao/ShoppingListRepository.javabackend/src/main/java/disscount/shoppingList/domain/ListAccess.javabackend/src/main/java/disscount/shoppingList/domain/ShoppingList.javabackend/src/main/java/disscount/shoppingList/dto/ShoppingListDto.javabackend/src/main/java/disscount/shoppingList/dto/ShoppingListRequest.javabackend/src/main/java/disscount/shoppingList/rest/SharedShoppingListController.javabackend/src/main/java/disscount/shoppingList/rest/ShoppingListController.javabackend/src/main/java/disscount/shoppingList/service/SharedShoppingListService.javabackend/src/main/java/disscount/shoppingList/service/ShoppingListMapper.javabackend/src/main/java/disscount/shoppingList/service/ShoppingListService.javabackend/src/main/java/disscount/shoppingListItem/rest/ShoppingListItemController.javabackend/src/main/java/disscount/shoppingListItem/service/ShoppingListItemService.javabackend/src/main/java/disscount/user/dao/UserRepository.javabackend/src/main/java/disscount/user/domain/User.javabackend/src/main/java/disscount/user/service/UserService.javadocs/AUTH.mddocs/LANDING.mddocs/MOBILE-NAV.mddocs/PWA.mddocs/README.mddocs/SHARING.mddocs/STATE-PERSISTENCE.mdfrontend/next.config.tsfrontend/sentry.server.config.tsfrontend/src/app/(user)/shopping-lists/[id]/components/items/shopping-list-items.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-access-banner.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-buttons.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-row.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-detail-client.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-header.tsxfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-unavailable.tsxfrontend/src/app/(user)/shopping-lists/[id]/hooks/use-shopping-list-actions.tsfrontend/src/app/(user)/shopping-lists/[id]/hooks/use-shopping-list-data.tsfrontend/src/app/(user)/shopping-lists/[id]/hooks/use-shopping-list-item-mutations.tsfrontend/src/app/(user)/shopping-lists/[id]/hooks/use-shopping-list-mutations.tsfrontend/src/app/(user)/shopping-lists/components/forms/copy-list-modal.tsxfrontend/src/app/(user)/shopping-lists/components/forms/copy-option-row.tsxfrontend/src/app/(user)/shopping-lists/components/forms/share-access-row.tsxfrontend/src/app/(user)/shopping-lists/components/forms/share-list-modal.tsxfrontend/src/app/(user)/shopping-lists/components/forms/shopping-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-lists-client.tsxfrontend/src/app/(user)/shopping-lists/hooks/use-copy-list-modal.tsfrontend/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/shopping-list-access.tsfrontend/src/app/(user)/watchlist/components/create-discounted-list-button.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/watchlist-action-button.tsxfrontend/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/robots.tsfrontend/src/app/s/[token]/components/shared-shopping-list-client.tsxfrontend/src/app/s/[token]/get-shared-list-preview.tsfrontend/src/app/s/[token]/page.tsxfrontend/src/app/sw.tsfrontend/src/components/custom/common/block-loading-spinner.tsxfrontend/src/components/custom/common/labeled-select.tsxfrontend/src/components/custom/form/stepper-number-input.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/modal-router/entity-modal-outlet.tsxfrontend/src/components/custom/price/price-info-button.tsxfrontend/src/components/custom/price/price-stack.tsxfrontend/src/components/custom/product/product-quick-actions-list.tsxfrontend/src/components/custom/settings/tabs/profile-tab.tsxfrontend/src/components/ui/select.tsxfrontend/src/components/ui/switch.tsxfrontend/src/constants/product-action-labels.tsfrontend/src/constants/protected-routes.tsfrontend/src/hooks/use-preselected-list-membership.tsfrontend/src/instrumentation-client.tsfrontend/src/lib/api/schemas/shopping-list.tsfrontend/src/lib/api/shopping-lists/hooks.tsfrontend/src/lib/api/shopping-lists/keys.tsfrontend/src/lib/api/shopping-lists/queries.tsfrontend/src/lib/modal/modal-registry.tsfrontend/src/lib/offline/cached-query-keys.tsfrontend/src/lib/offline/list-write-failed.tsfrontend/src/lib/offline/offline-mutation-keys.tsfrontend/src/lib/offline/offline-mutations.tsfrontend/src/lib/offline/persister.tsfrontend/src/lib/offline/purge.tsfrontend/src/lib/sentry/scrub-share-token.tsfrontend/src/utils/browser/storage/drafts.tsfrontend/src/utils/shopping-list-links.ts
💤 Files with no reviewable changes (12)
- frontend/src/lib/sentry/scrub-share-token.ts
- backend/src/main/java/disscount/shoppingList/rest/SharedShoppingListController.java
- frontend/src/app/s/[token]/components/shared-shopping-list-client.tsx
- frontend/src/app/s/[token]/page.tsx
- frontend/src/lib/api/schemas/shopping-list.ts
- frontend/sentry.server.config.ts
- frontend/src/app/s/[token]/get-shared-list-preview.ts
- backend/src/main/java/disscount/shoppingList/service/SharedShoppingListService.java
- backend/src/main/java/disscount/shoppingList/service/ShoppingListMapper.java
- frontend/src/lib/api/shopping-lists/queries.ts
- frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-header.tsx
- frontend/src/lib/offline/cached-query-keys.ts
Comment on lines
+118
to
+134
| /** | ||
| * Exactly the six routes an anonymous caller may reach. Listing methods explicitly means | ||
| * anything else, including PATCH and OPTIONS, falls through to the authenticated chain, | ||
| * so the default is deny. | ||
| */ | ||
| private static RequestMatcher shoppingListByIdMatcher() { | ||
| return new OrRequestMatcher( | ||
| new UuidScopedRequestMatcher(HttpMethod.GET, "/api/shopping-lists/{id}"), | ||
| // HEAD as well as GET: link unfurlers and crawlers probe a shared URL with | ||
| // it, and AntPathRequestMatcher compares the method exactly, so without | ||
| // this a publicly viewable list answers 401 to a HEAD. | ||
| new UuidScopedRequestMatcher(HttpMethod.HEAD, "/api/shopping-lists/{id}"), | ||
| new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}"), | ||
| new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}"), | ||
| new UuidScopedRequestMatcher(HttpMethod.POST, "/api/shopping-lists/{id}/items"), | ||
| new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}/items/{itemId}"), | ||
| new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}/items/{itemId}")); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the route count in the Javadoc.
The comment states six routes. The matcher lists seven entries. In a security allowlist, a reader compares the stated count against the entries during an audit, so the count must match.
📝 Proposed documentation fix
/**
- * Exactly the six routes an anonymous caller may reach. Listing methods explicitly means
+ * Exactly the seven routes an anonymous caller may reach. Listing methods explicitly means
* anything else, including PATCH and OPTIONS, falls through to the authenticated chain,
* so the default is deny.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Exactly the six routes an anonymous caller may reach. Listing methods explicitly means | |
| * anything else, including PATCH and OPTIONS, falls through to the authenticated chain, | |
| * so the default is deny. | |
| */ | |
| private static RequestMatcher shoppingListByIdMatcher() { | |
| return new OrRequestMatcher( | |
| new UuidScopedRequestMatcher(HttpMethod.GET, "/api/shopping-lists/{id}"), | |
| // HEAD as well as GET: link unfurlers and crawlers probe a shared URL with | |
| // it, and AntPathRequestMatcher compares the method exactly, so without | |
| // this a publicly viewable list answers 401 to a HEAD. | |
| new UuidScopedRequestMatcher(HttpMethod.HEAD, "/api/shopping-lists/{id}"), | |
| new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}"), | |
| new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}"), | |
| new UuidScopedRequestMatcher(HttpMethod.POST, "/api/shopping-lists/{id}/items"), | |
| new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}/items/{itemId}"), | |
| new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}/items/{itemId}")); | |
| /** | |
| * Exactly the seven routes an anonymous caller may reach. Listing methods explicitly means | |
| * anything else, including PATCH and OPTIONS, falls through to the authenticated chain, | |
| * so the default is deny. | |
| */ | |
| private static RequestMatcher shoppingListByIdMatcher() { | |
| return new OrRequestMatcher( | |
| new UuidScopedRequestMatcher(HttpMethod.GET, "/api/shopping-lists/{id}"), | |
| // HEAD as well as GET: link unfurlers and crawlers probe a shared URL with | |
| // it, and AntPathRequestMatcher compares the method exactly, so without | |
| // this a publicly viewable list answers 401 to a HEAD. | |
| new UuidScopedRequestMatcher(HttpMethod.HEAD, "/api/shopping-lists/{id}"), | |
| new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}"), | |
| new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}"), | |
| new UuidScopedRequestMatcher(HttpMethod.POST, "/api/shopping-lists/{id}/items"), | |
| new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}/items/{itemId}"), | |
| new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}/items/{itemId}")); |
🤖 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 `@backend/src/main/java/disscount/config/SecurityConfig.java` around lines 118
- 134, Update the Javadoc for shoppingListByIdMatcher to state seven routes
instead of six, matching the seven UuidScopedRequestMatcher entries while
leaving the allowlist unchanged.
Comment on lines
+38
to
+47
| @Override | ||
| public boolean matches(HttpServletRequest request) { | ||
| MatchResult result = delegate.matcher(request); | ||
| if (!result.isMatch()) { | ||
| return false; | ||
| } | ||
|
|
||
| String id = result.getVariables().get("id"); | ||
| return id != null && UUID_SHAPE.matcher(id).matches(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the Spring Security version and the matcher API surface.
set -euo pipefail
fd -g 'pom.xml' backend --exec rg -n -C3 'spring-boot-starter-parent|spring-security|<version>' {}
fd -g 'build.gradle*' backend --exec rg -n -C3 'springframework.security|springframework.boot' {}Repository: OffCrazyFreak/Disscount
Length of output: 1645
🌐 Web query:
Spring Security AntPathRequestMatcher matcher getVariables URI template variables
💡 Result:
In Spring Security, the AntPathRequestMatcher.extractUriTemplateVariables(HttpServletRequest request) method is deprecated [1][2][3]. While AntPathRequestMatcher implements the RequestVariablesExtractor interface, its primary purpose is request matching, and the ability to extract URI template variables via this class is no longer the recommended or standard way to handle path variables within Spring Security's authorization workflow [4][1][5]. If you need to access and use URI template variables for authorization purposes, the following approaches are recommended: 1. Spring Expression Language (SpEL) in Authorization Rules: You can use template variables directly in your security configuration using SpEL. If you define a pattern with a path variable (e.g., /user/{id}), you can reference that variable in an .access() expression. For example:.antMatchers("/user/{id}/").access("@mySecurityGuard.check(#id, authentication)") In this case, Spring Security automatically resolves the #id placeholder from the path variable [6]. 2. Use PathPatternRequestMatcher: For modern Spring MVC applications, Spring Security recommends using PathPatternRequestMatcher [7]. This matcher integrates more deeply with Spring MVC's URL matching logic and is better suited for applications that utilize sophisticated path pattern parsing [7]. 3. HandlerMapping: If you are working outside the standard security expression context and need to access the variables manually, you can retrieve them from the request attributes using the constant HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE [6]. The extractUriTemplateVariables method on AntPathRequestMatcher is deprecated and should be avoided in favor of these more integrated approaches [1][3][8].
Citations:
- 1: https://docs.enterprise.spring.io/spring-security/docs/5.8.22/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 2: https://docs.spring.io/spring-security/site/docs/6.5.x-SNAPSHOT/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 3: https://docs.spring.io/spring-security/site/docs/6.4.9-SNAPSHOT/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 4: https://docs.spring.io/spring-security/site/docs/6.3.5/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 5: https://www.springframework.org/spring-security/reference/6.5-SNAPSHOT/api/java/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 6: https://stackoverflow.com/questions/33115446/authorization-in-spring-security-based-on-path-variables
- 7: https://docs.spring.io/spring-security/reference/servlet/integrations/mvc.html
- 8: https://docs.spring.io/spring-security/site/docs/6.2.x/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pom files and Spring versions =="
for f in $(fd -g 'pom.xml' backend); do
echo "-- $f"
sed -n '1,140p' "$f" | nl -ba
done
echo "== UuidScopedRequestMatcher source =="
for f in $(fd -g 'UuidScopedRequestMatcher.java' backend/src/main/java); do
echo "-- $f"
wc -l "$f"
sed -n '1,140p' "$f" | nl -ba
done
echo "== matcher usages =="
rg -n "UuidScopedRequestMatcher|new AntPathRequestMatcher|matchers\\(|requestMatchers" backend/src/main/java backend/src/test/java || true
echo "== Search package-lock / dependency versions for org.springframework.security artifacts =="
rg -n "org:springframework:security|versioning|springSecurity" backend pom.xml backend/pom.xml backend/src/main/java backend/src/test/java 2>/dev/null || trueRepository: OffCrazyFreak/Disscount
Length of output: 256
🌐 Web query:
Spring Security 6.1.3 AntPathRequestMatcher RequestMatcher matcher getVariables API ant match variables request result
💡 Result:
In Spring Security 6.1.3, the AntPathRequestMatcher class is designed for pattern matching against an HttpServletRequest URL (servletPath + pathInfo) and does not natively support capturing URI template variables via the matcher API in the way Spring MVC does [1][2]. While AntPathRequestMatcher implements the RequestVariablesExtractor interface, the implementation of matcher(HttpServletRequest) returns a MatchResult where the default implementation of getVariables returns an empty map [1]. Additionally, the method extractUriTemplateVariables(HttpServletRequest) is marked as @Deprecated [1]. If your goal is to extract path variables from a request within Spring Security, you should consider the following approaches: 1. Use MvcRequestMatcher: This class utilizes Spring MVC's HandlerMappingIntrospector to match paths and is the standard way to handle path variables within Spring Security [3]. 2. Use PathPatternRequestMatcher: Available in more recent versions of Spring Security (specifically introduced/standardized for this purpose in later versions), this matcher is explicitly designed to handle path patterns with placeholders (e.g., /path/{value}/) and capture those values in RequestAuthorizationContext.getVariables [4][5]. For simple Ant-style matching where no variable extraction is required, AntPathRequestMatcher remains functional, but it should not be relied upon for extracting dynamic path parameters [1][2].
Citations:
- 1: https://docs.spring.io/spring-security/site/docs/6.1.4/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 2: https://docs.spring.io/spring-security/site/docs/6.0.3/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 3: https://docs.spring.io/spring-security/site/docs/6.2.x/api/org/springframework/security/web/servlet/util/matcher/MvcRequestMatcher.html
- 4: https://www.springframework.org/spring-security/reference/6.5/api/java/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.Builder.html
- 5: https://www.springframework.org/spring-security/reference/6.5/api/java/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import subprocess, pathlib, re, os
os.chdir('/testbed') # sandbox cwd
root = pathlib.Path('backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java')
if root.exists():
lines = root.read_text().splitlines()
for i, line in enumerate(lines, start=1):
print(f"{i:4}: {line}")
else:
print("UuidScopedRequestMatcher.java not found")
pom = pathlib.Path('backend/pom.xml')
if pom.exists():
s = pom.read_text()
m = re.search(r"<spring-boot-starter-parent>.*?</spring-boot-starter-parent>", s, re.S)
print("\nBackend Spring parent block:")
print(m.group(0) if m else "none")
PYRepository: OffCrazyFreak/Disscount
Length of output: 301
🌐 Web query:
Spring Security 6.1.4 AntPathRequestMatcher RequestMatcher matcher HttpServletRequest MatchResult getVariables implementation
💡 Result:
In Spring Security 6.1.4, the AntPathRequestMatcher implementation of the matcher(HttpServletRequest) method does not automatically extract path variables into the returned MatchResult [1]. The RequestMatcher interface defines the matcher(HttpServletRequest) method, which returns a MatchResult object [2][3]. By default, the RequestMatcher interface provides a default implementation for matcher(request) that invokes the matches(request) method and returns a MatchResult containing an empty map for variables [3]. While AntPathRequestMatcher implements RequestVariablesExtractor [1], its primary matcher(HttpServletRequest) method is generally used for simple boolean matching [1]. If you require extracted path variables (such as template variables), the recommended approach in modern Spring Security (especially for Spring MVC integration) is to use PathPatternRequestMatcher instead of AntPathRequestMatcher, as it is designed to align with Spring MVC's path pattern parsing and variable extraction logic [4]. Key details regarding the MatchResult and variables: 1. RequestMatcher.MatchResult: This class holds the match status (boolean) and a map of variables [5][3]. The getVariables method returns a Map<String, String> containing key-value pairs of extracted variable names and values [5][6]. 2. Default Behavior: When matcher(HttpServletRequest) is called on an AntPathRequestMatcher, it does not populate this map with path variables; it returns Collections.emptyMap [1][3]. 3. Alternative: For path variable extraction, use PathPatternRequestMatcher, which integrates directly with Spring MVC's pattern parsing [4].
Citations:
- 1: https://docs.spring.io/spring-security/site/docs/6.1.4/api/org/springframework/security/web/util/matcher/AntPathRequestMatcher.html
- 2: https://docs.spring.vmware.com/spring-security/docs/6.1.14/api/org/springframework/security/web/util/matcher/RequestMatcher.html
- 3: https://github.com/spring-projects/spring-security/blob/master/web/src/main/java/org/springframework/security/web/util/matcher/RequestMatcher.java
- 4: https://docs.spring.io/spring-security/reference/servlet/integrations/mvc.html
- 5: https://docs.spring.io/spring-security/site/docs/6.1.4/api/org/springframework/security/web/util/matcher/RequestMatcher.MatchResult.html
- 6: https://docs.spring.io/spring-security/reference/api/java/org/springframework/security/web/util/matcher/RequestMatcher.MatchResult.html
🌐 Web query:
Spring Security 6.1.4 PathPatternRequestMatcher builder patternMatch getVariables URI variables
💡 Result:
In Spring Security, the PathPatternRequestMatcher.Builder allows you to define path patterns that can capture URI variables, which are then accessible through the RequestAuthorizationContext. When using patterns like /path/{value}/, the {value} segment is captured as a URI variable [1][2][3]. To use this functionality, you create a matcher using the builder, for example: PathPatternRequestMatcher.Builder mvc = PathPatternRequestMatcher.withDefaults.basePath("/api"); http.authorizeHttpRequests(authorize -> authorize.requestMatchers(mvc.matcher("/user/{id}/")).hasAuthority("user")); When a request matches the pattern, the captured variables (such as "id") are stored in the RequestAuthorizationContext [1][2][3]. You can access these variables during authorization by using the RequestAuthorizationContext object in your custom authorization logic: http.authorizeHttpRequests(authorize -> authorize.requestMatchers(mvc.matcher("/user/{id}/")).access((authentication, context) -> { Map<String, Object> variables = context.getVariables; String userId = (String) variables.get("id"); // Apply custom logic using userId return new AuthorizationDecision(true); })); Note that the PathPatternRequestMatcher returns a MatchResult when matcher(HttpServletRequest) is called [4][5][6]. While the default implementation of some matcher methods might return empty maps, the framework-level integration with authorizeHttpRequests automatically handles the extraction and population of these variables into the RequestAuthorizationContext for you to access as shown above [1][2][3]. Top results: [1][2][3]
Citations:
- 1: https://www.springframework.org/spring-security/reference/6.5/api/java/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.Builder.html
- 2: https://docs.spring.io/spring-security/site/docs/6.5.1/api/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.Builder.html
- 3: https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.Builder.html
- 4: https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.html
- 5: https://docs.spring.io/spring-security/reference/api/java/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.html
- 6: https://www.springframework.org/spring-security/reference/api/java/org/springframework/security/web/servlet/util/matcher/PathPatternRequestMatcher.html
Use a matcher that captures path variables.
With Spring Boot 3.1, AntPathRequestMatcher.matcher(...) returns a MatchResult, but getVariables() stays empty, so this wrapper always rejects {id} paths. Replace the delegate with PathPatternRequestMatcher or another variable-aware matcher, and add tests for a UUID path and a literal path.
🤖 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 `@backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java` around
lines 38 - 47, Update UuidScopedRequestMatcher’s delegate used by matches so it
uses a variable-aware matcher such as PathPatternRequestMatcher, allowing
result.getVariables().get("id") to resolve for UUID paths. Preserve rejection of
non-matching and invalid IDs, and add tests covering both a valid UUID path and
a literal path.
Comment on lines
+128
to
+132
| // Constrained rather than a bare ElementType: the render below hands the component a | ||
| // className and aria-hidden, and an unconstrained type promises nothing about either. | ||
| icon?: React.ElementType< | ||
| Pick<React.SVGProps<SVGSVGElement>, "className" | "aria-hidden"> | ||
| >; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list matching ui select/switch =="
git ls-files 'frontend/src/components/ui/select.tsx' 'frontend/src/components/ui/switch.tsx' | sed -n '1,20p'
echo
echo "== select.tsx relevant content =="
wc -l frontend/src/components/ui/select.tsx frontend/src/components/ui/switch.tsx
sed -n '1,180p' frontend/src/components/ui/select.tsx
echo
echo "== switch.tsx relevant content =="
sed -n '1,120p' frontend/src/components/ui/switch.tsx
echo
echo "== git diff stat/name-only for relevant files if available =="
git diff --stat frontend/src/components/ui/select.tsx frontend/src/components/ui/switch.tsx || true
git diff --unified=8 frontend/src/components/ui/select.tsx frontend/src/components/ui/switch.tsx | sed -n '1,240p' || true
echo
echo "== repo files likely indicating shadcn/config/component generation ====="
git ls-files | rg '(^|/)(components\.json|shadcn|ui\/|components/\.source|tailwind\.config\.cjs|tailwind\.config\.ts|package\.json)' | sed -n '1,160p'
if [ -f frontend/src/components/ui/package.json ] || [ -f frontend/components.json ]; then
echo "== components/config =="
for f in frontend/components.json frontend/src/components/ui/package.json frontend/package.json; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
fiRepository: OffCrazyFreak/Disscount
Length of output: 14391
Do not hand-edit the shadcn UI primitives.
frontend/src/components/ui/select.tsx and frontend/src/components/ui/switch.tsx are part of the generated shadcn UI component set under frontend/src/components/ui/**/*.{ts,tsx}. Keep project-specific behavior behind owned wrappers or add/use generated overrides.
- Move the
SelectItemicon contract out offrontend/src/components/ui/select.tsx#128-L132. - Move the custom switch sizing rule out of
frontend/src/components/ui/switch.tsx#23-L40.
📍 Affects 2 files
frontend/src/components/ui/select.tsx#L128-L132(this comment)frontend/src/components/ui/switch.tsx#L23-L40
🤖 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 128 - 132, The generated
shadcn primitives must remain unmodified: in
frontend/src/components/ui/select.tsx lines 128-132, remove the project-specific
SelectItem icon contract and provide it through an owned wrapper or generated
override; likewise, in frontend/src/components/ui/switch.tsx lines 23-40, move
the custom switch sizing rule to an owned wrapper or generated override,
preserving both behaviors without editing the primitives directly.
Source: Coding guidelines
Comment on lines
+9
to
+14
| export const PRODUCT_ACTION_LABELS = { | ||
| addToList: "Dodaj na popis za kupnju", | ||
| editListEntry: "Uredi unos na popisu za kupnju", | ||
| watch: "Prati proizvod", | ||
| editWatch: "Ažuriraj praćenje", | ||
| } as const; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a default export for PRODUCT_ACTION_LABELS.
This module has one primary export. Export it as the default and update its imports.
Proposed fix
-export const PRODUCT_ACTION_LABELS = {
+const PRODUCT_ACTION_LABELS = {
addToList: "Dodaj na popis za kupnju",
editListEntry: "Uredi unos na popisu za kupnju",
watch: "Prati proizvod",
editWatch: "Ažuriraj praćenje",
} as const;
+
+export default PRODUCT_ACTION_LABELS;As per coding guidelines: “Prefer default exports wherever possible.”
🤖 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/constants/product-action-labels.ts` around lines 9 - 14, Change
PRODUCT_ACTION_LABELS from a named export to the module’s default export, then
update every import of PRODUCT_ACTION_LABELS to use default-import syntax while
preserving the existing constant name and values.
Source: Coding guidelines
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.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/SHARING.md (1)
90-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument all optional-authentication routes.
Line 90 states that six routes use optional bearer authentication.
SecurityConfig.shoppingListByIdMatcher()defines eight matcher entries. It includesPOST /api/shopping-lists/{id}/copyandHEAD /api/shopping-lists/{id}. Add the copy route to the table and describe the seven application routes plus the HEAD matcher.🤖 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/SHARING.md` at line 90, Update the optional-authentication route documentation in docs/SHARING.md to include POST /api/shopping-lists/{id}/copy in the route table and accurately describe all seven application routes plus the HEAD matcher defined by SecurityConfig.shoppingListByIdMatcher().backend/src/main/java/disscount/user/service/UserService.java (1)
94-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not retry the insert in the same transaction.
ensureActiveProfile()is transactional, so asaveAndFlushconstraint failure marks the transaction rollback-only. The fallbacksaveAndFlushat line 111 cannot commit, then profile provisioning fails. Also, ifidalready exists, this fallback still tries to insert the sameid. Run the insert retry inREQUIRES_NEW, and load the existing user whenidis present; only retry without username after a username constraint violation.🤖 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 `@backend/src/main/java/disscount/user/service/UserService.java` around lines 94 - 120, Update ensureActiveProfile() so the initial user insert and username-collision fallback do not retry within the rollback-only transaction: delegate the retry to a separate REQUIRES_NEW operation. When the supplied id already exists, load and reuse that User instead of inserting the same id; only perform the nameless retry for a username constraint violation, preserving the existing profile provisioning behavior.backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java (1)
62-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent JPA constraint failures from returning 500 when they are unique-key conflicts.
@ExceptionHandler(DuplicateKeyException.class)does not catch genericDataIntegrityViolationExceptionfrom the JPA path. Add aDataIntegrityViolationExceptionhandler before the generic exception fallback, inspect the cause chain fororg.hibernate.exception.ConstraintViolationException, and return the 409 for unique-constraint violations only.🤖 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 `@backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java` around lines 62 - 87, Add a DataIntegrityViolationException handler in GlobalExceptionHandler before the generic exception fallback; inspect the full cause chain for Hibernate ConstraintViolationException and return the existing 409 ProblemDetail only when it represents a unique-constraint violation, while allowing other integrity failures to retain their current handling. Reuse constraintNameOf or update it to traverse the cause chain consistently, and avoid treating every DataIntegrityViolationException as a conflict.
🤖 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 `@frontend/src/lib/api/schemas/shopping-list.ts`:
- Around line 47-48: Update the validation messages in the shopping-list title
schema to use direct informal Croatian wording addressing the user with “ti,”
while preserving the existing minimum and maximum length rules and ungendered
phrasing.
---
Outside diff comments:
In `@backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java`:
- Around line 62-87: Add a DataIntegrityViolationException handler in
GlobalExceptionHandler before the generic exception fallback; inspect the full
cause chain for Hibernate ConstraintViolationException and return the existing
409 ProblemDetail only when it represents a unique-constraint violation, while
allowing other integrity failures to retain their current handling. Reuse
constraintNameOf or update it to traverse the cause chain consistently, and
avoid treating every DataIntegrityViolationException as a conflict.
In `@backend/src/main/java/disscount/user/service/UserService.java`:
- Around line 94-120: Update ensureActiveProfile() so the initial user insert
and username-collision fallback do not retry within the rollback-only
transaction: delegate the retry to a separate REQUIRES_NEW operation. When the
supplied id already exists, load and reuse that User instead of inserting the
same id; only perform the nameless retry for a username constraint violation,
preserving the existing profile provisioning behavior.
In `@docs/SHARING.md`:
- Line 90: Update the optional-authentication route documentation in
docs/SHARING.md to include POST /api/shopping-lists/{id}/copy in the route table
and accurately describe all seven application routes plus the HEAD matcher
defined by SecurityConfig.shoppingListByIdMatcher().
🪄 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: 4e2cd79d-6105-437d-bd80-824a40cff8e1
📒 Files selected for processing (27)
AGENTS.mdbackend/src/main/java/disscount/config/SecurityConfig.javabackend/src/main/java/disscount/config/UuidScopedRequestMatcher.javabackend/src/main/java/disscount/exceptions/GlobalExceptionHandler.javabackend/src/main/java/disscount/shoppingList/dto/ShoppingListCopyRequest.javabackend/src/main/java/disscount/shoppingList/rest/ShoppingListController.javabackend/src/main/java/disscount/shoppingList/service/ShoppingListCopyService.javabackend/src/main/java/disscount/shoppingList/service/ShoppingListService.javabackend/src/main/java/disscount/user/service/UserService.javadocs/AUTH.mddocs/PWA.mddocs/SHARING.mdfrontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-access-banner.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/hooks/use-copy-list-modal.tsfrontend/src/app/(user)/shopping-lists/hooks/use-share-list-modal.tsfrontend/src/app/(user)/shopping-lists/utils/link-access-copy.tsfrontend/src/components/custom/price/price-stack.tsxfrontend/src/hooks/use-preselected-list-membership.tsfrontend/src/instrumentation-client.tsfrontend/src/lib/api/schemas/shopping-list.tsfrontend/src/lib/api/shopping-lists/hooks.tsfrontend/src/lib/api/shopping-lists/queries.tsfrontend/src/lib/offline/list-write-failed.tsfrontend/src/lib/offline/persister.tsfrontend/src/lib/offline/purge.ts
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.
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.
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.
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.
5 tasks
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
Sharing moves from a rotating capability token to the list's own id, so
/shopping-lists/<id>is both the URL an owner sees and the one worth sending. Follows the agreed spec on the "Share shopping list with other users (v1)" roadmap item.Four things ride along: usernames become unique, copying a list gains an options modal, two select-icon defects are fixed, and the outstanding review triage is applied.
Why
The token bought rotation: turning sharing off nulled it, turning it back on minted a new one, so anyone holding the old link genuinely lost access. That is real and it is given up deliberately. A grocery list is shared once, used for a week and abandoned, so the scenario rotation protects against does not arrive often enough to pay for. What it cost was that an owner copying their own URL handed out something that failed for everyone else, with nothing in the URL to explain why. Google Docs works the same way and cannot rotate a link at all.
The accepted trade
While a list is shared, its id is the capability, and it is not scrubbed from telemetry. Unlike the token it replaced, the id is the application's identifier everywhere (paths, query keys, IndexedDB), so redacting it would blind every shopping-list trace rather than protect one route. It is inert the moment the list is not shared, and both leak surfaces are ours. Recorded in
docs/SHARING.md§3 in those words.Session Replay is the exception and was not part of that trade: it records the DOM, so a recording of a shared list carries the list itself. Those pages stay excluded from recording, as they were under
/s/.Security shape
The by-id routes moved onto the
@Order(1)optional-bearer chain, whose rule ispermitAll. The service-layer access checks are now an authentication boundary rather than a convenience, and theSecurityConfigjavadoc says so.The matcher is an allowlist on both axes: method plus a UUID-shaped id.
/api/shopping-lists/meand/api/shopping-lists/itemsstay authenticated because they are not UUIDs, not because they are named as exceptions, so a future literal route is excluded automatically. A wildcard-minus-exceptions matcher would have silently exposed the next one added.ShoppingListService.findVisibleis the single loader and answers 404, never 403, for anything the caller may not see.findActiveByIdAndOwneris deleted so nothing can reach for the old semantics.Review
Three Opus 5 subagents (backend security, frontend merge, conventions) plus the CodeRabbit CLI. Their findings are fixed in the last two commits. The ones worth naming:
CACHE_BUSTERbump was a regression. A mismatch makespersistQueryClientcallremoveClient(), discarding the whole persisted client including queued writes under keys this branch never touched. Reverted, with tombstone defaults for the two retired mutation keys instead.ensureActiveProfile's catch was dead code. The id is assigned rather than generated, sosave()merges and the insert lands at commit, after the catch is out of scope. A first-login username collision would have been a 500. Now flushed and retried.onErrorreplaces the mutation default rather than running beside it. Only replays after a reload ever explained themselves.Database
Two manual steps, in this order. The SQL is in the chat; summary here.
app_user.usernameand add the unique index.ddl-auto=updatewill not add it to a populated table and logs the failure rather than refusing to start, so without this the 409 has no backstop. Production may also still carry a stale constraint from beforec61a1a7e, so check first.ALTER TABLE shopping_list DROP COLUMN IF EXISTS share_token;. The running image still selects it, so this cannot go first.Not done
backend/src/testdoes not exist, so an authentication boundary moved with the security matcher and six service guards verified only by reading. This is the largest remaining risk and I would put a MockMvc suite at an hour or two./s/<token>links break, as agreed.Verification
Full CI job locally: typecheck, eslint, prettier on
src, prettier on docs and root markdown, production build, andmvn -B clean verifyfrom scratch. The backend has no test suite, soverifyproves it compiles and packages, nothing more.Worth a manual pass before merge: open a shared list signed out, open a private list by id signed out (must be 404, not a login wall), copy the URL from the address bar as owner and open it in a private window, and confirm no owner-only control renders for a non-owner.