refactor(tui): store list interaction per tab - #108
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough本次改动重构了 TUI Changes列表交互与详情排序状态统一
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant KeyboardHandler as 键盘/鼠标处理
participant App
participant ListInteractions as list_interactions映射
User->>KeyboardHandler: 切换到目标Tab
KeyboardHandler->>App: switch_tab(target)
App->>ListInteractions: persist_current_list_interaction()
App->>ListInteractions: stored_list_interaction(target)
ListInteractions-->>App: 返回目标Tab的selected/scroll/visible
App->>App: set_current_list_interaction(restored)
App-->>User: 渲染恢复后的列表选中/滚动状态
sequenceDiagram
participant User
participant App
participant ListInteractions as list_interactions映射
participant DetailSortContexts as detail_sort_contexts映射
User->>App: 进入Daily/Period详情
App->>ListInteractions: persist_list_interaction_for(tab)
App->>DetailSortContexts: enter_detail_sort_context(kind)
User->>App: 关闭详情(ESC)
App->>ListInteractions: stored_list_interaction(tab)
ListInteractions-->>App: 返回stored selected/scroll/visible
App->>App: 重建并锚定当前ListInteraction
App->>DetailSortContexts: leave_detail_sort_context(kind)
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 |
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
看完 #98 的决策记录和这版 diff 后，整体方向是对的：daily/period 的影子字段已经被收进 ListInteraction/detail sort context，switch_tab 也从手写搬运变成统一 persist/restore。下面两个 inline 是同一个恢复路径问题：退出 detail 时应使用保存的 parent ListInteraction.visible，而不是当前 detail renderer 留下的 self.max_visible_items。
b2f7a2d to
bdca036
Compare
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
重新 review 了当前 head bdca036。
结论：方向仍然对，但现在还不该合。之前两条 inline 仍然适用于当前文件内容：close_daily_detail 和 close_period_detail 在恢复 parent list viewport 时仍从 self.max_visible_items 取 visible capacity，而不是从保存的 parent ListInteraction.visible 取值。这会把 detail renderer 的可见行数混入 Daily/Monthly/Weekly parent list state，随后 clamp_selection() 又会把这个混入后的状态持久化回 tab。
建议修法保持很小：Daily 用 daily_interaction.visible.max(1)，Period 用 period_interaction.visible.max(1)；更干净的是直接构造/恢复一个完整的 parent ListInteraction { selected, scroll, visible }，不要在 close path 上混用 detail 的 self.max_visible_items。请同时补一个 parent/detail visible capacity 不同的回归测试。
注：GitHub 不允许我在自己的 PR 上提交 Request changes，所以这里用 comment review 表达同等结论。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/tokscale-cli/src/tui/app.rs (1)
1578-1591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win过往关于
visible容量被 detail 视口污染的问题已修复:此处已改用daily_interaction.visible而非self.max_visible_items,parent 列表容量得到保留。另外,
close_daily_detail与close_period_detail(Line 1646-1659)的回锚逻辑(restored_index回退、viewport_still_holds判断、scroll计算、重建ListInteraction)几乎完全一致——而这正是之前需要在两处分别修复visible污染 bug 的位置。建议抽取一个共用 helper(入参:tab、restored_index),让两条退出路径共享同一实现,避免后续再次出现分歧性缺陷。♻️ 建议:抽取共用回锚 helper
fn reanchor_from_stored(&mut self, tab: Tab, restored_index: usize) { let stored = self.stored_list_interaction(tab); let max_visible = stored.visible.max(1); let viewport_still_holds = restored_index >= stored.scroll && restored_index < stored.scroll + max_visible; let scroll = if viewport_still_holds { stored.scroll } else { restored_index.saturating_sub(max_visible / 2) }; self.set_current_list_interaction(ListInteraction { selected: restored_index, scroll, visible: stored.visible, }); }🤖 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 `@crates/tokscale-cli/src/tui/app.rs` around lines 1578 - 1591, There are two near-identical re-anchoring paths in close_daily_detail and close_period_detail that rebuild ListInteraction after restoring the selected index, and they should be unified to avoid future divergence. Extract a shared helper on App, such as reanchor_from_stored(tab, restored_index), that encapsulates the stored_list_interaction lookup, viewport_still_holds check, scroll calculation, and set_current_list_interaction call, and have both close_*_detail flows delegate to it. Keep the helper using the stored interaction’s visible value so the restored list state stays consistent across both tabs.
🤖 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 `@crates/tokscale-cli/src/tui/app.rs`:
- Around line 1578-1591: There are two near-identical re-anchoring paths in
close_daily_detail and close_period_detail that rebuild ListInteraction after
restoring the selected index, and they should be unified to avoid future
divergence. Extract a shared helper on App, such as reanchor_from_stored(tab,
restored_index), that encapsulates the stored_list_interaction lookup,
viewport_still_holds check, scroll calculation, and set_current_list_interaction
call, and have both close_*_detail flows delegate to it. Keep the helper using
the stored interaction’s visible value so the restored list state stays
consistent across both tabs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b1cf90d-c446-4972-a778-24af6ff8f0cb
📒 Files selected for processing (1)
crates/tokscale-cli/src/tui/app.rs
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Re-reviewed current head d25d3805.
之前指出的 Daily/Period detail 退出问题已经修掉：close_daily_detail / close_period_detail 现在都用 parent ListInteraction.visible 计算 viewport，并通过完整 ListInteraction { selected, scroll, visible } 恢复当前状态，不再把 detail 页面的 self.max_visible_items 写回 parent tab。
新增的 Daily / Weekly 回归测试也覆盖了 parent visible=2、detail visible=5 的污染场景，并断言退出后当前 max_visible_items 与 stored parent interaction visible 都恢复为 2。
我没有新的 blocking finding。之前两个 inline thread 已标记 resolved。注：我仍然不能在自己的 PR 上提交正式 Approve，所以这里用 comment review 表达 LGTM。
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Re-reviewed current head da8e732.
The Hourly v regression flagged by cubic is fixed cleanly. The key handler now calls reset_hourly_view_interaction() instead of the broad reset_selection() path, and the old broad reset helper has been removed rather than left around with ambiguous semantics. The new helper only resets the current Hourly list interaction and the hourly profile text viewport scroll, so stored interactions for Models/Daily/etc. are preserved.
The added test_hourly_view_toggle_preserves_other_tab_interactions covers the intended boundary: Models and Daily both save non-zero interactions, Hourly toggles table/profile and resets its own state, then switching back restores Models and Daily selected/scroll/visible.
Previously resolved Daily/Period detail visible restoration still looks correct, and the old review threads plus cubic's P2 thread are resolved. I found no new blocking issues. Since this is the author's own PR, I can only leave a comment review here; semantically this is LGTM pending the remaining in-progress workflows completing successfully.
1 participant
Summary
ListInteractionstate.visiblecapacity when leaving Daily/Period detail so detail viewport height cannot overwrite parent tab state.Closes #98
Built on the current
personal/local-clientsbaseline.Interaction Checklist
test_switch_tab_preserves_each_tab_list_interaction.test_switch_tab_from_daily_detail_restores_daily_parent_state.ListInteraction, includingvisible: covered bytest_esc_from_daily_detail_restores_daily_selectionandtest_esc_from_weekly_detail_restores_week_selection.test_hourly_view_toggle_preserves_other_tab_interactions.Validation
rg -n "daily_list_|period_list_" crates/tokscale-cli/src/tui/app.rsreturns no matchescargo fmt --all --checkcargo clippy --workspace --all-targetscargo test -p tokscale-cli test_esc_from_daily_detail_restores_daily_selection(1 passed, 817 filtered out)cargo test -p tokscale-cli test_esc_from_weekly_detail_restores_week_selection(1 passed, 817 filtered out)cargo test -p tokscale-cli test_hourly_view_toggle_preserves_other_tab_interactions(1 passed, 817 filtered out)cargo test -p tokscale-cli test_hourly_view_mode_toggle(1 passed, 818 filtered out)cargo test -p tokscale-cli switch_tab(8 passed, 810 filtered out)cargo test -p tokscale-cli(817 passed, 1 ignored)cargo test -p tokscale-core test_driver_uses_simple_file_adapter_when_only_amp_requested(1 passed, 1074 filtered out)cargo test --workspace(1889 passed, 4 ignored)git diff --name-status fork/personal/local-clients...HEADshows onlycrates/tokscale-cli/src/tui/app.rs