fix: persist orders table state for smooth scrolling - #113
Conversation
WalkthroughThe Orders tab now keeps its table selection and scrolling state in ChangesOrders table state
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🔵 Low · up to The PR persists Orders table state to preserve smooth scrolling and appears localized, but the repository-mandated build and formatting checks still need to be run and reported before merge; this is a bounded readiness follow-up rather than a blocking product risk. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
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)
src/ui/tabs/orders_tab.rs (1)
307-325: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a two-frame regression test for the persistent viewport.
The scrolling test renders
render_orders_tabonly once. It can pass withTableState::default()created on every frame, so it does not verify the behavior targeted by this PR. Render with the last order selected, changeapp.selected_order_idto the preceding order, render again, and assert that the viewport offset remains stable.As per coding guidelines, keep this test deterministic with
ratatui::backend::TestBackendand a fixed-size buffer.🤖 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 `@src/ui/tabs/orders_tab.rs` around lines 307 - 325, Extend orders_table_scrolls_to_keep_selected_row_visible to render two frames using the existing fixed-size TestBackend: first with last_id selected, then update app.selected_order_id to the preceding order and render again. Assert after the second render that the stateful table viewport offset remains unchanged, preserving the existing deterministic backend setup.Source: Coding guidelines
🤖 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 `@src/ui/tabs/orders_tab.rs`:
- Around line 307-325: Extend orders_table_scrolls_to_keep_selected_row_visible
to render two frames using the existing fixed-size TestBackend: first with
last_id selected, then update app.selected_order_id to the preceding order and
render again. Assert after the second render that the stateful table viewport
offset remains unchanged, preserving the existing deterministic backend setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a766a59-b207-4298-84c7-ce99b21c60b1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/ui/app_state.rssrc/ui/tabs/orders_tab.rs
Moved TableState to AppState to prevent Ratatui from resetting the viewport offset on every frame. This allows the viewport to smoothly follow the cursor when scrolling up the order book.
There was a problem hiding this comment.
Review: changes requested
The code change is narrowly scoped and the stateful TableState approach looks correct, but the current head does not pass the repository formatting gate.
Blocking:
cargo fmt --all -- --checkfails insrc/ui/app_state.rsbecause the newratatui::widgets::TableStateimport is out of rustfmt order. Please runcargo fmt --alland push the formatted result.
Verified locally on e739014d1ed761f54c9fc27867793ba6a37ecef8:
cargo fmt --all -- --check❌cargo test orders_table --all-features✅cargo check --all-targets --all-features✅cargo clippy --all-targets --all-features -- -D warnings✅cargo test --all-features✅
Summary
This PR fixes the Orders tab table scroll behavior when navigating upwards through the list.
TableStateinsideAppStateto prevent Ratatui from resetting the viewport offset on every frame.render_orders_tabto use a mutableAppStatereference.Testing
cargo check --all-targets --all-featurescargo test --all-featurescargo clippy --all-targets --all-features -- -D warningsAll checks passed successfully.
Summary by CodeRabbit