This repository was archived by the owner on Jul 30, 2026. It is now read-only.
refactor(tui): unify responsive table layout - #84
Merged
makoMakoGo merged 1 commit intoJun 27, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
Walkthrough多个 TUI 表格改为共享响应式列布局，统一生成列集合、列宽和截断宽度，并移除了旧的手工宽度分配路径；Agents、Daily、Hourly、Models、Period 的渲染与测试也同步更新。 Changes响应式 TUI 表格布局
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
makoMakoGo
force-pushed
the
codex/tui-responsive-table-layout
branch
from
June 26, 2026 15:51
eebd80d to
c150f49
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/tokscale-cli/src/tui/ui/daily.rs`:
- Around line 807-810: Restore the muted theme for the
DailyDetailColumn::Provider cell in the daily table rendering path so it matches
the Source column’s styling. Update the Cell construction in the branch that
formats get_provider_display_name(...) and truncate_display_width(...) to apply
theme_muted after truncation, keeping the visual hierarchy consistent with the
other detail columns.
In `@crates/tokscale-cli/src/tui/ui/period.rs`:
- Around line 588-591: The Provider cell in the Period Detail table is missing
the muted styling, causing it to render with the default foreground while Source
still uses muted. Update the PeriodDetailColumn::Provider branch in the period
table rendering logic to apply the same muted style used by the Source cell,
keeping the visual hierarchy consistent in the Period Detail view.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8b7e09d-4488-49d3-a394-04a4a84043e7
📒 Files selected for processing (7)
crates/tokscale-cli/src/tui/ui/agents.rscrates/tokscale-cli/src/tui/ui/daily.rscrates/tokscale-cli/src/tui/ui/hourly.rscrates/tokscale-cli/src/tui/ui/model_usage_layout.rscrates/tokscale-cli/src/tui/ui/models.rscrates/tokscale-cli/src/tui/ui/period.rscrates/tokscale-cli/src/tui/ui/table_layout.rs
makoMakoGo
marked this pull request as ready for review
June 26, 2026 16:13
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 7 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
makoMakoGo
marked this pull request as draft
June 26, 2026 16:38
makoMakoGo
force-pushed
the
codex/tui-responsive-table-layout
branch
4 times, most recently
from
June 26, 2026 18:04
8706efe to
8ef9fb1
Compare
makoMakoGo
commented
Jun 27, 2026
makoMakoGo
left a comment
makoMakoGo
left a comment
Owner
Author
There was a problem hiding this comment.
PR 84 re-review（更新意见）
整体：当前 PR 在“视觉布局（SpaceBetween + distributed table）”层已经基本正确，但在“列选择语义一致性”上仍未收敛，导致 Models / Agents 出现行为漂移。
1. 关键问题：仍然存在 subset-selection 行为
当前 responsive_table_layout 依旧是 greedy subset fitting：
- optional columns 可以跳过
- 后续低优先级列可能越过高优先级列被选中
这会导致：
- Input / Provider / Source 出现顺序不稳定
- 违反 strict priority prefix 语义
建议必须明确收敛为：
- strict prefix selection
- optional 遇到第一个放不下即 break
- 不允许 skip-block 继续尝试后续 optional
2. Agents tab 仍未统一 schema
Agents 当前仍保留三段式分支（very_narrow / narrow / wide）：
- narrow 行为仍是硬编码列集合
- Tokens / Cost 优先级仍被分支覆盖
这会导致：
- Agents 与 Models / Detail 行为不一致
- 同一 layout engine 不同语义
建议：
- 删除 is_narrow / is_very_narrow 分支
- 统一 schema-driven layout
- Tokens 作为 required，Cost 为 highest optional
3. Models tab 行为仍存在“非前缀扩展”现象
当前 observed behavior：
- Input / Provider / Source 出现顺序非严格 prefix
- 某些低优先级列可越过高优先级列出现
这说明 selection semantics 仍未统一
设计判断（重要）
当前 PR 仍处于混合模型状态：
✔ layout（SpaceBetween / full width）是正确的
❌ selection（列选择逻辑）仍是 knapsack-like subset model
两者不等价
建议收敛方向
必须统一成单一语义：
- strict priority prefix selection（核心）
- optional columns 按 priority 顺序尝试
- 遇到第一个失败 optional → break
- 不允许 skip-block selection
这样才能保证：
- Models / Agents / Detail 行为一致
- column order 可预测
- UI 不出现跨优先级跳变
结论
PR 目前不建议合入。
需要先统一 selection semantics，再谈 layout polish。
makoMakoGo
force-pushed
the
codex/tui-responsive-table-layout
branch
from
June 27, 2026 07:13
8ef9fb1 to
aa3f2c6
Compare
makoMakoGo
marked this pull request as ready for review
June 27, 2026 07:13
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 `@crates/tokscale-cli/src/tui/ui/period.rs`:
- Around line 180-196: The current responsive setup in PeriodColumn makes the
required set too wide on narrow screens because Period is atomic required and
Cost is also required; update the layout in the period responsive column
definitions to let Period fall back to its minimum readable width when the
required set does not fit, and make Cost an optional lower-priority column so
Period + Total can still fit first. Also adjust the narrow-screen test
assertions around the Period table so they no longer treat Cost as a core
required column, using the existing ResponsiveColumn and PeriodColumn symbols to
locate the affected logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6074422a-8466-4fa2-b8df-7dbc08f4dae6
📒 Files selected for processing (7)
crates/tokscale-cli/src/tui/ui/agents.rscrates/tokscale-cli/src/tui/ui/daily.rscrates/tokscale-cli/src/tui/ui/hourly.rscrates/tokscale-cli/src/tui/ui/model_usage_layout.rscrates/tokscale-cli/src/tui/ui/models.rscrates/tokscale-cli/src/tui/ui/period.rscrates/tokscale-cli/src/tui/ui/table_layout.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/tokscale-cli/src/tui/ui/agents.rs
- crates/tokscale-cli/src/tui/ui/model_usage_layout.rs
- crates/tokscale-cli/src/tui/ui/daily.rs
There was a problem hiding this comment.
No issues found across 7 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
makoMakoGo
force-pushed
the
codex/tui-responsive-table-layout
branch
2 times, most recently
from
June 27, 2026 09:35
fcf41d6 to
7b67e9f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/tokscale-cli/src/tui/ui/period.rs (2)
187-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win避免窄屏时 required 列本身超出表格宽度。
Period是 atomic required，Cost也是 required；当前窄屏测试仍把宽度 30 的布局固定为19 + 10 + 10，这会在加上列间距前就超过table_width。让Period可收缩到最小可读宽度，并把Cost改成最高优先级 optional，才能保住Period + Total的核心列集。建议修复
- ResponsiveColumn::measured_atomic_required( + ResponsiveColumn::measured_required( PeriodColumn::Period, period_column_order(PeriodColumn::Period), PERIOD_MIN_WIDTH, period_content_width, PERIOD_MAX_WIDTH, @@ - ResponsiveColumn::fixed_required( + ResponsiveColumn::fixed_optional( PeriodColumn::Cost, + 5, period_column_order(PeriodColumn::Cost), COST_WIDTH, ),同时更新 Line 946-955 的窄屏断言，不要继续把
Cost当作核心 required 列。Also applies to: 944-955
🤖 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/ui/period.rs` around lines 187 - 203, The narrow-screen layout in PeriodColumn is still treating Cost as required, which can make the fixed required columns exceed the table width. Update the ResponsiveColumn setup in period.rs so Period remains atomic but can shrink to its minimum readable width, and change Cost from fixed_required to the highest-priority optional column; then adjust the narrow-screen assertions in the related test block so they no longer expect Cost to be part of the core required set.
591-594: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win给
Provider单元格补回 muted 样式。
Source仍是 muted，但Provider现在使用默认前景色，Detail 上下文列的视觉层级不一致。建议修复
PeriodDetailColumn::Provider => Cell::from(truncate_display_width( &get_provider_display_name(&row.provider), table_layout.width_for(PeriodDetailColumn::Provider), - )), + )) + .style(Style::default().fg(theme_muted)),🤖 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/ui/period.rs` around lines 591 - 594, The PeriodDetailColumn::Provider cell lost its muted styling, so the Detail table now renders Provider with the default foreground while Source remains muted. Update the Provider match arm in the period UI rendering logic to apply the same muted style used by the Source cell, keeping the visual hierarchy consistent across context columns.
🤖 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.
Duplicate comments:
In `@crates/tokscale-cli/src/tui/ui/period.rs`:
- Around line 187-203: The narrow-screen layout in PeriodColumn is still
treating Cost as required, which can make the fixed required columns exceed the
table width. Update the ResponsiveColumn setup in period.rs so Period remains
atomic but can shrink to its minimum readable width, and change Cost from
fixed_required to the highest-priority optional column; then adjust the
narrow-screen assertions in the related test block so they no longer expect Cost
to be part of the core required set.
- Around line 591-594: The PeriodDetailColumn::Provider cell lost its muted
styling, so the Detail table now renders Provider with the default foreground
while Source remains muted. Update the Provider match arm in the period UI
rendering logic to apply the same muted style used by the Source cell, keeping
the visual hierarchy consistent across context columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea93c1b1-0a3b-4f92-b1aa-4f7ca9452ae7
📒 Files selected for processing (7)
crates/tokscale-cli/src/tui/ui/agents.rscrates/tokscale-cli/src/tui/ui/daily.rscrates/tokscale-cli/src/tui/ui/hourly.rscrates/tokscale-cli/src/tui/ui/model_usage_layout.rscrates/tokscale-cli/src/tui/ui/models.rscrates/tokscale-cli/src/tui/ui/period.rscrates/tokscale-cli/src/tui/ui/table_layout.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/tokscale-cli/src/tui/ui/hourly.rs
- crates/tokscale-cli/src/tui/ui/table_layout.rs
- crates/tokscale-cli/src/tui/ui/models.rs
- crates/tokscale-cli/src/tui/ui/model_usage_layout.rs
- crates/tokscale-cli/src/tui/ui/agents.rs
- crates/tokscale-cli/src/tui/ui/daily.rs
Owner
Author
|
/juya review |
![juya-review-bot[bot]](https://avatars.githubusercontent.com/in/4074416?s=60&v=4){kind=small}
There was a problem hiding this comment.
OpenCodeReview found 1 issue(s).
1 issue(s) could not be attached inline and are summarized below.
crates/tokscale-cli/src/tui/ui/table_layout.rs
This O(W × N) loop decrements widths by 1 each iteration, where W is the total surplus width that needs to be removed. For typical terminal widths (up to ~200+), this means hundreds of iterations, each scanning all columns. Since the goal is to reduce total width to table_width, this can be done in O(N) by computing the required reduction and distributing it proportionally across columns that still have shrinkable surplus.
For example: compute the total surplus (spaced_width(&widths) - table_width), then repeatedly subtract in proportionally largest surpluses — or use a single-pass fair-distribution algorithm. This avoids hundreds of redundant iterations during every TUI render.
Suggested change
Before:
while spaced_width(&widths) > table_width {
let Some(index) = columns
.iter()
.enumerate()
.filter(|(index, column)| widths[*index] > column.min_width)
.max_by_key(|(index, column)| widths[*index] - column.min_width)
.map(|(index, _)| index)
else {
break;
};
widths[index] -= 1;
}
After:
// Compute surplus once, then distribute the reduction proportionally
// across columns that still have shrinkable headroom.
let surplus = spaced_width(&widths).saturating_sub(table_width);
let mut remaining = surplus;
while remaining > 0 {
let shrinkable: Vec<usize> = columns
.iter()
.enumerate()
.filter(|(i, c)| widths[*i] > c.min_width)
.map(|(i, _)| i)
.collect();
if shrinkable.is_empty() {
break;
}
let max_headroom = shrinkable
.iter()
.map(|&i| widths[i] - columns[i].min_width)
.max()
.unwrap_or(0);
// Reduce by the largest headroom in one step, distributed across columns
let step = remaining.min(max_headroom);
for &i in &shrinkable {
let headroom = widths[i] - columns[i].min_width;
let reduction = step.min(headroom);
widths[i] -= reduction;
remaining = remaining.saturating_sub(reduction);
}
}
makoMakoGo
force-pushed
the
codex/tui-responsive-table-layout
branch
from
June 27, 2026 11:50
7b67e9f to
96ec1a4
Compare
Owner
Author
|
Review follow-up:
Validation:
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Flex::SpaceBetweenselect_widthto responsive table columns so optional columns cannot force required columns below their readable widthModelandTotal/Tokensthe only required model usage columns across Models, Workspace Models, and Detail schemasAgentandTokensrequired, andCostas the highest-priority optional columnRoot Cause
The responsive layout rewrite selected optional columns using
min_width, then tried to grow required measured columns afterward. Since identity columns had small minimum widths, wide optional columns could be selected while the identity column was left truncated. A later revision protected required columns withselect_width, but still used skip-blocked optional selection, which let lower-priority small columns appear before higher-priority wide columns. That changed the original priority-prefix layout into a capacity-fitting subset layout.Notes
select_widthis_very_narrow/is_narrowhard-coded column setsFlex::Startlayout is restoredValidation
cargo fmt --checkcargo test -p tokscale-clicargo build -p tokscale-cli --release/home/travis/.local/bin/tokscale --version/home/travis/.local/bin/tokscale --no-spinner --helpSummary by CodeRabbit