fix: resolve stale closure in updateNode causing unread indicator to not clear

[2witstudios]

The updateNode function captured data from the closure, which could be
undefined or stale when called from an async callback. When a user opened
an unread page, the POST to mark as viewed would complete, but the
updateNode call would operate on stale data (often an empty array),
failing to find the node and never clearing the blue dot.

Fix uses SWR's functional mutate pattern to always receive the current
cache data as an argument, ensuring updates work correctly regardless
of when the callback executes.

https://claude.ai/code/session_01GGDSpoDcDwMq2aB4jmu5na

Summary by CodeRabbit

• Refactor

  • Improved page tree update flow for more efficient, memoized updates and reduced unnecessary data refreshes.

• Bug Fixes

  • Ensures page-view actions refresh unread counts when the tree is still loading.

• Tests

  • Updated tests to cover optimistic updater behavior, nested/missing-node scenarios, and handling when no tree data is loaded.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

📝 Walkthrough

Walkthrough

UsePageTree now applies SWR functional updaters and memoized callbacks for optimistic edits; fetch-and-merge and node-updates use functional mutate. The dashboard page tracks tree-loading state and conditionally triggers revalidation after posting a page view. Tests updated to assert functional-updater behavior.

Changes

Cohort / File(s)	Summary
usePageTree hook apps/web/src/hooks/usePageTree.ts	Replaced direct mutate-with-value calls with SWR functional updaters; fetchAndMergeChildren uses mutate((current)=>...) and dependency reduced to [mutate]; updateNode converted to a useCallback that invokes mutate with a recursive updater returning newTree only when changes occur; hook now exposes mutate and isLoading.
Page component apps/web/src/app/dashboard/[driveId]/[pageId]/page.tsx	Destructures mutate and isLoading from the hook; adds isTreeLoadingRef synced from isLoading and a cleanup/useEffect that conditionally calls mutate() to revalidate the tree after a successful page-view POST when the tree was previously loading.
Tests apps/web/src/hooks/__tests__/usePageTree.test.ts	Updated assertions to expect SWR functional updaters: verify mutate called with an updater function and { revalidate: false }; invoke the provided updater with mock data to assert updated tree, nested updates, no-data handling (returns undefined), and no-op for non-existent nodes (returns same reference).

Sequence Diagram(s)

sequenceDiagram
    participant Page as Page Component
    participant API as Page View API
    participant Hook as usePageTree Hook
    participant SWR as SWR Cache

    Page->>API: POST /page/view
    API-->>Page: 200 OK
    Page->>Hook: check isTreeLoadingRef (mirrors isLoading)
    alt tree was loading
        Hook->>SWR: mutate(updater, {revalidate: true})
        SWR-->>Hook: cache revalidated / updated
    end
    SWR-->>Page: UI reflects updated tree (via SWR subscription)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇
I hopped through updater lanes so neat,
Wrapped changes in a function-beat,
Mutate danced with current cache in tow,
The tree refreshed and titles glowed,
Tiny hops — big tidy feat! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: resolving a stale closure issue in updateNode that prevents the unread indicator from clearing, which directly matches the core problem and solution described in the PR objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch claude/fix-unread-indicator-7t93l

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

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/hooks/__tests__/usePageTree.test.ts (1)

220-244: ⚠️ Potential issue | 🔴 Critical

Use functional updater pattern for fetchAndMergeChildren to fix stale closure issue.

The concern is valid. fetchAndMergeChildren captures data from closure and then awaits an async operation before calling mutate(updatedTree, false). While awaiting the fetcher, WebSocket events from usePageTreeSocket can trigger concurrent mutations via invalidateTree, making the captured data stale. This mirrors the stale closure vulnerability that was already fixed in updateNode using a functional updater.

Change line 81-83 from:

const currentTree = data || [];
const updatedTree = mergeChildren(currentTree, pageId, children);
mutate(updatedTree, false);

To use the functional updater pattern like updateNode:

mutate((currentData) => {
  const currentTree = currentData || [];
  return mergeChildren(currentTree, pageId, children);
}, { revalidate: false });

The test should also be updated to verify the functional behavior, not just that mutate was called with expect.any(Array).