♻️(frontend) refactor thread query cache management#642
♻️(frontend) refactor thread query cache management#642
Conversation
The thread query is an infinite one and the frontend logic is based on the structuralSharing concept of react-query to optimiscally update the react query cache on thread mutation in order to improve ux. This part is a tricky one and it's easy to introduce regression, that's why refactor it by moving the corresponding logic into a mailbox-cache module and battle test it.
📝 WalkthroughWalkthroughThe PR modifies thread selection timing across UI components (moving from async Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 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: 1
🧹 Nitpick comments (3)
src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx (1)
186-197: Optional: consider guarding the synchronous unselect to the unread direction for consistency with the dropdown.The main button is a toggle and currently calls
unselectThread()/onClearSelection()synchronously for both directions, but the visibility-observer race only justifies it for the mark-as-unread path. The dropdown's "Mark as read" option (lines 291–304) still defers unselection toonSuccess, so the two controls diverge when the user marks an already-read selection as read (main button closes the open thread immediately; dropdown waits for success). Also note this is a behavior change for the mark-as-read direction, which previously kept the thread view open until the mutation succeeded.If the intent is to keep sync-close strictly for the unread case, something like:
♻️ Proposed adjustment
onClick={() => { - // Close the open thread before firing the mutation. Waiting for - // onSuccess would let the visibility observer re-observe the - // newly-unread messages and debounce a mark-as-read that silently - // reverts the action. - unselectThread(); - onClearSelection(); - markAsReadAt({ - threadIds: threadIdsToMark, - readAt: selectionReadStatus === SelectionReadStatus.READ ? null : new Date().toISOString(), - }); + const willMarkUnread = selectionReadStatus === SelectionReadStatus.READ; + if (willMarkUnread) { + // Close the open thread before firing the mutation. Waiting for + // onSuccess would let the visibility observer re-observe the + // newly-unread messages and debounce a mark-as-read that silently + // reverts the action. + unselectThread(); + onClearSelection(); + markAsReadAt({ threadIds: threadIdsToMark, readAt: null }); + } else { + markAsReadAt({ + threadIds: threadIdsToMark, + readAt: new Date().toISOString(), + onSuccess: () => { + unselectThread(); + onClearSelection(); + }, + }); + } }}Otherwise, consider aligning the dropdown "Mark as read" option (lines 291–304) to also unselect synchronously so both controls behave the same. Happy either way — just flagging the asymmetry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx` around lines 186 - 197, The main button always calls unselectThread() and onClearSelection() synchronously before markAsReadAt, but the dropdown's "Mark as read" path defers unselection to the mutation onSuccess, causing inconsistent UX; update the main-button click handler (the inline onClick that calls unselectThread(), onClearSelection(), and markAsReadAt()) to only perform the synchronous unselect when toggling to unread (i.e., when selectionReadStatus !== SelectionReadStatus.READ), and otherwise defer unselecting/clearing selection until the markAsReadAt mutation succeeds (aligning with the dropdown), or alternatively change the dropdown handler that uses onSuccess to also call unselectThread()/onClearSelection() synchronously—pick one approach and apply it consistently around markAsReadAt and the mutation onSuccess callbacks.src/frontend/src/features/providers/mailbox.tsx (1)
258-273: Nit: redundantpage > 1branch inprevious.Inside the catch block you already gate on
page > 1(line 260), soprevious: page > 1 ? page - 1 : nullcan only ever evaluate topage - 1. The inner ternary is dead code.♻️ Simplification
- return { - status: 200, - data: { - count: 0, - results: [], - next: null, - previous: page > 1 ? page - 1 : null, - } as PaginatedThreadList, - headers: new Headers(), - } as threadsListResponse; + return { + status: 200, + data: { + count: 0, + results: [], + next: null, + previous: page - 1, + } as PaginatedThreadList, + headers: new Headers(), + } as threadsListResponse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/providers/mailbox.tsx` around lines 258 - 273, The catch block that handles APIError checks page > 1 already (using page derived from pageParam), so the ternary used for the previous field is redundant; update the returned object in the APIError 404 branch (the block that returns a threadsListResponse / PaginatedThreadList) to set previous to page - 1 directly instead of using previous: page > 1 ? page - 1 : null, leaving all other fields intact and keeping the same APIError check and types.src/frontend/src/features/providers/mailbox-cache.test.ts (1)
1-566: LGTM — battle-tested indeed.Coverage is excellent: per-page re-insertion, duplicate prevention across pages, count inflation on the impacted page, reference preservation on no-op pages (line 316),
fetchNextPagevs server-confirm distinction, mark-then-mark regressions, read-pointer derivation for both "up to here" and "from here", plus the no-pointer pass-through. The inline comments tying each test to the real scenario (e.g. lines 206-224, 226-252, 277-299) are great documentation.One gap worth considering: every timestamp in the
applyMessageUpdatesuite uses the same ISO precision (.sssZor none, but consistent per test). Adding a case that mixes2026-01-02T00:00:00Zwith2026-01-02T00:00:00.000Zwould surface the lexicographic-comparison fragility flagged onmailbox-cache.tsline 143.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/providers/mailbox-cache.test.ts` around lines 1 - 566, Tests for applyMessageUpdate miss a case where message timestamps have mixed ISO precision (e.g. "2026-01-02T00:00:00Z" vs "2026-01-02T00:00:00.000Z"), which can hide lexicographic comparison bugs referenced at mailbox-cache.ts line 143; add a unit test in mailbox-cache.test.ts under the "derives is_unread..." scenarios that mixes precision across messages (use one message with ".000Z" and another without) and asserts the intended readAt behavior, and then fix applyMessageUpdate to normalize/parse timestamps (e.g. use Date.parse or toISOString normalization) when deriving is_unread so comparisons are robust to precision differences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/src/features/providers/mailbox-cache.ts`:
- Around line 143-146: The current deriveIsUnread function compares ISO
timestamp strings lexicographically (createdAt > readAt) which is fragile across
different ISO precisions; change deriveIsUnread to parse createdAt and readAt
into numeric epoch times (e.g., Date.parse or new Date(...).getTime()) and
compare those numbers, handle readAt === null as before and guard against
invalid dates (NaN) by treating invalid readAt as unread or logging/falling
back; update tests to include a regression case where createdAt has milliseconds
and readAt does not (and vice versa) to ensure mixed-precision inputs yield the
correct boolean.
---
Nitpick comments:
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 186-197: The main button always calls unselectThread() and
onClearSelection() synchronously before markAsReadAt, but the dropdown's "Mark
as read" path defers unselection to the mutation onSuccess, causing inconsistent
UX; update the main-button click handler (the inline onClick that calls
unselectThread(), onClearSelection(), and markAsReadAt()) to only perform the
synchronous unselect when toggling to unread (i.e., when selectionReadStatus !==
SelectionReadStatus.READ), and otherwise defer unselecting/clearing selection
until the markAsReadAt mutation succeeds (aligning with the dropdown), or
alternatively change the dropdown handler that uses onSuccess to also call
unselectThread()/onClearSelection() synchronously—pick one approach and apply it
consistently around markAsReadAt and the mutation onSuccess callbacks.
In `@src/frontend/src/features/providers/mailbox-cache.test.ts`:
- Around line 1-566: Tests for applyMessageUpdate miss a case where message
timestamps have mixed ISO precision (e.g. "2026-01-02T00:00:00Z" vs
"2026-01-02T00:00:00.000Z"), which can hide lexicographic comparison bugs
referenced at mailbox-cache.ts line 143; add a unit test in
mailbox-cache.test.ts under the "derives is_unread..." scenarios that mixes
precision across messages (use one message with ".000Z" and another without) and
asserts the intended readAt behavior, and then fix applyMessageUpdate to
normalize/parse timestamps (e.g. use Date.parse or toISOString normalization)
when deriving is_unread so comparisons are robust to precision differences.
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 258-273: The catch block that handles APIError checks page > 1
already (using page derived from pageParam), so the ternary used for the
previous field is redundant; update the returned object in the APIError 404
branch (the block that returns a threadsListResponse / PaginatedThreadList) to
set previous to page - 1 directly instead of using previous: page > 1 ? page - 1
: null, leaving all other fields intact and keeping the same APIError check and
types.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4fcc765-93b2-4354-bc69-513a6c5998f3
📒 Files selected for processing (6)
src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/providers/mailbox-cache.test.tssrc/frontend/src/features/providers/mailbox-cache.tssrc/frontend/src/features/providers/mailbox.tsx
| const deriveIsUnread = (createdAt: string, readAt: string | null): boolean => { | ||
| if (readAt === null) return true; | ||
| return createdAt > readAt; | ||
| }; |
There was a problem hiding this comment.
Fragile timestamp comparison in deriveIsUnread.
Lexicographic > on ISO strings only works when both operands share the exact same shape. Mixing formats — e.g. 2026-01-02T00:00:00Z (no ms) vs 2026-01-02T00:00:00.000Z (with ms) — breaks ordering: 'Z' (0x5A) > '.' (0x2E), so the no-ms string sorts after the same instant formatted with ms, flipping is_unread for a message created at exactly the read pointer.
This happens silently in production when the API emits timestamps without fractional seconds while a client-side new Date().toISOString() (always .sssZ) feeds readAt. The test suite happens to use consistent formatting so it does not catch this.
🛡️ Proposed fix
-const deriveIsUnread = (createdAt: string, readAt: string | null): boolean => {
- if (readAt === null) return true;
- return createdAt > readAt;
-};
+const deriveIsUnread = (createdAt: string, readAt: string | null): boolean => {
+ if (readAt === null) return true;
+ return new Date(createdAt).getTime() > new Date(readAt).getTime();
+};Worth adding a regression test with mixed-precision inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/src/features/providers/mailbox-cache.ts` around lines 143 - 146,
The current deriveIsUnread function compares ISO timestamp strings
lexicographically (createdAt > readAt) which is fragile across different ISO
precisions; change deriveIsUnread to parse createdAt and readAt into numeric
epoch times (e.g., Date.parse or new Date(...).getTime()) and compare those
numbers, handle readAt === null as before and guard against invalid dates (NaN)
by treating invalid readAt as unread or logging/falling back; update tests to
include a regression case where createdAt has milliseconds and readAt does not
(and vice versa) to ensure mixed-precision inputs yield the correct boolean.
1 participant
Purpose
The thread query is an infinite one and the frontend logic is based on the structuralSharing concept of react-query to optimiscally update the react query cache on thread mutation in order to improve ux. This part is a tricky one and it's easy to introduce regression, that's why refactor it by moving the corresponding logic into a mailbox-cache module and battle test it.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements