2witstudios · 2witstudios · Feb 26, 2026
diff --git a/.ppg/results/ag-0bqvq2sk.md b/.ppg/results/ag-0bqvq2sk.md
@@ -0,0 +1,337 @@
+# Result: ag-0bqvq2sk
+
+## PR
+https://github.com/2witstudios/PageSpace/pull/715
+
+## Summary
+Comprehensive code review of PR #715 "fix: frontend state management bugs and performance". Review-only — no code changes made.
+
+## Changes
+Review covered all 9 changed files:
+- `apps/web/src/components/layout/left-sidebar/page-tree/PageTreeItem.tsx`
+- `apps/web/src/components/layout/middle-content/content-header/EditableTitle.tsx`
+- `apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`
+- `apps/web/src/components/layout/middle-content/page-views/document/DocumentView.tsx`
+- `apps/web/src/components/layout/middle-content/page-views/sheet/SheetView.tsx`
+- `apps/web/src/hooks/__tests__/useDevices.test.ts`
+- `apps/web/src/hooks/page-agents/usePageAgents.ts`
+- `apps/web/src/hooks/useDevices.ts`
+- `apps/web/src/stores/useDocumentStore.ts`
+
+## Notes
+Full review follows below.
+
+---
+
+# Code Review: PR #715 — fix: frontend state management bugs and performance
+
+## Epic Scope Analysis
+
+**PR Summary**: 9 files changed, +168/-46 lines across 4 commits (including 3 follow-up review-response commits).
+
+**Functional Requirements Addressed:**
+1. Fix data loss bug in CanvasPageView (shared `saveTimeoutId`)
+2. Fix SWR initial fetch blocked (`hasLoadedRef` guard)
+3. Fix SheetView socket churn (refs for content/isDirty)
+4. Add React.memo to PageTreeItem (performance)
+5. Fix stale tab titles (EditableTitle sync)
+6. Fix permission fetch race (AbortController)
+7. Fix CanvasPageView save lifecycle (isDirty reset, force-save on unmount)
+8. Fix save error propagation (rethrow)
+9. Fix isDirty race condition (version counter)
+10. Fix stale content on revisit (initialize effect refresh, cache clear)
+
+**All 10 requirements appear addressed in the final diff.**
+
+---
+
+## 1. Code Structure & Organization
+
+### Strengths
+- **Clean cohort separation**: Each fix is logically isolated — permission AbortControllers, SWR hasLoadedRef, socket ref optimization, and document store migration are independent concerns that don't leak into each other.
+- **Consistent pattern application**: The `hasLoadedRef` guard is applied identically in both `useDevices` and `usePageAgents`, following the documented UI Refresh Protection pattern from CLAUDE.md.
+- **Deprecation discipline**: The old `useDocumentStore` gets a proper `@deprecated` JSDoc comment explaining why and what to use instead. No dead code removed prematurely.
+
+### Observations
+- **CanvasPageView grew significantly** (+80 lines net). The component now internalizes debounced save logic, version tracking, editing store registration, and unmount cleanup that was previously delegated to `useDocumentStore`. This is justifiable because the shared-timer bug demanded per-document ownership, but it's approaching extraction-worthy complexity.
+
+**Score: 8/10** — Good organization, one component becoming dense.
+
+---
+
+## 2. TypeScript & Code Quality
+
+### Strengths
+- **No `any` types** introduced anywhere. All new code is properly typed.
+- **Functional patterns**: `useCallback` used correctly for stable references; refs used appropriately for values that shouldn't trigger re-renders.
+- **`const` over `let`**: All new declarations use `const`. The `saveVersionRef` mutation via `++saveVersionRef.current` is the idiomatic React ref pattern.
+
+### Findings
+
+**[Minor] Empty catch block in setContent debounce:**
+```typescript
+} catch {
+  // saveContent already logged and toasted - isDirty stays true for retry/unmount-save
+}
+```
+The empty catch with a comment is acceptable here since `saveContent` handles error UX (toast + console.error) and rethrows specifically so `isDirty` persists. The comment explains the intent. Acceptable.
+
+**[Minor] `useDocumentStore` import still present in CanvasPageView (current branch):**
+The diff shows the migration to `useDocumentManagerStore`, but the current file in the base branch still imports from `useDocumentStore`. Verified this is just a worktree/branch issue — the PR branch has the correct import.
+
+**Score: 9/10** — Clean TypeScript, no type compromises.
+
+---
+
+## 3. React Patterns & Performance
+
+### Strengths
+
+**React.memo on PageTreeItem:**
+```typescript
+export const PageTreeItem = React.memo(function PageTreeItem({...}) { ... });
+```
+Good call. This is rendered once per tree node, and the named function preserves devtools debuggability. The component receives structured props that work well with shallow comparison.
+
+**AbortController in permission effects:**
+```typescript
+useEffect(() => {
+  const abortController = new AbortController();
+  // ...
+  return () => { abortController.abort(); };
+}, [user?.id, pageId]);
+```
+Textbook cleanup pattern. Applied consistently in both DocumentView and SheetView.
+
+**Refs for socket handler stability (SheetView):**
+```typescript
+const isDirtyRef = useRef(false);
+const contentRef = useRef(documentState?.content ?? '');
+useEffect(() => {
+  isDirtyRef.current = documentState?.isDirty || false;
+  contentRef.current = documentState?.content ?? '';
+}, [documentState?.isDirty, documentState?.content]);
+```
+This eliminates the socket effect's dependency on `documentState?.content` and `documentState?.isDirty`, preventing re-subscription on every keystroke. Correct approach.
+
+**Version-guarded isDirty clearing:**
+```typescript
+const version = ++saveVersionRef.current;
+// ... after save ...
+if (saveVersionRef.current === version) {
+  useDocumentManagerStore.getState().updateDocument(page.id, { isDirty: false, ... });
+}
+```
+Prevents race condition where a fast edit during an in-flight save would have its dirty flag incorrectly cleared. Good pattern.
+
+### Findings
+
+**[Info] Empty-deps useEffect with refs for unmount cleanup:**
+```typescript
+useEffect(() => {
+  return () => {
+    if (saveTimeoutRef.current) clearTimeout(saveTimeoutRef.current);
+    const id = pageIdRef.current;
+    const store = useDocumentManagerStore.getState();
+    const doc = store.getDocument(id);
+    if (doc?.isDirty) {
+      saveContentRef.current(id, doc.content).catch(console.error);
+    }
+    store.clearDocument(id);
+    useEditingStore.getState().endEditing(id);
+  };
+}, []);
+```
+The empty dependency array with refs is the correct pattern for true-unmount-only cleanup. The `pageIdRef` sync effect ensures the ref always holds the current page ID. The comment "parent renders with key={page.id}" documents the assumption. Sound.
+
+**[Note] Force-save on unmount is fire-and-forget:**
+The `saveContentRef.current(id, doc.content).catch(console.error)` call during unmount is async but the component is unmounting. The `await` is not possible in a cleanup function. The `catch(console.error)` prevents unhandled rejection. This is the best you can do in React — acceptable.
+
+**[Note] EditableTitle tab sync uses imperative store access:**
+```typescript
+useOpenTabsStore.getState().updateTabTitle(updatedPage.id, updatedPage.title);
+const tabsState = useTabsStore.getState();
+for (const tab of tabsState.tabs) {
+  if (tab.path.endsWith(`/${updatedPage.id}`)) {
+    tabsState.updateTabMeta(tab.id, { title: updatedPage.title });
+  }
+}
+```
+Using `getState()` inside a callback is the correct Zustand pattern for imperative updates. The `endsWith` path matching is a reasonable heuristic for tab-to-page mapping.
+
+**Score: 9/10** — Strong React patterns throughout.
+
+---
+
+## 4. Test Coverage & Quality
+
+### Strengths
+- **Tests updated to match new semantics**: The `useDevices.test.ts` file properly tests the new `hasLoadedRef` behavior with three clear scenarios:
+  1. Initial load not completed + editing = allow revalidation (was blocking before)
+  2. Initial load completed + editing = pause revalidation
+  3. Initial load completed + not editing = allow revalidation
+- **`onSuccess` simulation**: Tests correctly simulate the SWR `onSuccess` callback to flip the `hasLoadedRef`.
+
+### Findings
+
+**[Concern] No new tests for CanvasPageView:**
+The CanvasPageView underwent the most significant refactor (store migration, debounce, version guard, unmount cleanup, editing store registration), but no tests were added. Given that this is a bug fix PR that addresses data loss, test coverage for the save lifecycle would be valuable. However, testing React component effects with store interactions is complex and may warrant a follow-up.
+
+**[Concern] No tests for usePageAgents hasLoadedRef:**
+The same pattern applied to `useDevices` (with tests) was also applied to `usePageAgents` (without tests). The pattern is identical, so the risk is low, but parity would be ideal.
+
+**[Minor] Test descriptions could be more requirement-focused:**
+```typescript
+it('given initial load has not completed, should allow revalidation even when editing', () => {
+```
+This follows the given/should pattern well. The descriptions clearly state the behavioral requirement.
+
+**Score: 7/10** — Existing tests updated correctly; gaps in new coverage for the most critical change.
+
+---
+
+## 5. Security Analysis (OWASP Top 10)
+
+Reviewing all changes against OWASP Top 10 (2021):
+
+| # | Category | Status | Notes |
+|---|----------|--------|-------|
+| A01 | Broken Access Control | **Pass** | Permission checks enhanced with AbortController; no permissions bypassed |
+| A02 | Cryptographic Failures | **N/A** | No crypto changes |
+| A03 | Injection | **Pass** | No user input concatenated into queries/commands; API calls use parameterized `patch()` |
+| A04 | Insecure Design | **Pass** | Version-guarded saves prevent race conditions; force-save on unmount prevents data loss |
+| A05 | Security Misconfiguration | **N/A** | No configuration changes |
+| A06 | Vulnerable Components | **N/A** | No dependency changes |
+| A07 | Auth Failures | **Pass** | Permission check race condition fixed (stale responses no longer overwrite `isReadOnly`) |
+| A08 | Software/Data Integrity | **Pass** | Save version counter prevents stale writes from clearing dirty state |
+| A09 | Logging/Monitoring | **Pass** | Debug `console.log` statements removed from save flow (good cleanup) |
+| A10 | SSRF | **N/A** | No server-side request changes |
+
+**No XSS risk**: The diff doesn't introduce any `dangerouslySetInnerHTML`, `innerHTML`, or unescaped template insertion.
+
+**No CSRF risk**: Existing `fetchWithAuth` pattern preserved for all API calls.
+
+**Score: 10/10** — No security concerns introduced.
+
+---
+
+## 6. UI/UX Assessment
+
+- **Tab title sync**: Renaming a page now immediately updates both tab bars. Good UX improvement with no visual regressions.
+- **Permission toast**: Users are notified when they lack edit access. Applied consistently in DocumentView and SheetView.
+- **No UI markup changes**: The PR is purely behavioral — no CSS, layout, or component structure changes that could affect visual appearance.
+
+**Score: 9/10** — Solid UX improvements.
+
+---
+
+## 7. Architecture & Design Patterns
+
+### Strengths
+
+**Per-document state ownership:**
+The migration from `useDocumentStore` (shared singleton with one `saveTimeoutId`) to `useDocumentManagerStore` (per-document `Map<string, DocumentState>`) is the correct architectural fix. The old store was fundamentally broken for multi-document scenarios.
+
+**Editing store integration:**
+Following the CLAUDE.md UI Refresh Protection pattern:
+```typescript
+useEffect(() => {
+  if (documentState?.isDirty) {
+    useEditingStore.getState().startEditing(page.id, 'document');
+  } else {
+    useEditingStore.getState().endEditing(page.id);
+  }
+  return () => useEditingStore.getState().endEditing(page.id);
+}, [documentState?.isDirty, page.id]);
+```
+This matches the documented pattern exactly.
+
+**hasLoadedRef pattern for SWR:**
+```typescript
+const hasLoadedRef = useRef(false);
+// ...
+isPaused: () => hasLoadedRef.current && isAnyActive,
+onSuccess: () => { hasLoadedRef.current = true; },
+```
+This matches the CLAUDE.md SWR protection pattern that says "Only pause AFTER initial load — never block the first fetch". Applied consistently in both hooks.
+
+### Findings
+
+**[Note] CanvasPageView now manages its own debounce rather than delegating to the store:**
+The store migration means CanvasPageView now owns the debounce timer, version counter, and save orchestration internally. This is valid because the component is the natural boundary for page-scoped state, and the old centralized approach was the root cause of the bug. However, if more page view types need this pattern, consider extracting a `useDocumentSave(pageId)` hook.
+
+**[Note] updateContentFromServer defined locally in CanvasPageView:**
+```typescript
+const updateContentFromServer = useCallback((newContent: string) => {
+  if (saveTimeoutRef.current) {
+    clearTimeout(saveTimeoutRef.current);
+    saveTimeoutRef.current = null;
+  }
+  useDocumentManagerStore.getState().updateDocument(page.id, {
+    content: newContent, isDirty: false, lastSaved: Date.now(), lastUpdateTime: Date.now(),
+  });
+}, [page.id]);
+```
+This cancels in-flight debounce before applying server content — correct behavior to prevent server content from being overwritten by a stale debounce.
+
+**Score: 9/10** — Sound architectural decisions aligned with project patterns.
+
+---
+
+## 8. Commit Quality
+
+**4 commits with clear progression:**
+1. `7b2180e` — Initial fix (store migration, hasLoadedRef, memo, AbortController, tab sync)
+2. `d7a8c26` — Review follow-up: isDirty reset and force-save on unmount
+3. `d9e8054` — Review follow-up: error propagation, version guard, stale cache fix
+4. `e6147c6` — Review follow-up: editing store registration, pageIdRef for safe cleanup
+
+All follow conventional commit format (`fix:` prefix with scope). Commit messages are descriptive.
+
+**Score: 9/10** — Clean commit history showing responsive iteration.
+
+---
+
+## Critical Findings
+
+### Strengths (Outstanding)
+
+1. **Root cause fix for data loss**: The shared `saveTimeoutId` bug is correctly diagnosed and the per-document migration is the right solution.
+2. **Comprehensive race condition handling**: Version counter, AbortController, and ref-based socket handlers collectively eliminate multiple timing bugs.
+3. **CLAUDE.md pattern compliance**: UI Refresh Protection, SWR hasLoadedRef guard, and editing store integration all follow the documented patterns exactly.
+4. **Review responsiveness**: Three follow-up commits systematically addressed CodeRabbit and reviewer feedback — error propagation, version guard, stale cache, editing store registration, and pageIdRef.
+5. **Clean debug log removal**: `console.log` statements for save flow removed.
+
+### Areas for Improvement
+
+1. **[Medium] Test coverage gap**: CanvasPageView's new save lifecycle (debounce, version guard, unmount force-save, error propagation) has no test coverage. This is the most critical change in the PR and the most likely to regress.
+
+2. **[Low] Component complexity**: CanvasPageView now manages debounce, version tracking, editing store registration, unmount cleanup, and server content sync internally. Consider extracting a `useDocumentSave(pageId, saveCallback)` hook if this pattern needs to be reused.
+
+3. **[Low] Missing usePageAgents test parity**: `hasLoadedRef` is tested in `useDevices.test.ts` but not in `usePageAgents`. Low risk since the pattern is identical.
+
+---
+
+## Final Assessment
+
+### Overall Score: 88/100 (Strong)
+
+| Dimension | Score | Notes |
+|-----------|-------|-------|
+| Requirements Adherence | 10/10 | All 10 stated fixes implemented |
+| Code Quality | 9/10 | Clean TypeScript, no type shortcuts |
+| React Patterns | 9/10 | Proper memo, refs, cleanup, AbortController |
+| Test Coverage | 7/10 | Existing tests updated; new coverage needed for CanvasPageView |
+| Security | 10/10 | No OWASP concerns; race conditions fixed |
+| Architecture | 9/10 | Sound per-document store migration; aligned with project patterns |
+| UX | 9/10 | Tab sync, permission toasts, no visual regressions |
+| Commit Quality | 9/10 | Clean conventional commits with responsive review iteration |
+
+### Production Readiness: APPROVED with follow-up
+
+**Recommendation**: Ship as-is. The fixes address real data loss and race condition bugs that are actively impacting users. Open a follow-up issue for CanvasPageView save lifecycle test coverage.
+
+### Follow-up Items
+1. Add unit tests for CanvasPageView save lifecycle (debounce, version guard, error propagation, unmount force-save)
+2. Add hasLoadedRef test parity for usePageAgents
+3. Consider extracting `useDocumentSave` hook if pattern is needed by additional page view types