fix(web): keep archived list pagination on unarchive#635
Merged
Conversation
The Data Controls archive list optimistically hid rows on Unarchive but never rolled back on failure, and the subsequent mutate(ARCHIVED_SESSIONS_KEY) revalidation reset the "Load more" pagination via the SWR onSuccess callback. Switch to the request-first + scoped cache-update pattern used by the session sidebar: on success, remove the row from the first-page SWR cache with revalidate: false and filter it from extraSessions; on failure, leave state alone so the row stays visible. Adds component-level tests covering: success removes the row, 500 keeps it visible, page-1 unarchive preserves page-2 rows, page-2 unarchive removes only that row, and successful unarchive revalidates the sidebar.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates the unarchive flow in ChangesUnarchive Flow Implementation and Testing
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
PR #635, fix(web): keep archived list pagination on unarchive, by @ColeMurray. Reviewed 2 changed files (+326/-14); the request-first cache update is a good direction and the new focused tests pass, but there is one remaining pagination correctness issue before merge.
Critical Issues
- [Functionality]
packages/web/src/components/settings/data-controls-settings.tsx:71- After a successful unarchive, the fetched row is removed from local UI state butoffsetis not decremented. A later Load more request can skip the next archived session because the server-side result set has shifted.
Suggestions
- [Testing] Add coverage for unarchive followed by another Load more request with enough archived sessions to keep
hasMoretrue. This would catch the stale-offset regression.
Nitpicks
- None.
Positive Feedback
- The request-first behavior fixes the failed-unarchive rollback problem cleanly by leaving visible state untouched on errors.
- Using a scoped SWR cache update with
revalidate: falseavoids collapsing already-loaded extra rows on successful unarchive. - The tests cover success, failure, page-2 removal, and sidebar invalidation paths.
Questions
- None.
Verdict
Request Changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/components/settings/data-controls-settings.test.tsx (1)
268-285: ⚡ Quick winConsider extracting the SWRConfig setup to reduce duplication.
The
SWRConfigsettings here are identical to those inrenderComponent()(lines 108-119). You could refactorrenderComponentto accept optional children, eliminating the duplication.♻️ Proposed refactor
-function renderComponent() { +function renderComponent(additionalChildren?: React.ReactNode) { return render( <SWRConfig value={{ dedupingInterval: 0, revalidateOnFocus: false, revalidateIfStale: false, revalidateOnReconnect: false, fetcher: async (url: string) => { const response = await fetch(url); if (!response.ok) throw new Error(`HTTP ${response.status}`); return response.json(); }, }} > <DataControlsSettings /> + {additionalChildren} </SWRConfig> ); }Then update the sidebar test:
- render( - <SWRConfig - value={{ - dedupingInterval: 0, - revalidateOnFocus: false, - revalidateIfStale: false, - revalidateOnReconnect: false, - fetcher: async (url: string) => { - const response = await fetch(url); - if (!response.ok) throw new Error(`HTTP ${response.status}`); - return response.json(); - }, - }} - > - <DataControlsSettings /> - <SidebarProbe /> - </SWRConfig> - ); + renderComponent(<SidebarProbe />);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/components/settings/data-controls-settings.test.tsx` around lines 268 - 285, The SWRConfig block is duplicated; refactor the helper renderComponent to accept optional children and the SWR config so tests reuse it: update renderComponent(...) (the test helper) to wrap children with the SWRConfig settings (dedupingInterval: 0, revalidateOnFocus: false, revalidateIfStale: false, revalidateOnReconnect: false, and the fetcher) and then replace the inline SWRConfig in this test by calling renderComponent(<DataControlsSettings />, <SidebarProbe />) or renderComponent(<><DataControlsSettings /><SidebarProbe /></>) so DataControlsSettings and SidebarProbe are rendered without duplicating the SWRConfig setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/web/src/components/settings/data-controls-settings.test.tsx`:
- Around line 268-285: The SWRConfig block is duplicated; refactor the helper
renderComponent to accept optional children and the SWR config so tests reuse
it: update renderComponent(...) (the test helper) to wrap children with the
SWRConfig settings (dedupingInterval: 0, revalidateOnFocus: false,
revalidateIfStale: false, revalidateOnReconnect: false, and the fetcher) and
then replace the inline SWRConfig in this test by calling
renderComponent(<DataControlsSettings />, <SidebarProbe />) or
renderComponent(<><DataControlsSettings /><SidebarProbe /></>) so
DataControlsSettings and SidebarProbe are rendered without duplicating the
SWRConfig setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75b5a0fe-dde8-41d8-85c8-2e033ecd2b78
📒 Files selected for processing (2)
packages/web/src/components/settings/data-controls-settings.test.tsxpackages/web/src/components/settings/data-controls-settings.tsx
Successful unarchive removed the row from the visible list but left the local Load-more `offset` pointing at the pre-unarchive count. The server-side archived list (ORDER BY updated_at DESC + OFFSET) shifts down by one when a row leaves the result set, so the next Load more request started one offset too high and skipped the session that took the freed slot. Decrements `offset` on success and adds a regression test that loads page 1 (20 of 23 sessions), unarchives session 1, clicks Load more, and asserts session 21 — which now sits at offset 19 — appears. Also refactors the test helper to a flat-list `archivedSessions` mock so pagination shifts naturally, and threads `additionalChildren` through `renderComponent` so the sidebar-invalidation test reuses the SWRConfig wrapper.
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Owner
Author
|
CodeRabbit nit (SWRConfig duplication in the test file) addressed in 639ae7f — threaded an optional |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hiddenIdson failure, and on success themutate(ARCHIVED_SESSIONS_KEY)revalidation collapsed any "Load more" pagination via the SWRonSuccesscallback.session-sidebar.tsx(handleSessionArchived): on success, remove the row from the first-page SWR cache withrevalidate: falseand filter it fromextraSessions; on failure, leave state alone so the row stays visible.Behavior change
toast.errorshownextraSessionsmutate(SIDEBAR_SESSIONS_KEY)(kept)Out of scope (flagged for follow-up)
onSuccesscallback on the archived-sessionsuseSWRstill resetsextraSessionson unrelated revalidations (focus/reconnect). The sidebar handles the same issue with aprevDataRefsynchronous-render pattern (session-sidebar.tsx:96-101); worth porting in a separate PR./api/sessions/[id]/unarchiveroute handler — would match theroute.test.tspattern from existing API route tests.Test plan
npx vitest runinpackages/web— 30 files, 242 tests pass (5 new indata-controls-settings.test.tsx)npx tsc --noEmitinpackages/web— cleannpx eslint src/inpackages/web— clean/api/sessions/<id>/unarchiveto 500 via devtools, confirm row stays visible and an error toast appears>20archived sessions, click "Load more", then unarchive a session from page 1; confirm page-2 rows remain visibleSummary by CodeRabbit
Bug Fixes
Tests