feat(bookmarks): production harden saved bounty system - #188
Conversation
- Replace in-memory bookmark storage with GraphQL-backed persistence - Introduce O(1) Set-based bookmark lookups for UI and notifications - Split bookmark state into ids() and list() caches for consistency - Fix optimistic updates to avoid undefined state and fake bounty objects - Ensure atomic rollback across all bookmark-related caches - Improve notification logic with meaningful-change filtering and unique IDs - Fix authentication UX by removing inconsistent disabled states - Add /bookmarks/ids endpoint for optimized state checks - Remove legacy BookmarkService and unused in-memory logic
|
Someone is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@Shadow-MMN Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds server-backed bookmark functionality: GraphQL schema and operations, codegen updates, server API endpoints, React Query hooks with optimistic updates, UI components (bookmark button, saved page), and notification wiring for bookmarked bounty status changes. Changes
Sequence DiagramsequenceDiagram
participant User as User (browser)
participant UI as BookmarkButton (client)
participant Cache as React Query Cache
participant API as /api/bookmarks/[bountyId]
participant GraphQL as GraphQL Server
User->>UI: Click bookmark
UI->>Cache: cancel queries & snapshot caches
UI->>Cache: optimistic update ids & list caches
UI->>UI: show loading state
UI->>API: POST /api/bookmarks/{bountyId}
API->>GraphQL: ToggleBookmark mutation via graphqlRequest
GraphQL-->>API: return Bookmark | null
API-->>UI: response (200/500)
alt success
UI->>Cache: invalidate/read latest ids
UI->>UI: show final state & toast
else error
UI->>Cache: rollback snapshots
UI->>UI: show error toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
components/ui/stellar-link.tsx (1)
135-135: Optimization is correct, but appears unrelated to bookmark PR.Removing
valuefrom the dependency array is correct becausevalueis not used in thetooltipTextcomputation. The tooltip text only depends onisValid,type,network,explorer, andtooltipPrefix. SinceisValidalready depends onvalue(line 76), changes tovaluethat affect validity will still trigger tooltip recomputation through theisValiddependency.This optimization avoids unnecessary recomputation when
valuechanges between two valid inputs of the same type, improving performance.However, this change doesn't appear related to the bookmark system (the stated focus of this PR). Consider keeping unrelated optimizations in separate PRs for cleaner git history and easier review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/stellar-link.tsx` at line 135, The change removing value from the dependency array of the tooltipText computation is correct (tooltipText only depends on isValid, type, network, explorer, tooltipPrefix), but it’s unrelated to the bookmark work in this PR—either move this optimization into its own small PR or revert it here and add it in a follow-up; reference the tooltipText computation and the dependency array that now reads [type, network, explorer, isValid, tooltipPrefix] in components/ui/stellar-link.tsx and ensure isValid continues to reflect value changes so tooltip updates remain correct.app/api/bookmarks/ids/route.ts (1)
19-32: Reuse the generated typed document instead of an inline query string.A typed
BookmarksDocumentalready exists inlib/graphql/generated.tsand a dedicatedBookmarksoperation lives inlib/graphql/operations/bookmark-operations.graphql. Hand-rolling a newGetBookmarkIdsstring here:
- Bypasses codegen's type safety (the response is
any-cast via the explicit generic).- Creates a second source of truth that won't be updated when the schema changes.
Consider either:
- Adding a small
BookmarkIdsoperation tobookmark-operations.graphqlselecting onlybountyId, then importing the generated document; or- Reusing
BookmarksDocumenthere (over-fetches, but is type-safe and consistent withapp/api/bookmarks/route.ts).Same applies to
app/api/bookmarks/route.ts(see related comment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/bookmarks/ids/route.ts` around lines 19 - 32, Replace the inline BOOKMARKS_QUERY and manual typing with the generated document: import and use the existing BookmarksDocument (or add a new BookmarkIds operation in bookmark-operations.graphql and use its generated document) when calling graphqlRequest so you get codegen types instead of the explicit generic; update the call site that currently references BOOKMARKS_QUERY to pass the generated document (e.g., BookmarksDocument or BookmarkIdsDocument) and then map data.bookmarks to extract bountyId as before.app/saved/page.tsx (1)
8-10: Optional: preserve return path on auth redirect.Consider passing the original path so the user lands back on
/savedafter signing in, e.g.redirect("/auth?redirect=/saved")(matching whatever query param/authalready supports). Not blocking — current behavior satisfies the acceptance criteria.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/saved/page.tsx` around lines 8 - 10, Update the redirect inside the authentication check so the original path is preserved and the user returns to /saved after signing in: in the if (!user) { redirect("/auth"); } block, change the redirect to include a redirect query parameter containing the current request path (or '/saved' if you prefer) and ensure the path is URL-encoded; use the same symbol names shown (the if (!user) guard and redirect(...) call) so the auth page can read the query (e.g. redirect=?redirect=...) and navigate back after successful signin.lib/graphql/client.ts (2)
50-55:normalizeDocumentworks, but loses the typed-document metadata.
TypedDocumentStringcarries__meta__and the parameterized result/variable types, both of which are dropped byquery.toString(). For the runtime path this is fine (graphql-request only needs the query string), but anything downstream that introspects the document (e.g., persisted-query plugins, query whitelisting) will see only the raw string. If you ever add such middleware, you’ll need to pass the typed document through unchanged when possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graphql/client.ts` around lines 50 - 55, normalizeDocument currently converts non-string GraphQL documents to a string via query.toString(), which strips TypedDocumentString metadata like __meta__ and typed parameter/result info; update normalizeDocument to return the original query object unchanged when it is a TypedDocumentString (or otherwise carries __meta__) so downstream middleware can access its metadata, i.e., detect the TypedDocumentString shape (presence of __meta__ or instanceof TypedDocumentString) and return query directly instead of query.toString(), while still falling back to toString() for other non-string document types.
75-83: Double-castas unknown as RequestOptions<TVariables, TData>defeats the type system here.The
as unknown as ...escape hatch suppresses any future API drift ingraphql-request(e.g., variables required when the document has non-optional vars). Given this fetcher fans out to every codegen hook in the app, it's worth a tighter, narrowly-typed wrapper rather than a blanket cast. Ifgraphql-request's overloads make a clean signature impossible, please at least leave a comment pointing at the exact constraint being worked around so future maintainers don't widen the hole further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graphql/client.ts` around lines 75 - 83, The double-cast on requestOptions should be removed and replaced with a narrower, explicit typing and a short comment documenting the graphql-request overload constraint being worked around: construct the object into a variable named requestOptions and either declare it with the exact type RequestOptions<TVariables, TData> (or use a Partial-to-complete pattern) so the compiler enforces shape for document/variables/requestHeaders, and only resort to a single, justified cast if an overload prevents a clean assignment; add a one-line comment referencing the graphql-request overload issue (e.g., "workaround for graphql-request RequestOptions overloads requiring different shapes for documents with required variables") so future maintainers don't widen the type hole, and keep graphQLClient.request<TData, TVariables>(requestOptions) as the call site.app/saved/saved-client.tsx (3)
64-72:window.location.hreftriggers a full page reload — switch to Next.js router for SPA navigation.Using
window.location.hrefhere forces a hard navigation, throwing away the React Query cache, scroll position, layouts, and theBountyCardinteraction state. The rest of this app is App Router-based, so navigation should go throughuseRouter().push()(or wrap the card in a<Link>).🛠️ Proposed change
+import { useRouter } from "next/navigation"; @@ function SavedBountiesClient() { + const router = useRouter(); const { data: bookmarks, isLoading, error } = useBookmarks(); @@ {bookmarkedBounties.map((bounty) => ( <BountyCard key={bounty.id} bounty={bounty} onClick={() => { - window.location.href = `/bounty/${bounty.id}`; + router.push(`/bounty/${bounty.id}`); }} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/saved/saved-client.tsx` around lines 64 - 72, Replace the hard navigation using window.location.href in saved-client.tsx with Next.js SPA navigation: import and call useRouter from 'next/navigation' (const router = useRouter()) and replace the onClick handler on BountyCard to router.push(`/bounty/${bounty.id}`); alternatively wrap the BountyCard with next/link to the same path. Ensure the component is a client component (has "use client") if using useRouter from next/navigation, remove window.location.href usage, and keep the BountyCard's key and props unchanged.
29-38: Retry should refetch the query, not reload the page.
window.location.reload()re-runs the entire app shell just to recover from a transient bookmark-fetch error.useBookmarks()exposesrefetchfor exactly this case, which preserves the rest of the app state.🛠️ Proposed change
- const { data: bookmarks, isLoading, error } = useBookmarks(); + const { data: bookmarks, isLoading, error, refetch } = useBookmarks(); @@ - <Button onClick={() => window.location.reload()}>Retry</Button> + <Button onClick={() => refetch()}>Retry</Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/saved/saved-client.tsx` around lines 29 - 38, The Retry button currently calls window.location.reload() which refreshes the whole app; change its onClick to call the refetch function returned by useBookmarks instead so only the bookmarks query is retried. Locate the component using useBookmarks (the hook and its refetch symbol) in saved-client.tsx and update the Button onClick from window.location.reload() to invoke refetch (e.g., onClick={() => refetch()}), and ensure refetch is defined/returned from the useBookmarks call in the same scope.
41-43: Type predicate is a no-op; the cast still loses safety.
(b): b is BookmarkTypedoesn't narrow anything becausebis already typed asBookmarkTypeandBookmarkType.bountyis non-nullable per the schema (Bookmark.bounty: Bounty!). If the API ever can return entries withbounty: null, the underlying type needs to change; otherwise the filter +as BountyTypecast adds noise without correctness benefit. Consider simplifying to(bookmarks ?? []).map((b) => b.bounty).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/saved/saved-client.tsx` around lines 41 - 43, The filter uses a no-op type predicate and then force-casts to BountyType, losing type safety; update the bookmarkedBounties computation to either map directly to b.bounty when BookmarkType.bounty is non-nullable (replace the filter+cast with (bookmarks ?? []).map(b => b.bounty)), or if the API can actually return null bounties make the BookmarkType.bounty nullable and use a real runtime guard (e.g., filter((b): b is BookmarkType & { bounty: BountyType } => b.bounty != null).map(b => b.bounty)) so you avoid the unsafe as BountyType cast while keeping bookmarkedBounties correctly typed.components/bounty/bookmark-button.tsx (2)
80-82: Suspicious cast: widensbuttonSizeto variants it can never produce.
buttonSizeis derived fromsize: "sm" | "md" | "lg" | "icon"and can only be"default" | "sm" | "lg" | "icon". The cast adds"icon-sm" | "icon-lg", which are unreachable here, masking any real type drift on theButtonsizeprop. If the underlyingButton'ssizeprop already includes"default" | "sm" | "lg" | "icon", drop the cast entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/bounty/bookmark-button.tsx` around lines 80 - 82, The prop cast on size is widening buttonSize incorrectly; update the usage so the Button size prop receives the correct narrowed type instead of the current cast. Replace the explicit cast on buttonSize with either no cast (if Button.size already accepts "default" | "sm" | "lg" | "icon") or map/convert the component's size union ("sm" | "md" | "lg" | "icon") to the exact Button size union, ensuring symbols referenced are the local variable buttonSize and the Button component's size prop so unreachable variants "icon-sm" and "icon-lg" are not introduced.
89-91: Loading state regresses the icon affordance.While the mutation is pending, the button collapses from the bookmark icon to a literal
..., which (a) removes the visual anchor users were just looking at, and (b) breaks layout consistency with the rest of the icon-buttons in this app that useLoader2fromlucide-react. Consider keeping the bookmark glyph and reducing opacity, or swapping in<Loader2 className="animate-spin" />.🛠️ Proposed change
-import { Bookmark, BookmarkCheck } from "lucide-react"; +import { Bookmark, BookmarkCheck, Loader2 } from "lucide-react"; @@ - {isLoading ? ( - <span className="animate-pulse">...</span> - ) : isBookmarked ? ( + {isLoading ? ( + <Loader2 + className="animate-spin" + style={{ width: iconSize, height: iconSize }} + /> + ) : isBookmarked ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/bounty/bookmark-button.tsx` around lines 89 - 91, The loading state currently replaces the bookmark icon with literal "..." causing layout and affordance regressions; update the BookmarkButton component so when isLoading is true you keep the bookmark glyph visible (the same markup used for the isBookmarked/isNotBookmarked branches) and either reduce its opacity or overlay a Loader2 spinner with className="animate-spin" instead of showing "..."; locate uses of isLoading and isBookmarked inside the BookmarkButton render and replace the conditional that renders "<span className=\"animate-pulse\">...</span>" so it renders the bookmark icon + a Loader2 spinner (or the icon with reduced opacity) to preserve layout and accessibility.app/api/bookmarks/[bountyId]/route.ts (1)
33-85: Prefer the codegenToggleBookmarkDocumentover an inline duplicated mutation string.The PR already migrates client-side flows to use codegen typed documents (
lib/graphql/client.tsnow acceptsTypedDocumentString). Hard-coding the mutation string here duplicates the operation definition and skips the typed-document benefits, so any future schema field rename (e.g.,bountyWindow,_count) silently rots in this file alone.🛠️ Sketch
-import type { Bookmark } from "@/lib/graphql/generated"; +import { + ToggleBookmarkDocument, + type ToggleBookmarkMutation, + type ToggleBookmarkMutationVariables, +} from "@/lib/graphql/generated"; - const TOGGLE_BOOKMARK_MUTATION = `...inline...`; - const data = await graphqlRequest<{ toggleBookmark: Bookmark | null }>( - TOGGLE_BOOKMARK_MUTATION, - { input: { bountyId } }, - ); + const data = await graphqlRequest<ToggleBookmarkMutation>( + ToggleBookmarkDocument.toString(), + { input: { bountyId } } satisfies ToggleBookmarkMutationVariables, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/bookmarks/`[bountyId]/route.ts around lines 33 - 85, Replace the inline TOGGLE_BOOKMARK_MUTATION string with the generated TypedDocument ToggleBookmarkDocument and call graphqlRequest with that document (and the same variables) so this route uses the codegen-typed operation; specifically, remove TOGGLE_BOOKMARK_MUTATION, import ToggleBookmarkDocument, and pass ToggleBookmarkDocument to graphqlRequest (preserving the { input: { bountyId } } payload and the expected response type) so fields like bountyWindow and _count remain synced with the schema.lib/server-graphql.ts (2)
21-31: Add a request timeout to prevent hanging route handlers.
fetchhere has no timeout, so a slow or unresponsive GraphQL backend will keep the Next.js route handler (and its serverless function / connection) blocked until the platform-level timeout. Consider passingsignal: AbortSignal.timeout(10_000)(or similar) so the bookmark API fails fast and returns a 5xx promptly.🛠️ Proposed change
const response = await fetch(GRAPHQL_URL, { method: "POST", headers, body: JSON.stringify({ query, variables }), // Don't cache - always get fresh data cache: "no-store", + signal: AbortSignal.timeout(10_000), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/server-graphql.ts` around lines 21 - 31, The fetch call that posts to GRAPHQL_URL in the GraphQL request (where you pass method, headers, body, cache) has no timeout and can hang; add a timeout AbortSignal to the fetch options (e.g., signal: AbortSignal.timeout(10_000)) so the request fails fast, and handle the resulting abort/timeout error path before the existing response.ok check to surface a 5xx or appropriate error from the bookmark API.
29-38: Lossy error propagation hides backend status (e.g., 401) and trailing GraphQL errors.Two minor issues that combine to make production debugging harder:
- Line 30 throws only the HTTP status; the response body is dropped, so the upstream GraphQL message is gone by the time it reaches
app/api/bookmarks/[bountyId]/route.ts.- Line 35 throws only
json.errors[0]?.messageand never surfacesextensions.code/ status, so the route handler always responds with500, even when the underlying GraphQL error was 401/403.Consider attaching status/extensions to a custom error class so callers can map back to the right HTTP code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/server-graphql.ts` around lines 29 - 38, The current GraphQL response handling in lib/server-graphql.ts loses important error details: when response.ok is false you only throw response.status and drop the response body, and when json.errors exists you only throw json.errors[0]?.message ignoring extensions.code/status. Fix this by creating/using a custom error type (e.g., GraphQLErrorWithStatus) and populate it with the full response body and HTTP status when response.ok is false, and when json.errors exists include the first error's message plus its extensions.code (or map extensions.code to an HTTP status) and attach both the GraphQL errors array and a status field before throwing; update the throw sites in the response handling block to throw that enriched error so callers (like the route handler) can map to the correct HTTP code.hooks/use-bookmarks.ts (2)
112-119: Prefer the server result over re-reading the optimistic cache for the success toast.
onSuccessinfers the new state by re-readingbookmarkKeys.ids()— which contains the optimistic value, not the authoritative server response. If the server diverges from the optimistic guess (e.g., the bounty was already bookmarked from another device, so toggle removed it), the toast text will be wrong. The mutation already returnsBookmark | nullfrom the server; using_resultis more accurate and avoids the redundantas stringcast.Proposed fix
- onSuccess: (_result, variables) => { - // Source of truth: check current IDs cache using Set for O(1) - const currentIds = - queryClient.getQueryData<string[]>(bookmarkKeys.ids()) ?? []; - const currentIdsSet = new Set(currentIds); - const isBookmarked = currentIdsSet.has(variables as string); - toast.success(isBookmarked ? "Bounty bookmarked!" : "Bookmark removed"); - }, + onSuccess: (result) => { + toast.success(result ? "Bounty bookmarked!" : "Bookmark removed"); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bookmarks.ts` around lines 112 - 119, onSuccess currently re-reads the optimistic cache (bookmarkKeys.ids()) and casts variables to string to decide the toast; instead use the server response (_result) returned by the mutation to determine whether the bookmark exists and show the correct toast. Update the onSuccess handler in useBookmarks (the mutation's onSuccess callback) to inspect _result (which is Bookmark | null) and call toast.success with "Bounty bookmarked!" when _result is non-null or "Bookmark removed" when null, remove the Set/check against queryClient.getQueryData<string[]>(bookmarkKeys.ids()) and avoid the redundant "as string" cast.
99-105:err.messageaccess is unsafe without typing the mutationTError.
useMutationdefaultsTErrortoError, but if the underlyingpost()throws a non-Errorvalue (e.g., a string or plain object from a fetch wrapper),err.messagemay beundefinedand the toast rendersFailed to bookmark: undefined. Either type the mutation explicitly or guard the access.Proposed fix
- onError: (err, bountyId, context) => { + onError: (err: Error, _bountyId, context) => { if (context) { queryClient.setQueryData(bookmarkKeys.list(), context.previousList); queryClient.setQueryData(bookmarkKeys.ids(), context.previousIds); } - toast.error(`Failed to bookmark: ${err.message}`); + const message = err instanceof Error ? err.message : "Unknown error"; + toast.error(`Failed to bookmark: ${message}`); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bookmarks.ts` around lines 99 - 105, onError currently assumes err.message exists which is unsafe; update the mutation error handling in the onError callback (the one that calls queryClient.setQueryData, bookmarkKeys.list(), bookmarkKeys.ids(), and toast.error) to safely extract the error text instead of using err.message directly — either explicitly type the mutation error generic (TError) so err is an Error, or more robustly guard and normalize the value (e.g., compute const msg = typeof err === 'string' ? err : err && typeof (err as any).message === 'string' ? (err as any).message : String(err) ) and then call toast.error(`Failed to bookmark: ${msg}`) so the toast never shows "undefined".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/bookmarks/route.ts`:
- Around line 19-72: The route currently builds a custom BOOKMARKS_QUERY and
calls graphqlRequest<{ bookmarks: Bookmark[] }>, which diverges from the
generated bookmark operation (BookmarksDocument) that uses the canonical
fragment BountyFields and causes an unsound type assertion; replace the
hand‑rolled BOOKMARKS_QUERY and the manual generic on graphqlRequest with the
generated BookmarksDocument (or create/import a dedicated bookmark operation in
bookmark-operations.graphql), then call graphqlRequest with that generated
document and its generated types so the returned bounty shape matches
BountyFields and TypeScript types remain sound for useBookmarks and other
consumers.
In `@components/bounty/bounty-card.tsx`:
- Around line 121-125: The bookmark control inside the clickable Card is causing
click/keyboard events to bubble and trigger the Card's onClick/onKeyDown
navigation; wrap the BookmarkButton handler so it stops propagation for both
pointer and keyboard interactions: in the component containing <BookmarkButton
bountyId={bounty.id} size="sm" /> (the wrapper div around the BookmarkButton),
add event handlers that call event.stopPropagation() for onClick and for
onKeyDown (handling Space/Enter) to prevent the Card's onClick/onKeyDown from
firing when interacting with BookmarkButton.
In `@hooks/use-bookmarks.ts`:
- Around line 106-111: The onSettled handler in the mutation currently calls
Promise.all([...]) but doesn't return or await it, causing mutateAsync to
resolve before cache invalidations finish; update the onSettled in useBookmarks
(the mutation config using onSettled) to return the Promise from
Promise.all([...]) (or make the handler async and await the two
queryClient.invalidateQueries calls) so that invalidateQueries for
bookmarkKeys.list() and bookmarkKeys.ids() is awaited and any rejection is
propagated.
In `@hooks/use-notifications.ts`:
- Around line 90-106: getBookmarkedStatusMap currently returns an empty Map when
bookmarkKeys.ids() is populated but bookmarkKeys.list() is missing, which causes
first incoming bountyUpdated events to be treated as status changes; change
getBookmarkedStatusMap so that if ids exist but list is not fetched it returns
null/undefined (or another explicit sentinel) instead of an empty Map, and
update callers to check for that sentinel before generating notifications (i.e.,
only treat a missing previous status as “first observation” when the function
returns a real Map and contains the bountyId); reference getBookmarkedStatusMap,
queryClient.getQueryData, bookmarkKeys.ids() and bookmarkKeys.list() when making
this change.
- Around line 167-196: The handler currently detects a status-change and calls
addNotification but never updates the bookmark status cache, so subsequent
duplicate events see the old previousStatus and re-fire notifications; after
creating the notification (inside the isBookmarked check where you use
getBookmarkedStatusMap, addNotification and normaliseTimestamp), update the
bookmark status cache (bookmarkKeys.statusCache()) in the queryClient to record
bounty.id => bounty.status (e.g. by getting the current map/record and setting
the new status) so future events will compare against the last-seen status and
not produce duplicate "saved-bounty-updated" notifications; keep the existing
detail invalidation (bountyKeys.detail(bounty.id)) as is.
In `@lib/graphql/schema.graphql`:
- Around line 921-922: The GraphQL mutation toggleBookmark currently returns a
non-null Bookmark but the client expects null when removing a bookmark; update
the contract to allow null by changing the mutation signature to nullable
(toggleBookmark(input: ToggleBookmarkInput!): Bookmark), or alternatively
implement explicit addBookmark/removeBookmark mutations or a
ToggleBookmarkResult wrapper type (e.g., { bookmarked: Boolean!, bookmark:
Bookmark }). Make the matching change in the NestJS resolver decorators that
define the toggleBookmark return type in the upstream schema (boundless-nestjs),
regenerate the schema, and run npm run sync-schema so lib/graphql/schema.graphql
is updated to the chosen nullable/result form; ensure client-facing docs/types
(e.g., hooks/use-bookmarks) align with the new return type.
In `@package.json`:
- Line 87: The CI is failing because the lockfile is out of sync after adding
the dependency "@graphql-typed-document-node/core"; run pnpm install locally
(without --frozen-lockfile) to regenerate pnpm-lock.yaml, verify the lockfile
changes, and commit the updated pnpm-lock.yaml alongside the package.json change
so CI will pass `pnpm install --frozen-lockfile`.
---
Nitpick comments:
In `@app/api/bookmarks/`[bountyId]/route.ts:
- Around line 33-85: Replace the inline TOGGLE_BOOKMARK_MUTATION string with the
generated TypedDocument ToggleBookmarkDocument and call graphqlRequest with that
document (and the same variables) so this route uses the codegen-typed
operation; specifically, remove TOGGLE_BOOKMARK_MUTATION, import
ToggleBookmarkDocument, and pass ToggleBookmarkDocument to graphqlRequest
(preserving the { input: { bountyId } } payload and the expected response type)
so fields like bountyWindow and _count remain synced with the schema.
In `@app/api/bookmarks/ids/route.ts`:
- Around line 19-32: Replace the inline BOOKMARKS_QUERY and manual typing with
the generated document: import and use the existing BookmarksDocument (or add a
new BookmarkIds operation in bookmark-operations.graphql and use its generated
document) when calling graphqlRequest so you get codegen types instead of the
explicit generic; update the call site that currently references BOOKMARKS_QUERY
to pass the generated document (e.g., BookmarksDocument or BookmarkIdsDocument)
and then map data.bookmarks to extract bountyId as before.
In `@app/saved/page.tsx`:
- Around line 8-10: Update the redirect inside the authentication check so the
original path is preserved and the user returns to /saved after signing in: in
the if (!user) { redirect("/auth"); } block, change the redirect to include a
redirect query parameter containing the current request path (or '/saved' if you
prefer) and ensure the path is URL-encoded; use the same symbol names shown (the
if (!user) guard and redirect(...) call) so the auth page can read the query
(e.g. redirect=?redirect=...) and navigate back after successful signin.
In `@app/saved/saved-client.tsx`:
- Around line 64-72: Replace the hard navigation using window.location.href in
saved-client.tsx with Next.js SPA navigation: import and call useRouter from
'next/navigation' (const router = useRouter()) and replace the onClick handler
on BountyCard to router.push(`/bounty/${bounty.id}`); alternatively wrap the
BountyCard with next/link to the same path. Ensure the component is a client
component (has "use client") if using useRouter from next/navigation, remove
window.location.href usage, and keep the BountyCard's key and props unchanged.
- Around line 29-38: The Retry button currently calls window.location.reload()
which refreshes the whole app; change its onClick to call the refetch function
returned by useBookmarks instead so only the bookmarks query is retried. Locate
the component using useBookmarks (the hook and its refetch symbol) in
saved-client.tsx and update the Button onClick from window.location.reload() to
invoke refetch (e.g., onClick={() => refetch()}), and ensure refetch is
defined/returned from the useBookmarks call in the same scope.
- Around line 41-43: The filter uses a no-op type predicate and then force-casts
to BountyType, losing type safety; update the bookmarkedBounties computation to
either map directly to b.bounty when BookmarkType.bounty is non-nullable
(replace the filter+cast with (bookmarks ?? []).map(b => b.bounty)), or if the
API can actually return null bounties make the BookmarkType.bounty nullable and
use a real runtime guard (e.g., filter((b): b is BookmarkType & { bounty:
BountyType } => b.bounty != null).map(b => b.bounty)) so you avoid the unsafe as
BountyType cast while keeping bookmarkedBounties correctly typed.
In `@components/bounty/bookmark-button.tsx`:
- Around line 80-82: The prop cast on size is widening buttonSize incorrectly;
update the usage so the Button size prop receives the correct narrowed type
instead of the current cast. Replace the explicit cast on buttonSize with either
no cast (if Button.size already accepts "default" | "sm" | "lg" | "icon") or
map/convert the component's size union ("sm" | "md" | "lg" | "icon") to the
exact Button size union, ensuring symbols referenced are the local variable
buttonSize and the Button component's size prop so unreachable variants
"icon-sm" and "icon-lg" are not introduced.
- Around line 89-91: The loading state currently replaces the bookmark icon with
literal "..." causing layout and affordance regressions; update the
BookmarkButton component so when isLoading is true you keep the bookmark glyph
visible (the same markup used for the isBookmarked/isNotBookmarked branches) and
either reduce its opacity or overlay a Loader2 spinner with
className="animate-spin" instead of showing "..."; locate uses of isLoading and
isBookmarked inside the BookmarkButton render and replace the conditional that
renders "<span className=\"animate-pulse\">...</span>" so it renders the
bookmark icon + a Loader2 spinner (or the icon with reduced opacity) to preserve
layout and accessibility.
In `@components/ui/stellar-link.tsx`:
- Line 135: The change removing value from the dependency array of the
tooltipText computation is correct (tooltipText only depends on isValid, type,
network, explorer, tooltipPrefix), but it’s unrelated to the bookmark work in
this PR—either move this optimization into its own small PR or revert it here
and add it in a follow-up; reference the tooltipText computation and the
dependency array that now reads [type, network, explorer, isValid,
tooltipPrefix] in components/ui/stellar-link.tsx and ensure isValid continues to
reflect value changes so tooltip updates remain correct.
In `@hooks/use-bookmarks.ts`:
- Around line 112-119: onSuccess currently re-reads the optimistic cache
(bookmarkKeys.ids()) and casts variables to string to decide the toast; instead
use the server response (_result) returned by the mutation to determine whether
the bookmark exists and show the correct toast. Update the onSuccess handler in
useBookmarks (the mutation's onSuccess callback) to inspect _result (which is
Bookmark | null) and call toast.success with "Bounty bookmarked!" when _result
is non-null or "Bookmark removed" when null, remove the Set/check against
queryClient.getQueryData<string[]>(bookmarkKeys.ids()) and avoid the redundant
"as string" cast.
- Around line 99-105: onError currently assumes err.message exists which is
unsafe; update the mutation error handling in the onError callback (the one that
calls queryClient.setQueryData, bookmarkKeys.list(), bookmarkKeys.ids(), and
toast.error) to safely extract the error text instead of using err.message
directly — either explicitly type the mutation error generic (TError) so err is
an Error, or more robustly guard and normalize the value (e.g., compute const
msg = typeof err === 'string' ? err : err && typeof (err as any).message ===
'string' ? (err as any).message : String(err) ) and then call
toast.error(`Failed to bookmark: ${msg}`) so the toast never shows "undefined".
In `@lib/graphql/client.ts`:
- Around line 50-55: normalizeDocument currently converts non-string GraphQL
documents to a string via query.toString(), which strips TypedDocumentString
metadata like __meta__ and typed parameter/result info; update normalizeDocument
to return the original query object unchanged when it is a TypedDocumentString
(or otherwise carries __meta__) so downstream middleware can access its
metadata, i.e., detect the TypedDocumentString shape (presence of __meta__ or
instanceof TypedDocumentString) and return query directly instead of
query.toString(), while still falling back to toString() for other non-string
document types.
- Around line 75-83: The double-cast on requestOptions should be removed and
replaced with a narrower, explicit typing and a short comment documenting the
graphql-request overload constraint being worked around: construct the object
into a variable named requestOptions and either declare it with the exact type
RequestOptions<TVariables, TData> (or use a Partial-to-complete pattern) so the
compiler enforces shape for document/variables/requestHeaders, and only resort
to a single, justified cast if an overload prevents a clean assignment; add a
one-line comment referencing the graphql-request overload issue (e.g.,
"workaround for graphql-request RequestOptions overloads requiring different
shapes for documents with required variables") so future maintainers don't widen
the type hole, and keep graphQLClient.request<TData, TVariables>(requestOptions)
as the call site.
In `@lib/server-graphql.ts`:
- Around line 21-31: The fetch call that posts to GRAPHQL_URL in the GraphQL
request (where you pass method, headers, body, cache) has no timeout and can
hang; add a timeout AbortSignal to the fetch options (e.g., signal:
AbortSignal.timeout(10_000)) so the request fails fast, and handle the resulting
abort/timeout error path before the existing response.ok check to surface a 5xx
or appropriate error from the bookmark API.
- Around line 29-38: The current GraphQL response handling in
lib/server-graphql.ts loses important error details: when response.ok is false
you only throw response.status and drop the response body, and when json.errors
exists you only throw json.errors[0]?.message ignoring extensions.code/status.
Fix this by creating/using a custom error type (e.g., GraphQLErrorWithStatus)
and populate it with the full response body and HTTP status when response.ok is
false, and when json.errors exists include the first error's message plus its
extensions.code (or map extensions.code to an HTTP status) and attach both the
GraphQL errors array and a status field before throwing; update the throw sites
in the response handling block to throw that enriched error so callers (like the
route handler) can map to the correct HTTP code.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 434376aa-44cf-4a94-92e1-80b28c968d04
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
app/api/bookmarks/[bountyId]/route.tsapp/api/bookmarks/ids/route.tsapp/api/bookmarks/route.tsapp/saved/page.tsxapp/saved/saved-client.tsxcodegen.tscomponents/bounty-detail/bounty-detail-header-card.tsxcomponents/bounty/bookmark-button.tsxcomponents/bounty/bounty-card.tsxcomponents/ui/stellar-link.tsxhooks/use-bookmarks.tshooks/use-notifications.tslib/graphql/client.tslib/graphql/generated.tslib/graphql/operations/bookmark-operations.graphqllib/graphql/schema.graphqllib/query/query-keys.tslib/server-graphql.tspackage.json
Ensure notification deduplication cache is both written to and read from. Previously, statusCache was updated but never used, causing repeated notifications for identical bounty status updates. Now: - Read from statusCache as primary baseline - Fallback to list-derived status when absent - Skip notifications when status hasn't changed - Maintain cache consistency after emitting notification Also confirms: - GraphQL generated documents are used across bookmark routes - React Query onSettled properly awaits cache invalidation - Notification false positives avoided when cache is uninitialized
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
Hey @Shadow-MMN
Two blockers before this can merge.
The PR has a merge conflict in components/bounty/bounty-card.tsx that needs to be resolved.
CI is failing because of the JSR install issue (#187), which is also fixed on main now via #177. Pull from main to solve this.
A couple of smaller things to address while you're in there.
The bookmark button click is propagation-stopped in handleToggle via e.stopPropagation(), but only for mouse clicks. Keyboard activation (Space/Enter on the bookmark button) fires a keydown event that bubbles to the parent card, where onKeyDown handles Space/Enter and triggers navigation. CodeRabbit flagged this and the suggested fix was to add onClick and onKeyDown stop-propagation handlers on the wrapper div in bounty-card.tsx, not just inside the BookmarkButton. Same fix would help on the detail header card.
useBookmarks and useBookmarkIds both use staleTime: 5 minutes but they're independent queries fetched at different times, so the two caches go stale at slightly different points. If a user opens /saved after browsing for a while, the ids cache may invalidate before list (or vice versa), producing a brief desync between the bookmark icons and the saved page. Either align them on the same trigger or have one derive from the other rather than fetching twice.
Once the conflict is resolved and the keyboard propagation is fixed, this should be ready. Ping me when CI is green.
|
Ok on it |
|
@Benjtalkshow , can you have a look now |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hooks/use-notifications.ts (1)
183-218:⚠️ Potential issue | 🟠 Major
bookmarkKeys.statusCache()is written but never read — duplicate notifications still possible.
previousStatusis sourced exclusively fromstatusMap(derived frombookmarkKeys.list()cache), and the list cache is not updated after firing the notification — onlybountyKeys.detail(bounty.id)is invalidated. So on a duplicatebountyUpdatedevent (subscription replay on reconnect, server re-publish, etc.)statusMap.get(bounty.id)returns the same pre-change status, the inequality check is true again, and a secondsaved-bounty-updatednotification is appended with a freshDate.now()-based id thatupsertNotificationcannot dedupe.The
bookmarkKeys.statusCache()write at lines 211-217 was added for exactly this purpose but is never read. Either sourcepreviousStatusfrom it (preferred) or also patch the list cache after notifying:🛠️ Suggested fix — read from statusCache first
if (isBookmarked) { - // Get previous status from status map (derived from list cache) - const statusMap = getBookmarkedStatusMap(queryClient); - - // If no cached data, skip notification (avoid false positives) - if (statusMap === null) { - return; - } - - const previousStatus = statusMap.get(bounty.id); + // Prefer the dedicated dedupe cache; fall back to list-derived + // status only on first observation. + const statusCache = + queryClient.getQueryData<Record<string, string>>( + bookmarkKeys.statusCache(), + ) ?? {}; + const statusMap = getBookmarkedStatusMap(queryClient); + const previousStatus = + statusCache[bounty.id] ?? statusMap?.get(bounty.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-notifications.ts` around lines 183 - 218, The code reads previousStatus from statusMap (via getBookmarkedStatusMap) but never reads the dedicated cache written to by bookmarkKeys.statusCache(), so duplicate notifications can still occur; change the check to source previousStatus by first reading queryClient.getQueryData(bookmarkKeys.statusCache()) (or a helper that wraps that), falling back to statusMap/list cache only if the statusCache is null/undefined, and keep the existing queryClient.setQueryData(bookmarkKeys.statusCache(), ...) update after addNotification so the canonical statusCache is always updated; reference getBookmarkedStatusMap, previousStatus, bookmarkKeys.statusCache(), addNotification and queryClient.setQueryData when making the change.
🧹 Nitpick comments (1)
hooks/use-bookmarks.ts (1)
60-129: Toggle implementation looks solid; consider deriving the success toast from the server response.The optimistic strategy (ids authoritative, list derivative, atomic rollback, awaited invalidation) is implemented correctly and prior
onSettledawait issue is resolved.One small robustness nit:
onSuccessdecides the toast text by re-readingbookmarkKeys.ids(). If the user double-toggles quickly, a secondonMutatemay have already flipped the optimistic state by the time the first mutation'sonSuccessruns, producing a toast that contradicts what the first server call actually did. SincemutationFnalready returnsBookmark | null(non-null on add, null on remove), the result itself is the unambiguous signal:♻️ Optional: derive toast from server response
- onSuccess: (_result, variables) => { - // Source of truth: check current IDs cache using Set for O(1) - const currentIds = - queryClient.getQueryData<string[]>(bookmarkKeys.ids()) ?? []; - const currentIdsSet = new Set(currentIds); - const isBookmarked = currentIdsSet.has(variables as string); - toast.success(isBookmarked ? "Bounty bookmarked!" : "Bookmark removed"); - }, + onSuccess: (result) => { + toast.success(result ? "Bounty bookmarked!" : "Bookmark removed"); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bookmarks.ts` around lines 60 - 129, The onSuccess currently infers bookmark state by re-reading bookmarkKeys.ids(), which can be wrong if another optimistic toggle occurred; update useToggleBookmark's onSuccess to derive the toast directly from the mutation result returned by mutationFn (the Bookmark | null value) instead of querying the cache—i.e., use the result param (rename _result to result if needed) and show "Bounty bookmarked!" when result is non-null, otherwise "Bookmark removed".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/use-notifications.ts`:
- Around line 182-219: The early `return` inside the isBookmarked branch causes
the whole handler to exit and prevents the unconditional invalidation of
bountyKeys.detail(bounty.id); instead of returning when statusMap === null,
limit the skip to the notification logic: replace the `if (statusMap === null) {
return; }` pattern with a conditional that only skips the notification (e.g.,
`if (statusMap !== null) { /* existing notification + setQueryData logic using
previousStatus */ }`) so the rest of the handler still executes, then leave the
existing `queryClient.invalidateQueries({ queryKey: bountyKeys.detail(bounty.id)
})` call intact; keep references to getBookmarkedStatusMap, previousStatus,
addNotification, queryClient.setQueryData and bountyKeys.detail to locate and
update the code.
---
Duplicate comments:
In `@hooks/use-notifications.ts`:
- Around line 183-218: The code reads previousStatus from statusMap (via
getBookmarkedStatusMap) but never reads the dedicated cache written to by
bookmarkKeys.statusCache(), so duplicate notifications can still occur; change
the check to source previousStatus by first reading
queryClient.getQueryData(bookmarkKeys.statusCache()) (or a helper that wraps
that), falling back to statusMap/list cache only if the statusCache is
null/undefined, and keep the existing
queryClient.setQueryData(bookmarkKeys.statusCache(), ...) update after
addNotification so the canonical statusCache is always updated; reference
getBookmarkedStatusMap, previousStatus, bookmarkKeys.statusCache(),
addNotification and queryClient.setQueryData when making the change.
---
Nitpick comments:
In `@hooks/use-bookmarks.ts`:
- Around line 60-129: The onSuccess currently infers bookmark state by
re-reading bookmarkKeys.ids(), which can be wrong if another optimistic toggle
occurred; update useToggleBookmark's onSuccess to derive the toast directly from
the mutation result returned by mutationFn (the Bookmark | null value) instead
of querying the cache—i.e., use the result param (rename _result to result if
needed) and show "Bounty bookmarked!" when result is non-null, otherwise
"Bookmark removed".
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 33f00965-551b-4667-8326-fb71dfbe276f
📒 Files selected for processing (9)
app/api/bookmarks/[bountyId]/route.tsapp/api/bookmarks/route.tscomponents/bounty-detail/bounty-detail-header-card.tsxcomponents/bounty/bounty-card.tsxhooks/use-bookmarks.tshooks/use-notifications.tslib/graphql/schema.graphqllib/server-graphql.tspackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- components/bounty-detail/bounty-detail-header-card.tsx
- app/api/bookmarks/[bountyId]/route.ts
- lib/server-graphql.ts
- lib/graphql/schema.graphql
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
Almost there.
One thing to fix before merge: useBookmarks and useBookmarkIds both declare staleTime: 1000 * 60 * 5 independently (lines 25 and 44 of hooks/use-bookmarks.ts). Since they're fetched at different times the two caches will go stale at slightly different points, which is exactly the kind of desync the rest of this PR is built to avoid. Lift the value into a shared constant at the top of the file (e.g. const BOOKMARK_STALE_TIME = 5 * 60 * 1000;) and reference it from both hooks.
Once that's in, this is good to merge.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hooks/use-notifications.ts (1)
182-217:⚠️ Potential issue | 🟠 MajorNotification dedup is broken:
statusCacheis written but never read.The list cache (
bookmarkKeys.list()) is not invalidated by abountyUpdatedevent — onlybountyKeys.detail(bounty.id)is invalidated at line 219. SostatusMap(derived from the list cache viagetBookmarkedStatusMap) keeps returning the stale pre-change status indefinitely (until the list is refetched onstaleTimeexpiry or a bookmark toggle).Trace of a duplicate event:
- Initial list cache:
bounty.status === "OPEN".- Subscription event arrives with
status === "IN_REVIEW".previousStatus = "OPEN"≠"IN_REVIEW"→ notification fired,statusCache[id] = "IN_REVIEW"written.- Duplicate event arrives with
status === "IN_REVIEW".statusMap.get(id)still returns"OPEN"(list cache untouched), sopreviousStatus !== bounty.statusis again true → duplicate notification fired.Because the notification id includes
Date.now()(line 198),upsertNotification's key-based dedupe cannot collapse the duplicates either. ThesetQueryDatawrite at lines 208-214 is effectively dead code.Resolve
previousStatusfrombookmarkKeys.statusCache()first and fall back to the list-derived status only as a "first observation" seed:🛠️ Proposed fix
if (isBookmarked) { - // Get previous status from status map (derived from list cache) - const statusMap = getBookmarkedStatusMap(queryClient); - - // Only process notification if cached data exists (avoid false positives) - if (statusMap !== null) { - const previousStatus = statusMap.get(bounty.id); - - // Only notify if status changed (meaningful update) - if ( - previousStatus === undefined || - previousStatus !== bounty.status - ) { + // Authoritative dedupe cache; falls back to list-derived status as a + // seed for the first observation. + const statusCache = + queryClient.getQueryData<Record<string, string>>( + bookmarkKeys.statusCache(), + ) ?? {}; + const seededStatus = + getBookmarkedStatusMap(queryClient)?.get(bounty.id); + const previousStatus = statusCache[bounty.id] ?? seededStatus; + + // Only notify if we have a known previous status that differs from + // the incoming one (avoids first-event-after-bookmark false positives + // and dedupes duplicate publishes/reconnect replays). + if ( + previousStatus !== undefined && + previousStatus !== bounty.status + ) { const timestamp = Date.now(); addNotification( { id: `saved-bounty-updated-${bounty.id}-${timestamp}`, message: `Saved bounty "${bounty.title}" was updated.`, type: "saved-bounty-updated", timestamp: normaliseTimestamp(bounty.updatedAt), read: false, }, [], ); - - // Update status cache to prevent duplicate notifications - queryClient.setQueryData<Record<string, string>>( - bookmarkKeys.statusCache(), - (old = {}) => ({ - ...old, - [bounty.id]: bounty.status, - }), - ); - } } + + // Always record last-seen status (even on the first observation) so + // future events can be deduped reliably. + queryClient.setQueryData<Record<string, string>>( + bookmarkKeys.statusCache(), + (old = {}) => ({ + ...old, + [bounty.id]: bounty.status, + }), + ); }Note: gating on
previousStatus !== undefined(instead of treatingundefinedas a status change) avoids spurious "saved-bounty-updated" notifications on the very first event after a user bookmarks a bounty. If the product intent is to notify on the first event regardless, keep the=== undefinedbranch but still move the dedupe read tostatusCache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-notifications.ts` around lines 182 - 217, The current dedupe reads the list-derived status via getBookmarkedStatusMap but never reads the persisted status cache written to bookmarkKeys.statusCache(), causing duplicates; change the logic in the bounty update handler to first read previousStatus from queryClient.getQueryData(bookmarkKeys.statusCache()) (use that value as the authoritative previousStatus), only fall back to the list-derived map (getBookmarkedStatusMap) when the cache entry is missing to seed an initial value, and then only call addNotification when previousStatus !== bounty.status (or if your product wants first-observation notifications, keep the undefined branch but still seed/write to bookmarkKeys.statusCache()); ensure you still update bookmarkKeys.statusCache() via queryClient.setQueryData after notifying so future events use the cache.
🧹 Nitpick comments (1)
hooks/use-bookmarks.ts (1)
38-48: ConsiderinitialDataUpdatedAtand usingb.bountyIddirectly.Two optional improvements on the ids query:
- With
initialDataset, React Query treats it as freshly fetched relative tostaleTime. IfbookmarkKeys.list()was cached ~5 minutes ago,useBookmarkIds()will inherit that data as "just fetched" and skip a real fetch for another 5 minutes. PassinitialDataUpdatedAtto inherit the list query's timestamp (so freshness is correctly tracked), or useplaceholderDataif you don't want it persisted to the cache.- Lines 89 and 100 use
b.bountyId; line 46 usesb.bounty.id. Both reference the same value, but use the direct field for consistency.Proposed change
export function useBookmarkIds() { const queryClient = useQueryClient(); - const listData = queryClient.getQueryData<Bookmark[]>(bookmarkKeys.list()); + const listState = queryClient.getQueryState<Bookmark[]>(bookmarkKeys.list()); + const listData = listState?.data; return useQuery<string[]>({ queryKey: bookmarkKeys.ids(), queryFn: fetchBookmarkIds, staleTime: BOOKMARK_STALE_TIME, - initialData: listData?.map((b) => b.bounty.id) ?? undefined, + initialData: listData?.map((b) => b.bountyId) ?? undefined, + initialDataUpdatedAt: listState?.dataUpdatedAt, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bookmarks.ts` around lines 38 - 48, The ids query in useBookmarkIds currently sets initialData to listData?.map(b => b.bounty.id) which incorrectly marks that data as freshly fetched and also uses b.bounty.id inconsistently; update useBookmarkIds to either (A) pass initialDataUpdatedAt using the timestamp from the list query (queryClient.getQueryState(bookmarkKeys.list())?.dataUpdatedAt) so freshness is inherited while keeping initialData, or (B) switch to placeholderData instead of initialData if you don't want to persist it, and replace the mapping to use b.bountyId (listData?.map(b => b.bountyId)) for consistency; keep queryKey bookmarkKeys.ids(), queryFn fetchBookmarkIds and stale time BOOKMARK_STALE_TIME unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hooks/use-notifications.ts`:
- Around line 182-217: The current dedupe reads the list-derived status via
getBookmarkedStatusMap but never reads the persisted status cache written to
bookmarkKeys.statusCache(), causing duplicates; change the logic in the bounty
update handler to first read previousStatus from
queryClient.getQueryData(bookmarkKeys.statusCache()) (use that value as the
authoritative previousStatus), only fall back to the list-derived map
(getBookmarkedStatusMap) when the cache entry is missing to seed an initial
value, and then only call addNotification when previousStatus !== bounty.status
(or if your product wants first-observation notifications, keep the undefined
branch but still seed/write to bookmarkKeys.statusCache()); ensure you still
update bookmarkKeys.statusCache() via queryClient.setQueryData after notifying
so future events use the cache.
---
Nitpick comments:
In `@hooks/use-bookmarks.ts`:
- Around line 38-48: The ids query in useBookmarkIds currently sets initialData
to listData?.map(b => b.bounty.id) which incorrectly marks that data as freshly
fetched and also uses b.bounty.id inconsistently; update useBookmarkIds to
either (A) pass initialDataUpdatedAt using the timestamp from the list query
(queryClient.getQueryState(bookmarkKeys.list())?.dataUpdatedAt) so freshness is
inherited while keeping initialData, or (B) switch to placeholderData instead of
initialData if you don't want to persist it, and replace the mapping to use
b.bountyId (listData?.map(b => b.bountyId)) for consistency; keep queryKey
bookmarkKeys.ids(), queryFn fetchBookmarkIds and stale time BOOKMARK_STALE_TIME
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 247f0907-ac1a-49d6-9a55-1600b261a738
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
hooks/use-bookmarks.tshooks/use-notifications.ts
2 participants
Production Hardening: Saved / Bookmarked Bounties
This PR finalizes the bookmark system and brings it to production-grade reliability, performance, and consistency.
What Changed
1. Persistence Layer
2. Performance Optimization
bookmarkKeys.ids()cache for O(1) lookup.includes()and.some()scans with Set-based checks3. Cache Architecture Improvements
ids()→ source of truth for membershiplist()→ derived full bookmark objects4. Optimistic Update Stability
?? [])5. Notification System Fixes
ids()cache for membership checks6. API Improvements
/api/bookmarks/idsendpoint for lightweight state checks7. UX Improvements
8. Code Cleanup
BookmarkService(in-memory implementation)Verification Summary
Result
The bookmark system is now:
No further action required.
Closes #185
Summary by CodeRabbit
New Features
Refactor