Fixed the - Transition Global State from globalThis to TanStack Query - - #189
Conversation
|
@BYTES-TECHES is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request migrates the application's state management from imperative Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
|
@BYTES-TECHES 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! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/store.ts (1)
47-192: Significant duplication across the five entity sections; consider extracting generic helpers.Applications, Submissions, MilestoneParticipations, and CompetitionParticipations all share the same shape (
{ id; bountyId; … }) and the same read/add/update implementations. A pair of generic helpers would remove ~120 lines of near-identical code and make race-condition / null-handling fixes apply uniformly.♻️ Sketch of a generic helper
type WithId = { id: string; bountyId?: string }; function useLocalList<T extends WithId>( key: readonly unknown[], initial: T[] = [], ) { return (bountyId?: string) => useQuery({ queryKey: key, queryFn: () => initial, initialData: initial, staleTime: Infinity, select: (xs: T[]) => bountyId ? xs.filter((x) => x.bountyId === bountyId) : xs, }); } function useAddLocal<T extends WithId>(key: readonly unknown[]) { const qc = useQueryClient(); return useMutation({ mutationFn: async (item: T) => { qc.setQueryData<T[]>(key, (old = []) => [...old, item]); }, }); } function useUpdateLocal<T extends WithId>(key: readonly unknown[]) { const qc = useQueryClient(); return useMutation({ mutationFn: async ({ id, updates }: { id: string; updates: Partial<T> }) => { let found = false; qc.setQueryData<T[]>(key, (old = []) => { const i = old.findIndex((x) => x.id === id); if (i === -1) return old; found = true; const next = [...old]; next[i] = { ...next[i], ...updates }; return next; }); if (!found) throw new Error(`${String(key)} ${id} not found`); }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/store.ts` around lines 47 - 192, The four repeated sections (useLocalApplications/useAddLocalApplication/useUpdateLocalApplication, useLocalSubmissions/useAddLocalSubmission/useUpdateLocalSubmission, useLocalMilestoneParticipations/useAddLocalMilestoneParticipation/useUpdateLocalMilestoneParticipation, useLocalCompetitionParticipations/useAddLocalCompetitionParticipation) duplicate read/add/update logic and risk inconsistent fixes; extract generic helpers like useLocalList<T extends WithId>(key, initial) for queries and useAddLocal<T>(key)/useUpdateLocal<T>(key) for mutations (WithId = { id: string; bountyId?: string }), and refactor each entity hook to call these helpers; implement mutations using queryClient.setQueryData(updater) to avoid race conditions and to atomically add/update items and return/throw on not-found where appropriate.hooks/use-submission-draft.ts (1)
48-57: Use the stable.mutatefunction reference inuseCallbackdependencies, not the mutation object.In TanStack Query v5,
useMutationreturns a new object instance on every render (due to volatile state likeisPending,data, etc.). Using[saveMutation]or[clearMutation]in the dependency array causes the callbacks to lose referential stability on every render, defeating the purpose ofuseCallback. The.mutatefunction itself remains stable across renders.♻️ Proposed fix
const saveDraft = useCallback( (formData: SubmissionForm) => { saveMutation.mutate(formData); }, - [saveMutation], + [saveMutation.mutate], ); const clearDraft = useCallback(() => { clearMutation.mutate(); - }, [clearMutation]); + }, [clearMutation.mutate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-submission-draft.ts` around lines 48 - 57, The callbacks saveDraft and clearDraft use the entire mutation objects (saveMutation, clearMutation) in their useCallback dependency arrays, causing unstable references; replace those dependencies with the stable mutate function references (saveMutation.mutate and clearMutation.mutate) so the useCallback uses [saveMutation.mutate] and [clearMutation.mutate] respectively, ensuring referential stability while keeping the implementation of saveDraft (calling saveMutation.mutate(formData)) and clearDraft (calling clearMutation.mutate()) unchanged.
🤖 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-bounty.ts`:
- Around line 19-29: Remove the redundant cache bridge: delete the
useQueryClient()/getQueryData(bountyKeys.detail(id)) + passing initialData to
useBountyQuery and call useBountyQuery({ id }, options) directly so React Query
de-duplicates on the shared queryKey (useBountyQuery.getKey({ id })) and
surfaces cached data with correct staleness. If you truly must seed initialData,
also supply initialDataUpdatedAt using
queryClient.getQueryState(bountyKeys.detail(id))?.dataUpdatedAt so the original
fetch timestamp is preserved for staleness checks.
In `@hooks/use-submission-draft.ts`:
- Around line 13-20: The current useQuery queryFn (in
hooks/use-submission-draft.ts) directly reads localStorage and JSON.parse which
can throw in non-browser environments or on corrupted data; update the queryFn
used by useQuery to first guard with typeof window !== "undefined" before
accessing localStorage, and wrap the retrieval/JSON.parse of draftKey in a
try/catch that returns null on any error so the hook never throws (preserve the
existing default draft = null behavior).
In `@lib/store.ts`:
- Around line 33-43: The update mutation functions (e.g., useUpdateLocalBounty,
useUpdateLocalApplication, useUpdateLocalSubmission,
useUpdateLocalMilestoneParticipation) currently return null when the target id
isn't found which makes callers unable to detect a missing item; change each
mutationFn so that when findIndex returns -1 it throws a descriptive Error
(e.g., `new Error("not_found: bounty <id>")`) instead of returning null, so the
mutation rejects and onError handlers run (also update any TypeScript return
types/signatures if necessary to reflect that the mutation will throw on
not-found).
- Around line 30-45: The current useUpdateLocalBounty (and related hooks like
useAddLocalApplication, useUpdateLocalApplication, useAddLocalSubmission,
useUpdateLocalSubmission, useAddLocalMilestoneParticipation,
useUpdateLocalMilestoneParticipation, useAddLocalCompetitionParticipation)
performs a racy read-modify-write via queryClient.getQueryData then
setQueryData; change each hook to use the functional updater form
queryClient.setQueryData(key, (old) => { ... }) so the read and write are atomic
inside the cache (drop the standalone getQueryData), e.g., compute the updated
array inside the updater (fall back to mock arrays when old is undefined), and
collapse mutationFn/onSuccess so the mutation applies the change directly using
setQueryData and returns the new value.
---
Nitpick comments:
In `@hooks/use-submission-draft.ts`:
- Around line 48-57: The callbacks saveDraft and clearDraft use the entire
mutation objects (saveMutation, clearMutation) in their useCallback dependency
arrays, causing unstable references; replace those dependencies with the stable
mutate function references (saveMutation.mutate and clearMutation.mutate) so the
useCallback uses [saveMutation.mutate] and [clearMutation.mutate] respectively,
ensuring referential stability while keeping the implementation of saveDraft
(calling saveMutation.mutate(formData)) and clearDraft (calling
clearMutation.mutate()) unchanged.
In `@lib/store.ts`:
- Around line 47-192: The four repeated sections
(useLocalApplications/useAddLocalApplication/useUpdateLocalApplication,
useLocalSubmissions/useAddLocalSubmission/useUpdateLocalSubmission,
useLocalMilestoneParticipations/useAddLocalMilestoneParticipation/useUpdateLocalMilestoneParticipation,
useLocalCompetitionParticipations/useAddLocalCompetitionParticipation) duplicate
read/add/update logic and risk inconsistent fixes; extract generic helpers like
useLocalList<T extends WithId>(key, initial) for queries and
useAddLocal<T>(key)/useUpdateLocal<T>(key) for mutations (WithId = { id: string;
bountyId?: string }), and refactor each entity hook to call these helpers;
implement mutations using queryClient.setQueryData(updater) to avoid race
conditions and to atomically add/update items and return/throw on not-found
where appropriate.
🪄 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: 22cef1af-c845-4342-bf89-5a817a2c4ddc
📒 Files selected for processing (4)
hooks/use-bounty.tshooks/use-submission-draft.tslib/store.tsproviders/query-client-provider.tsx
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
Thanks for picking this up. A few items before merge:
Dead code. providers/query-client-provider.tsx is never imported. The app still uses QueryProvider from providers/query-provider.tsx. Remove the new file or wire it in via app/layout.tsx.
Broken test. lib/store.test.ts still imports BountyStore, which this PR deletes. pnpm tsc --noEmit fails. Update or delete the test.
Misleading JSDoc. The @deprecated block in lib/store.ts references BountyStoreData, which no longer exists in the file.
Unguarded JSON.parse in hooks/use-submission-draft.ts will throw on corrupted localStorage. Wrap in try/catch.
Please address all CodeRabbit findings, especially the useBounty redundancy.
|
Okay will do the fixes right away |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
hooks/use-submission-draft.ts (1)
67-72: Consider exposing mutation state for loading/error feedback.The
saveDraftwrapper fires mutation but doesn't exposesaveMutation.isPendingorsaveMutation.isError. Callers cannot show save-in-progress indicators or handle errors. If UI feedback is needed, consider returning mutation state.♻️ Optional enhancement to expose mutation state
return { draft, saveDraft, clearDraft, autoSave, + isSaving: saveMutation.isPending, + isClearing: clearMutation.isPending, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-submission-draft.ts` around lines 67 - 72, The saveDraft wrapper currently calls saveMutation.mutate(formData) but doesn't expose mutation state, preventing callers from showing loading/error UI; modify the hook that defines saveDraft (in hooks/use-submission-draft.ts) to return saveMutation's state (at least isLoading/isPending, isError, error, and maybe reset) alongside the saveDraft function so consumers can render progress and handle errors; specifically, update the hook's return value to include saveMutation (or its selected fields) and update any call sites to read saveMutation.isLoading/isError or the provided fields.hooks/__tests__/use-submission-draft.test.ts (2)
112-113: Redundant assertion.Line 113 duplicates the assertion inside
waitForon line 112.♻️ Proposed fix
await waitFor(() => expect(result2.current.draft?.formData).toEqual(formData)); - expect(result2.current.draft?.formData).toEqual(formData); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/__tests__/use-submission-draft.test.ts` around lines 112 - 113, The test contains a redundant assertion: the direct expect(result2.current.draft?.formData).toEqual(formData) duplicates the assertion already wrapped inside waitFor(() => expect(result2.current.draft?.formData).toEqual(formData)); remove the second, redundant expect on result2.current.draft?.formData (the standalone line) so the test only asserts via waitFor; this targets the assertion involving result2.current.draft?.formData in the use-submission-draft.test.ts file.
89-91: Redundant assertion.Line 91 duplicates the assertion on line 89. Consider removing the redundant check.
♻️ Proposed fix
await waitFor(() => expect(result.current.draft?.formData).toEqual(formData)); - expect(result.current.draft?.formData).toEqual(formData); jest.useRealTimers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/__tests__/use-submission-draft.test.ts` around lines 89 - 91, Remove the duplicate assertion that repeats the waitFor check: keep the awaited assertion using waitFor(() => expect(result.current.draft?.formData).toEqual(formData)) and delete the subsequent direct expect(result.current.draft?.formData).toEqual(formData); ensure jest.useRealTimers() remains after the single assertion so timers are restored.lib/store.ts (2)
72-84: Consider addingonErrorrollback for add mutations.The add mutations optimistically update the cache but lack
onErrorhandlers to rollback on failure. While mutations here are local-only (no network), if future refactoring adds server calls, the lack of rollback could lead to UI/cache inconsistency. Thehooks/use-bookmarks.tspattern (lines 1-50) demonstrates proper optimistic update with snapshot and rollback.This is a low-priority suggestion for local-only mutations but worth noting for future resilience.
Also applies to: 125-137, 180-192, 244-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/store.ts` around lines 72 - 84, The add-local mutation useAddLocalApplication (and the similar add hooks at the other ranges) performs an optimistic cache update but lacks onError rollback; implement the same pattern used in hooks/use-bookmarks.ts by adding an onMutate that snapshots the current applications via queryClient.getQueryData(localStoreKeys.applications), performing the optimistic setQueryData, then return the snapshot in the context; add an onError that receives that context and restores the snapshot with queryClient.setQueryData(localStoreKeys.applications, snapshot) (and optionally clear or refetch in onSettled) so the cache is rolled back if the mutation fails.
244-256: Add missinguseUpdateLocalCompetitionParticipationhook for consistency.The pattern established across other entity types (Bounty, Application, Submission, MilestoneParticipation) includes both add and update mutations. CompetitionParticipation currently has only the add hook. While the hooks are not yet used in the codebase, implementing the update mutation maintains consistency and prepares for future requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/store.ts` around lines 244 - 256, Add a new useUpdateLocalCompetitionParticipation hook mirroring the pattern used for other entities: create export function useUpdateLocalCompetitionParticipation() that calls useMutation with a mutationFn accepting an updated CompetitionParticipation, reads the current array from queryClient.getQueryData/localStoreKeys.competitionParticipations (or uses setQueryData with updater), maps over the existing CompetitionParticipation[] to replace the item with matching id (or unique key) with the updated participation, and returns the updated participation; ensure it uses the same queryClient instance and key as useAddLocalCompetitionParticipation and updates the cache immutably via queryClient.setQueryData.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hooks/__tests__/use-submission-draft.test.ts`:
- Around line 112-113: The test contains a redundant assertion: the direct
expect(result2.current.draft?.formData).toEqual(formData) duplicates the
assertion already wrapped inside waitFor(() =>
expect(result2.current.draft?.formData).toEqual(formData)); remove the second,
redundant expect on result2.current.draft?.formData (the standalone line) so the
test only asserts via waitFor; this targets the assertion involving
result2.current.draft?.formData in the use-submission-draft.test.ts file.
- Around line 89-91: Remove the duplicate assertion that repeats the waitFor
check: keep the awaited assertion using waitFor(() =>
expect(result.current.draft?.formData).toEqual(formData)) and delete the
subsequent direct expect(result.current.draft?.formData).toEqual(formData);
ensure jest.useRealTimers() remains after the single assertion so timers are
restored.
In `@hooks/use-submission-draft.ts`:
- Around line 67-72: The saveDraft wrapper currently calls
saveMutation.mutate(formData) but doesn't expose mutation state, preventing
callers from showing loading/error UI; modify the hook that defines saveDraft
(in hooks/use-submission-draft.ts) to return saveMutation's state (at least
isLoading/isPending, isError, error, and maybe reset) alongside the saveDraft
function so consumers can render progress and handle errors; specifically,
update the hook's return value to include saveMutation (or its selected fields)
and update any call sites to read saveMutation.isLoading/isError or the provided
fields.
In `@lib/store.ts`:
- Around line 72-84: The add-local mutation useAddLocalApplication (and the
similar add hooks at the other ranges) performs an optimistic cache update but
lacks onError rollback; implement the same pattern used in
hooks/use-bookmarks.ts by adding an onMutate that snapshots the current
applications via queryClient.getQueryData(localStoreKeys.applications),
performing the optimistic setQueryData, then return the snapshot in the context;
add an onError that receives that context and restores the snapshot with
queryClient.setQueryData(localStoreKeys.applications, snapshot) (and optionally
clear or refetch in onSettled) so the cache is rolled back if the mutation
fails.
- Around line 244-256: Add a new useUpdateLocalCompetitionParticipation hook
mirroring the pattern used for other entities: create export function
useUpdateLocalCompetitionParticipation() that calls useMutation with a
mutationFn accepting an updated CompetitionParticipation, reads the current
array from queryClient.getQueryData/localStoreKeys.competitionParticipations (or
uses setQueryData with updater), maps over the existing
CompetitionParticipation[] to replace the item with matching id (or unique key)
with the updated participation, and returns the updated participation; ensure it
uses the same queryClient instance and key as
useAddLocalCompetitionParticipation and updates the cache immutably via
queryClient.setQueryData.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef038b0a-37a4-4ab9-b979-af3d2ea4f576
📒 Files selected for processing (4)
hooks/__tests__/use-submission-draft.test.tshooks/use-submission-draft.tslib/store.test.tslib/store.ts
💤 Files with no reviewable changes (1)
- lib/store.test.ts
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
Big improvement:
One new issue from the latest commit: hooks/__tests__/use-submission-draft.test.ts uses JSX (the <QueryClientProvider> wrapper on line 16), but the file extension is .ts not .tsx. pnpm tsc --noEmit produces 4 syntax errors at line 16, and CI build-and-lint will likely fail. Please rename the file to use-submission-draft.test.tsx.
Once that's fixed, this is ready to merge.
|
Done the fixes |
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
Force-push rebased onto main, but the file extension fix from the previous review wasn't applied. hooks/__tests__/use-submission-draft.test.ts still uses JSX (<QueryClientProvider> on line 16), and pnpm tsc --noEmit produces the same 4 syntax errors.
Please rename the file to use-submission-draft.test.tsx. That's the only thing standing between this PR and merge.
Do not force push to this branch. Make sure the CI lint check and test passes |
|
I've done the corrections. Kindly merge the PR. |
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
LGTM!
File rename is in. Typecheck and lint pass locally, CI is green. Ready to merge.
|
Thank you! I enjoyed working with you! |
2 participants
Closes #125
Fixed Issue #125
Summary by CodeRabbit
Refactor
Tests