fix: frontend state management bugs and performance#715
fix: frontend state management bugs and performance#7152witstudios wants to merge 4 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughMemoizes a sidebar item, migrates canvas/sheet views to per-page document manager with debounced saves and unmount flush, syncs edited page titles to open tabs, adds AbortController-backed permission checks with toast, and changes SWR pausing to require an initial successful load before pausing during editing. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CanvasPageView as CanvasPageView
participant DocumentManagerStore as DocumentManagerStore
participant SaveScheduler as SaveScheduler
participant API as API
User->>CanvasPageView: Edit content
CanvasPageView->>DocumentManagerStore: setContent(content) & mark isDirty
CanvasPageView->>SaveScheduler: schedule debounced save (1s)
SaveScheduler-->>CanvasPageView: timer fires (if no new edits)
CanvasPageView->>API: saveContent(latest content)
API-->>CanvasPageView: save success
CanvasPageView->>DocumentManagerStore: clear isDirty, update lastSaved
User->>CanvasPageView: Navigate away / unmount
CanvasPageView->>CanvasPageView: if isDirty -> force save before unmount
sequenceDiagram
actor User
participant EditableTitle as EditableTitle
participant API as API
participant OpenTabsStore as OpenTabsStore
participant TabsStore as TabsStore
User->>EditableTitle: Submit new title
EditableTitle->>API: PATCH /pages/:id {title}
API-->>EditableTitle: 200 OK (updated title)
EditableTitle->>OpenTabsStore: updateTabTitle(pageId, newTitle)
EditableTitle->>TabsStore: iterate tabs -> update metadata for matching path/pageId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c02eca689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx
Show resolved
Hide resolved
|
Re: Codex review on CanvasPageView cleanup (P1: flush pending edits) Addressed in commit 7c02f1d. The cleanup effect was split into two:
Additionally, the debounced save timeout now resets |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`:
- Around line 52-73: The debounce currently catches all errors so the
success-path updateDocument call clears isDirty even when save fails; remove the
empty catch here (or rethrow the error) so failures from saveContent(page.id,
newContent) propagate and prevent updating isDirty. Update the setContent
callback (inside the setTimeout) to either await saveContent without a try/catch
or, if you must catch to log, rethrow the caught error; ensure saveContent still
throws on failure so useDocumentManagerStore.getState().updateDocument(page.id,
{ isDirty: false, lastSaved: ... }) only runs when saveContent resolves
successfully.
- Around line 88-92: The mounted effect calls
useDocumentManagerStore.getState().createDocument(page.id, ...) but
createDocument is a no-op if the doc exists, causing stale content when
revisiting; fix by first reading the cached document via
useDocumentManagerStore.getState().getDocument(page.id) (or equivalent getter)
and if it exists compare its text to the fresh page.content and call an update
method (e.g., updateDocument or createDocument with overwrite) when they differ;
additionally add a cleanup in the same useEffect that removes or clears the
document on unmount (e.g.,
useDocumentManagerStore.getState().deleteDocument(page.id) or resetDocument) so
documents don't persist across remounts when out-of-band server changes occur.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/src/components/layout/left-sidebar/page-tree/PageTreeItem.tsxapps/web/src/components/layout/middle-content/content-header/EditableTitle.tsxapps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsxapps/web/src/components/layout/middle-content/page-views/document/DocumentView.tsxapps/web/src/components/layout/middle-content/page-views/sheet/SheetView.tsxapps/web/src/hooks/page-agents/usePageAgents.tsapps/web/src/hooks/useDevices.tsapps/web/src/stores/useDocumentStore.ts
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx
Show resolved
Hide resolved
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx
Outdated
Show resolved
Hide resolved
Addressing Review Feedback (d556e65)Thread 1: Codex Bot P1 — Flush pending canvas edits before clearing debounce timerThe codex bot reviewed commit 8c02eca which didn't have the force-save on unmount. Commit 7c02f1d added it, and d556e65 further strengthens it:
Thread 2: CodeRabbit Critical — Debounced save can clear isDirty even when save failsFixed. Thread 3: CodeRabbit Major — Stale content shown when revisiting canvas pageFixed. The initialize effect now checks for an existing cached document and refreshes it from the prop if the content differs and the doc isn't dirty. On unmount, CI Fix: useDevices.test.ts (2 failing tests)Updated tests to match the User Review: P1 — Keep canvas dirty state until latest save confirmedFixed via |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useDevices.test.ts (1)
75-75: GuardonSuccesswithtoBeDefinedbefore calling it via non-null assertion.Both lines call
swrConfig.onSuccess!()without first asserting the field exists. IfonSuccessis ever removed fromuseDevices.ts, the tests will throw an unguarded runtime error instead of a clean, descriptive assertion failure.🛡️ Proposed fix (shown for line 75; apply the same pattern at line 101)
+ expect(swrConfig.onSuccess).toBeDefined(); swrConfig.onSuccess!();Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/__tests__/useDevices.test.ts` at line 75, Add a guard asserting that swrConfig.onSuccess is defined before invoking it in the test: replace the direct non-null assertion call with an assertion like expect(swrConfig.onSuccess).toBeDefined(); followed by the call to swrConfig.onSuccess!(); Apply this change for both places where swrConfig.onSuccess is invoked in the useDevices.test.ts tests so the test fails with a clear assertion if the handler is removed.apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx (1)
246-251: React.memo comparator is appropriate but note the coupling with unmount cleanup.The comparator correctly limits re-renders to changes in
page.idandpage.content. However, because it allows re-renders onpage.idchange (not just remounts), the empty-deps unmount effect (lines 112-124) must use refs to access the currentpage.id— see related comment above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx` around lines 246 - 251, The memo comparator on CanvasPageView only prevents renders when page.id and page.content are unchanged, but because CanvasPageView's empty-deps unmount effect (the cleanup run on component unmount) currently closes over the original page.id, update that effect to read the current page id from a ref instead of the stale prop: add a pageIdRef (e.g., const pageIdRef = useRef(page.id)), update pageIdRef.current on each render or in a useEffect that depends on page.id, and then reference pageIdRef.current inside the cleanup callback so cleanup uses the latest page id when the component unmounts; keep the React.memo comparator as-is (export default React.memo(CanvasPageView, ...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`:
- Around line 112-124: The cleanup effect captures a stale page.id; update the
component to track the current page id in a ref (e.g., pageIdRef) and keep it in
sync whenever props.page.id changes, then use pageIdRef.current inside the
useEffect cleanup instead of the closed-over page.id; specifically, create
pageIdRef (like saveContentRef), set pageIdRef.current = page.id in a short
effect that runs on page.id changes, and in the existing useEffect cleanup
replace references to page.id with pageIdRef.current when calling
saveContentRef.current(...) and
useDocumentManagerStore.getState().clearDocument(...).
- Around line 23-32: CanvasPageView never registers editing with the global
editing store; add a useEffect inside the CanvasPageView component that calls
useEditingStore.getState().startEditing() when the component enters edit mode
(e.g., when activeTab === 'edit' or when the editor is focused) and returns a
cleanup that calls useEditingStore.getState().stopEditing() (or the appropriate
end method) to unregister on unmount or when leaving edit mode; place this
effect in CanvasPageView near the other refs/hooks (alongside
containerRef/saveTimeoutRef) so SWR revalidation is suppressed while editing.
---
Nitpick comments:
In
`@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`:
- Around line 246-251: The memo comparator on CanvasPageView only prevents
renders when page.id and page.content are unchanged, but because
CanvasPageView's empty-deps unmount effect (the cleanup run on component
unmount) currently closes over the original page.id, update that effect to read
the current page id from a ref instead of the stale prop: add a pageIdRef (e.g.,
const pageIdRef = useRef(page.id)), update pageIdRef.current on each render or
in a useEffect that depends on page.id, and then reference pageIdRef.current
inside the cleanup callback so cleanup uses the latest page id when the
component unmounts; keep the React.memo comparator as-is (export default
React.memo(CanvasPageView, ...)).
In `@apps/web/src/hooks/__tests__/useDevices.test.ts`:
- Line 75: Add a guard asserting that swrConfig.onSuccess is defined before
invoking it in the test: replace the direct non-null assertion call with an
assertion like expect(swrConfig.onSuccess).toBeDefined(); followed by the call
to swrConfig.onSuccess!(); Apply this change for both places where
swrConfig.onSuccess is invoked in the useDevices.test.ts tests so the test fails
with a clear assertion if the handler is removed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsxapps/web/src/hooks/__tests__/useDevices.test.ts
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx
Show resolved
Hide resolved
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx
Outdated
Show resolved
Hide resolved
Addressing Round 2 Review Feedback (e1e70a3)Thread 4: CodeRabbit — Missing useEditingStore registrationFixed. Added Thread 5: CodeRabbit — Stale page.id in unmount cleanupFixed. Added Note: The parent ( |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx (1)
140-173:⚠️ Potential issue | 🟡 MinorMissing
AbortController— stale fetch can overwrite freshly-initialized content on rapid remount.
fetchWithAuthon line 155 is not aborted when the component unmounts or the socket listener is torn down. The unmount cleanup callsclearDocument, so a late response arriving after unmount is a safe no-op. However, if the parent remounts the component with the samepage.idbefore the fetch resolves (the init effect will have re-created the document), the stale response will callupdateContentFromServeron the newly-created document, potentially replacing fresher content with an older snapshot.The PR already uses
AbortControllerinDocumentViewandSheetViewfor permission checks; apply the same pattern here.🛠️ Proposed fix
useEffect(() => { if (!socket) return; + let abortController: AbortController | null = null; const handleContentUpdate = async (eventData: PageEventPayload) => { if (eventData.socketId && eventData.socketId === socket.id) return; if (eventData.pageId === page.id) { console.log(`[Canvas] Received content update for page ${page.id}`); try { + abortController = new AbortController(); - const response = await fetchWithAuth(`/api/pages/${page.id}`); + const response = await fetchWithAuth(`/api/pages/${page.id}`, { signal: abortController.signal }); if (response.ok) { const updatedPage = await response.json(); const newContent = typeof updatedPage.content === 'string' ? updatedPage.content : ''; updateContentFromServer(newContent); } } catch (error) { + if (error instanceof DOMException && error.name === 'AbortError') return; console.error('Failed to fetch updated canvas content:', error); } } }; socket.on('page:content-updated', handleContentUpdate); return () => { socket.off('page:content-updated', handleContentUpdate); + abortController?.abort(); }; }, [socket, page.id, updateContentFromServer]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx` around lines 140 - 173, The socket handler handleContentUpdate must use an AbortController so stale fetches don't overwrite newly-initialized documents: inside the useEffect create a controller-per-invocation (e.g., create a new AbortController at the top of handleContentUpdate or push it into an array controllers declared in the effect scope), pass controller.signal into fetchWithAuth, and on cleanup for the effect abort all active controllers and remove the socket listener via socket.off('page:content-updated', handleContentUpdate); also make the catch ignore AbortError (or skip calling updateContentFromServer if the fetch was aborted) to avoid applying stale content; reference handleContentUpdate, fetchWithAuth, updateContentFromServer, and the socket.on/off setup when making the change.
🧹 Nitpick comments (2)
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx (2)
82-93:updateContentFromServershould also bumpsaveVersionRefto invalidate any in-flight debounced save.
clearTimeoutonly cancels a timer that hasn't fired yet. If the debounced callback is already mid-execution (i.e., awaitingsaveContent(...)) when a server update arrives,clearTimeoutis a no-op for it. That in-flight save will still complete, pass thesaveVersionRef.current === versionguard, and overwritelastSavedwith a stale timestamp — even though the authoritative content now came from the server.Bumping the version counter prevents this:
♻️ Proposed fix
const updateContentFromServer = useCallback((newContent: string) => { if (saveTimeoutRef.current) { clearTimeout(saveTimeoutRef.current); saveTimeoutRef.current = null; } + // Invalidate any in-flight debounced save so its version guard fails + saveVersionRef.current++; useDocumentManagerStore.getState().updateDocument(page.id, { content: newContent, isDirty: false, lastSaved: Date.now(), lastUpdateTime: Date.now(), }); }, [page.id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx` around lines 82 - 93, The updateContentFromServer handler currently clears the debounce timer but does not increment the in-flight save version, so an already-running save (awaiting saveContent) can still pass the saveVersionRef.current === version check and overwrite server-updated state; inside updateContentFromServer (function name) after clearing saveTimeoutRef (saveTimeoutRef.current) and before calling updateDocument, increment or bump saveVersionRef.current (e.g., ++saveVersionRef.current or assign a new token) so any in-flight save will fail its version guard (saveVersionRef and the saveContent version check) and be prevented from writing stale lastSaved/lastUpdateTime.
259-264: Custommemocomparator silently suppresses re-renders for any newTreePagefields added in the future.The comparator is correct for the current component surface (
page.idandpage.contentare the onlypagefields consumed). Consider adding a brief comment so future contributors know why only these two fields are compared, to avoid accidentally introducing silent stale-prop bugs if newpagefields (e.g.,page.title,page.permissions) are later referenced in the component.♻️ Suggested comment
export default React.memo( CanvasPageView, + // Only page.id and page.content drive this component's rendering and effects. + // If you add consumption of other page fields, extend this comparator accordingly. (prevProps, nextProps) => prevProps.page.id === nextProps.page.id && prevProps.page.content === nextProps.page.content );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx` around lines 259 - 264, The custom React.memo comparator on CanvasPageView currently only compares prevProps.page.id and prevProps.page.content which will silently prevent re-renders if new TreePage fields are later used; update the memo call by adding a concise comment above the comparator explaining that the comparator intentionally limits checks to page.id and page.content (and that any time a new page field like page.title or page.permissions is consumed by CanvasPageView the comparator must be updated to include it or be removed), and ensure the comment references CanvasPageView, prevProps/nextProps and the page.id/page.content checks so future contributors understand why only those fields are compared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`:
- Around line 140-173: The socket handler handleContentUpdate must use an
AbortController so stale fetches don't overwrite newly-initialized documents:
inside the useEffect create a controller-per-invocation (e.g., create a new
AbortController at the top of handleContentUpdate or push it into an array
controllers declared in the effect scope), pass controller.signal into
fetchWithAuth, and on cleanup for the effect abort all active controllers and
remove the socket listener via socket.off('page:content-updated',
handleContentUpdate); also make the catch ignore AbortError (or skip calling
updateContentFromServer if the fetch was aborted) to avoid applying stale
content; reference handleContentUpdate, fetchWithAuth, updateContentFromServer,
and the socket.on/off setup when making the change.
---
Nitpick comments:
In
`@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`:
- Around line 82-93: The updateContentFromServer handler currently clears the
debounce timer but does not increment the in-flight save version, so an
already-running save (awaiting saveContent) can still pass the
saveVersionRef.current === version check and overwrite server-updated state;
inside updateContentFromServer (function name) after clearing saveTimeoutRef
(saveTimeoutRef.current) and before calling updateDocument, increment or bump
saveVersionRef.current (e.g., ++saveVersionRef.current or assign a new token) so
any in-flight save will fail its version guard (saveVersionRef and the
saveContent version check) and be prevented from writing stale
lastSaved/lastUpdateTime.
- Around line 259-264: The custom React.memo comparator on CanvasPageView
currently only compares prevProps.page.id and prevProps.page.content which will
silently prevent re-renders if new TreePage fields are later used; update the
memo call by adding a concise comment above the comparator explaining that the
comparator intentionally limits checks to page.id and page.content (and that any
time a new page field like page.title or page.permissions is consumed by
CanvasPageView the comparator must be updated to include it or be removed), and
ensure the comment references CanvasPageView, prevProps/nextProps and the
page.id/page.content checks so future contributors understand why only those
fields are compared.
- Migrate CanvasPageView from useDocumentStore to useDocumentManagerStore to fix shared saveTimeoutId data loss bug with parallel documents - Add hasLoadedRef guard to useDevices and usePageAgents SWR isPaused to prevent blocking initial data fetch when editing is active - Move SheetView socket listener deps to refs to stop re-subscribing on every keystroke (documentState.content was in useEffect deps) - Wrap PageTreeItem with React.memo to prevent re-rendering every tree node on any parent state change - Update EditableTitle to sync renamed page title to both tab stores (useOpenTabsStore and useTabsStore) - Add AbortController to permission check fetches in DocumentView and SheetView to prevent stale responses overwriting isReadOnly state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address two issues from code review: - isDirty flag now reset to false after debounced save succeeds - Dirty content is force-saved on component unmount, matching the pattern used in DocumentView and SheetView Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rded isDirty, stale content - saveContent now rethrows errors so debounced save won't clear isDirty on failure - Add saveVersionRef counter so only the latest save can mark document clean, preventing a race where an older in-flight save clears isDirty while newer edits are unsaved - Initialize effect now refreshes cached doc from props when content differs and doc isn't dirty, fixing stale content on page revisit - Unmount cleanup now calls clearDocument after force-save to prevent stale cache across remounts - Update useDevices tests to match hasLoadedRef behavior: isPaused only blocks revalidation after initial fetch completes via onSuccess Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…leanup - Register editing state with useEditingStore when content is dirty, preventing SWR revalidation from disrupting canvas edits (per CLAUDE.md UI Refresh Protection guidelines) - Add pageIdRef to avoid stale page.id capture in empty-deps unmount cleanup, making the pattern resilient even without key-based remounting - Clean up useEditingStore session on unmount alongside document cache - Remove unused eslint-disable directive Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2witstudios
force-pushed
the
ppg/frontend-state-fixes
branch
from
e1e70a3 to
e6147c6
Compare
1 participant
Summary
useDocumentStore(sharedsaveTimeoutId) touseDocumentManagerStore(per-documentMap<string, DocumentState>)hasLoadedRefguard touseDevicesandusePageAgentssoisPausednever blocks the first fetchdocumentState.content/isDirtyreads into refs so the socketuseEffectdoesn't re-subscribe on every keystrokeEditableTitlenow syncs renamed page title to bothuseOpenTabsStoreanduseTabsStoreAbortControllerto permission checks inDocumentViewandSheetViewso stale responses don't overwriteisReadOnlyisDirtyafter successful debounced save; force-save dirty content on unmountsaveContentnow rethrows so failed saves leaveisDirty=trueTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests