feat: preserve orch table state on back nav#713
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adds persistence for the orchestrator list browsing experience so that navigating into an orchestrator account and returning via Back restores list context (yield assumptions, sorting, pagination, and scroll position) for both the home page and /orchestrators.
Changes:
- Adds a keyed persisted orchestrator-list state to the Zustand explorer store (
homevsorchestrators). - Extends the shared Table wrapper to support disabling auto-resets and emitting pagination/sort state changes.
- Implements scroll position capture/restore for orchestrator lists using
sessionStorage+ Next router events.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pages/orchestrators.tsx | Passes a stable listKey so /orchestrators persists/restores its own list state. |
| pages/index.tsx | Passes listKey="home" so the homepage list persists independently from /orchestrators. |
| hooks/useExplorerStore.tsx | Introduces persisted orchestrator list state and an updater API in the global store. |
| components/Table/index.tsx | Adds table state persistence hooks (no auto-reset + onStateChange). |
| components/OrchestratorList/index.tsx | Persists yield assumptions, table state, and scroll position keyed per list route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds keyed persisted UI state for explorer lists, including pagination, sort order, scroll position, and orchestrator assumptions. Shared table and hook utilities emit and restore that state, and page entry points pass stable list identifiers and route paths. ChangesPersist Explorer List State
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@components/OrchestratorList/index.tsx`:
- Around line 189-218: The scroll restoration in restoreSavedScroll can still
fire before the page is tall enough, so a single RAF loop plus the 250ms timeout
may clamp the position near the top and stop retrying. Update restoreSavedScroll
in OrchestratorList to keep retrying until the document is tall enough for
scrollY (for example by checking document.documentElement.scrollHeight -
window.innerHeight >= scrollY) or by re-invoking restoration when the list
data/rows finish loading, while keeping a bounded timeout. Make sure the
retry/cancellation logic remains tied to restoreSavedScroll and its cleanup
return from useEffect.
- Around line 1348-1355: The delegation input in OrchestratorList’s onChange
handler only caps the upper bound, so invalid values like 0, negatives, or an
empty field can still enter state. Update the setPrinciple flow to clamp both
ends (respecting the minimum of 1 as well as maxSupplyTokens), or switch to
storing the input as a string and only commit a validated numeric value. Make
sure the fix is applied in the same onChange logic that currently calls
Number(e.target.value) and setPrinciple.
In `@hooks/useExplorerStore.tsx`:
- Around line 9-22: OrchestratorListState is duplicating the canonical ROI union
types instead of reusing the shared aliases from lib/roi.ts. Update
useExplorerStore.tsx to import and use ROIFactors, ROIInflationChange, and
ROITimeHorizon in OrchestratorListState so the store stays aligned with the
central ROI definitions and avoids type drift.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f452562-ceda-4b9b-80d9-e1a54adae7d4
📒 Files selected for processing (5)
components/OrchestratorList/index.tsxcomponents/Table/index.tsxhooks/useExplorerStore.tsxpages/index.tsxpages/orchestrators.tsx
There was a problem hiding this comment.
@ECWireless thanks for this pull request. One comment about the caching frequency. Also validated it works:
@rickstaa it revalidates if more than 10 seconds have passed since the last user |
rickstaa
left a comment
rickstaa
left a comment
There was a problem hiding this comment.
@ECWireless would it be possible to generalise to all tables. The gateway and ticket tables also benefit of keeping state.
Added to the Performance Leaderboard, Transactions, and Gateways tables in e41452b. The only one I skipped was the Treasury Votes table, since the dynamic routing makes it a bit more complex. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@hooks/usePersistedExplorerListState.ts`:
- Around line 29-203: Add JSDoc to usePersistedExplorerListState and the helper
functions getScrollStorageKey, readStoredScrollY, and writeStoredScrollY.
Document the hook’s purpose, its params (listKey, routePath, optional
persistedState and setPersistedState overrides), and the returned shape
(handleTableStateChange, persistedState, saveCurrentScroll), plus brief one-line
intent for the scroll storage helpers so the restore/storage flow is clear.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc8b71e7-1bde-411c-8780-37a5130507d1
📒 Files selected for processing (10)
components/GatewayList/index.tsxcomponents/OrchestratorList/index.tsxcomponents/PerformanceList/index.tsxcomponents/TransactionsList/index.tsxhooks/index.tsxhooks/useExplorerStore.tsxhooks/usePersistedExplorerListState.tspages/gateways.tsxpages/index.tsxpages/transactions.tsx
rickstaa
left a comment
rickstaa
left a comment
There was a problem hiding this comment.
@ECWireless very clean PR thanks. I left some minor comments. If you can respond or handle those and update the squash commit to include the fact that this now applies to all tables we can merge. 🙏🏻
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)
components/TransactionsList/index.tsx (1)
567-582: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClamp the restored page index before passing it to
Table
- Problem:
persistedState.pageIndexis reused as-is withautoResetPage={false}, so a shortereventsset can mount on an out-of-range page and render an empty table.- Why it matters: Back navigation or data changes can leave users stranded on a blank transactions page with no automatic recovery.
- Suggested fix: Clamp the restored
pageIndexto the current page count, or reset it when the currenteventslength no longer supports the saved page.🤖 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 `@components/TransactionsList/index.tsx` around lines 567 - 582, The restored page index in TransactionsList is being passed directly into Table via the initialState pageIndex, which can leave the table on an invalid page when the events set is shorter. Update the TransactionsList component to clamp persistedState.pageIndex against the current page count derived from events and pageSize before building the Table initialState, or reset it when the saved page is no longer valid. Keep the fix localized around the initialState passed to Table and the related persistedState handling so back navigation and data refreshes always land on a valid page.
🤖 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.
Outside diff comments:
In `@components/TransactionsList/index.tsx`:
- Around line 567-582: The restored page index in TransactionsList is being
passed directly into Table via the initialState pageIndex, which can leave the
table on an invalid page when the events set is shorter. Update the
TransactionsList component to clamp persistedState.pageIndex against the current
page count derived from events and pageSize before building the Table
initialState, or reset it when the saved page is no longer valid. Keep the fix
localized around the initialState passed to Table and the related persistedState
handling so back navigation and data refreshes always land on a valid page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0aeb3f21-14b3-4af8-a705-e208fc5b4f89
📒 Files selected for processing (1)
components/TransactionsList/index.tsx
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 (2)
components/GatewayList/index.tsx (1)
310-341: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing bounds check on restored
pageIndex.
persistedState.pageIndexis passed straight intoinitialState.pageIndexwith no clamp against the currentrows.length. If the gateway list is shorter on return (filtered/updated data) than when the page index was saved, react-table will render an out-of-range page (empty table) at mount, sinceinitialStateis only applied once.TransactionsListin this same PR already solves this by clamping (Math.min(persistedState.pageIndex, pageCount - 1)) — this file should do the same.Problem: No bounds check on restored
pageIndexagainst current row count.
Why it matters: Back-navigation with a shrunk gateway list can show a blank table instead of the intended restored view — directly undermines this PR's goal.
Suggested fix: ClamppageIndexthe same wayTransactionsListdoes.🐛 Proposed fix
if (!rows?.length) { ... } + const pageCount = Math.max(1, Math.ceil(rows.length / pageSize)); + const pageIndex = Math.min(persistedState.pageIndex, pageCount - 1); + return ( <Box onClickCapture={saveCurrentScroll}> <Table data={rows} columns={columns} autoResetPage={false} autoResetSortBy={false} onStateChange={handleTableStateChange} initialState={{ - pageIndex: persistedState.pageIndex, + pageIndex, pageSize, sortBy: persistedState.sortBy.length ? (persistedState.sortBy as SortingRule<GatewayRow>[]) : DEFAULT_SORT_BY, }} /> </Box> );🤖 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 `@components/GatewayList/index.tsx` around lines 310 - 341, The restored page index in GatewayList should be clamped before being passed to Table initialState, since persistedState.pageIndex can be out of range when rows.length has changed. Update the GatewayList component to compute a bounded pageIndex the same way TransactionsList does, using the current row count/page count, and pass that safe value into initialState.pageIndex so react-table mounts on a valid page.components/PerformanceList/index.tsx (1)
51-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame missing
pageIndexclamp asGatewayList.
initialState.pageIndexusespersistedState.pageIndexdirectly, unbounded againstmergedData.length. SinceorchestratorIds/region/pipeline/modelcan change the effective row count between sessions (or on remount viakey={listKey}), a stale, largerpageIndexwill render an empty leaderboard page at mount instead of resuming correctly.Problem: No clamp of restored
pageIndexto the current data size.
Why it matters: Same blank-table regression risk described forGatewayList, here on the leaderboard.
Suggested fix: Clamp usingmergedData.length(computed aboveinitialState, since it's already declared later in the file — reorder if needed).🐛 Proposed fix
+ const mergedData = useMemo( + () => + orchestratorIds.map((o) => ({ ...o, ...performanceMetrics?.[o?.id] })), + [performanceMetrics, orchestratorIds] + ); + + const pageCount = Math.max(1, Math.ceil(mergedData.length / pageSize)); + const clampedPageIndex = Math.min(persistedState.pageIndex, pageCount - 1); + const initialState = { - pageIndex: persistedState.pageIndex, + pageIndex: clampedPageIndex, pageSize: pageSize, sortBy: persistedState.sortBy.length ? persistedState.sortBy : DEFAULT_SORT_BY, ... }; - - const mergedData = useMemo( - () => - orchestratorIds.map((o) => ({ ...o, ...performanceMetrics?.[o?.id] })), - [performanceMetrics, orchestratorIds] - );🤖 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 `@components/PerformanceList/index.tsx` around lines 51 - 80, The restored page index in PerformanceList is used without bounds checking, which can leave the leaderboard on an empty page when the data size changes. Update the initialState setup in PerformanceList to clamp persistedState.pageIndex against the current mergedData.length, similar to GatewayList, and reorder the mergedData computation if needed so the clamp can be applied before initialState is built.
🤖 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.
Outside diff comments:
In `@components/GatewayList/index.tsx`:
- Around line 310-341: The restored page index in GatewayList should be clamped
before being passed to Table initialState, since persistedState.pageIndex can be
out of range when rows.length has changed. Update the GatewayList component to
compute a bounded pageIndex the same way TransactionsList does, using the
current row count/page count, and pass that safe value into
initialState.pageIndex so react-table mounts on a valid page.
In `@components/PerformanceList/index.tsx`:
- Around line 51-80: The restored page index in PerformanceList is used without
bounds checking, which can leave the leaderboard on an empty page when the data
size changes. Update the initialState setup in PerformanceList to clamp
persistedState.pageIndex against the current mergedData.length, similar to
GatewayList, and reorder the mergedData computation if needed so the clamp can
be applied before initialState is built.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cb26b1e-9a60-4570-a9ae-32d8f8f286e4
📒 Files selected for processing (3)
components/GatewayList/index.tsxcomponents/PerformanceList/index.tsxcomponents/TransactionsList/index.tsx
rickstaa
left a comment
rickstaa
left a comment
There was a problem hiding this comment.
Thanks for handling my commnents good to merge. 🔥
Description
This PR preserves orchestrator list browsing context when navigating into an orchestrator account and returning with Back. The home and
/orchestratorslists now restore yield assumptions, sorting, pagination, and scroll position, with cached list data left in place so users can continue browsing where they left off.Type of Change
Related Issue(s)
Closes: #349
Changes Made
Testing
How to test (optional unless test is not trivial)
/orchestratorspageImpact / Risk
Risk level: Low
Impacted areas: UI
User impact: user does not lose their orchestrator search preferences when they click on a particular orchestrator
Rollback plan: Vercel rollback, then manual git revert
Summary by CodeRabbit