feat(tui): land the Tideline Settings shell on typed row facts and one taxonomy - #5730
Conversation
…e taxonomy Replace the raw ConfigView table with the Tideline settings shell: a category rail, the setting/action list, and a detail pane at >=100 columns; a horizontally windowed category strip over a full-width list below that (design `.settings-nav` rule), so 80x24 and 40x12 keep the active category, Search, list rows, and per-row affordances. Truth: every row carries typed `ConfigRowFacts` (setting / action / diagnostic, authority, apply semantics, category override, App-observed effective value, snapshot lane, activation command) decided at construction. Saved rows fill saved/startup from the persisted value and leave current/effective unobserved unless an explicit App field or the UiSnapshot supplies it; action and diagnostic rows are not persisted facts. `SettingApplySemantics` gains `ReloadRequired` (mcp_config_path) and `UiNowEngineRestart` (workspace_follow_symlinks); default_mode, launch_screen, and session_auto_resume are next-session. The detail pane paints current, saved, startup, source, scope, apply, kind, and an honestly unobserved availability lane. One taxonomy: `ConfigCategory::ALL` (Appearance, Models & providers, Pod, Work, Tools & MCP, Trust, Motion, Advanced) drives ConfigView and the Tideline stage scaffold through shared rail/strip painters; the old Composer/Notifications/Keybindings/Privacy scaffold list is gone and the settings goldens are re-blessed. Settings opens on Appearance; raw endpoint, credential receipt, context diagnostic, timeout, and routing rows live under Advanced. Interaction: list rows and category chips store exact `Rect` hitboxes and require `Rect::contains`, so rail, divider, detail, status, and footer clicks never select or activate a row. Every row paints a toggle/choose/edit/open/read-only affordance, read-only rows are dimmed, and the selected row states "Enter or click again: <verb>". Search indexes localized category labels and a category click clears the filter. Localization: 40 new `MessageId`s for the category names, fact labels, sources, apply states, editor kinds, notes, navigation hint, and activation copy, translated in all fifteen packs; no shell chrome is hard-coded English. Tests cover truthful lanes, apply semantics, action rows, Rect exclusion, shared taxonomy and strip windowing, localized rendering, category search/click, affordances at the blocker sizes, short-height Advanced reachability, default Appearance, and raw-row routing. An interaction-evidence test drives the real view by keys and pointer at 40x12, 80x24, 100x30, and 120x32 and prints the rendered buffers under --nocapture; it caught and fixed the 40x12 status row being shed (so the activation copy never showed), a section caption over-charged one line at the top of the window, the scope badge crowding the value below 60 columns, and an unscrolled list overflowing its last cell. Review corrections (NO-GO pass): - A settings.toml or config.toml that fails to load no longer becomes defaults labelled saved/startup: `Settings::load_persisted` carries the parse failure in `load_error`, every row backed by the failed store is read-only, reads "(unavailable)", and its saved/startup lanes report "unavailable: <error>". Live App lanes still render. - Theme and locale take their current lane from App (`theme_id`, `ui_locale`). `settings::detect_low_motion_override` names the runtime overlay that forces low motion (NO_ANIMATIONS, VS Code, Termius, SSH, Tabby, legacy console) in the same precedence as `apply_env_overrides`; the motion rows then report `SettingAuthority::Environment` / `Terminal` with the concrete token. - The category strip's ‹ / › overflow markers are hitboxes, so every category is reachable by pointer alone at 40 columns. - Choice and text editors paint clickable `[ Apply ] [ Cancel ]` controls with exact Rect hitboxes; choice rows are Rect hitboxes too; Enter/Esc and the controls share `commit_edit` / `cancel_edit`. The compact editor merges scope and current onto one row so choices and controls both fit at 40x12. - Second-click activation is disarmed by every keyboard step, filter change, focus rebuild, and editor entry/exit; transient status clears on navigation. - The full reachable surface is localized: search label, Choose label, choice footers, Apply/Cancel, choice labels and details, every setting hint, the default description, load-error and override sources — 112 new ids across all fifteen packs. The unreachable per-key description table is removed in favour of the localized hints. - Settings goldens are stored right-trimmed with no trailing blank rows. - PTY receipts (tmux, SGR mouse) at 40x12, 80x24, and 120x32 exercised /settings, →/→, chip and › clicks, the choice editor, and a pointer Cancel against the built binary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pm3rhiLxu2Zbgr64CrqWW8 Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @Hmbown's task in 3m 11s —— View job Review: Tideline Settings shell (typed row facts + one taxonomy)Todo list
SummaryThis is a large but coherent, mechanical-in-the-good-sense refactor: it moves the settings surface onto typed Correctness
Provider/model/route safety (EPIC #2608) — No concerns. The Reuse/simplification — Good direction: Tests — Strong coverage: new unit tests for the motion-override detector, broken-store unavailability, the new apply-semantics variants, and a new Security — No secret handling, shell/exec, or input-validation changes in this diff. Note on verification: this sandbox's Bash tool required interactive approval for every command beyond read-only No blocking issues found. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 potential issues.
🐛 2 issues in files not directly in the diff
🐛 Motion edits can erase malformed settings
With a motion override active, authority hides a settings-file parse failure and leaves both motion rows editable. Editing either can overwrite the malformed file from defaults.
⚠️ Missing settings appear explicitly saved
When a key is absent, saved still receives its deserialized default. The settings view falsely presents an implicit default as an explicit saved choice.
| /// Goldens are stored without cell padding: every row is right-trimmed and | ||
| /// trailing empty rows are dropped, so `git diff --check` stays clean. | ||
| fn trim_rows(text: &str) -> String { | ||
| let mut rows: Vec<&str> = text.lines().map(str::trim_end).collect(); | ||
| while rows.last().is_some_and(|row| row.is_empty()) { | ||
| rows.pop(); | ||
| } | ||
| let mut out = rows.join("\n"); | ||
| out.push('\n'); | ||
| out |
There was a problem hiding this comment.
🔍 Golden checks lose screen height
trim_rows drops trailing rows before comparison, so the 80×24 fixture now has 17 lines. Bottom-of-screen rendering regressions can escape this golden.
Was this helpful? React with 👍 or 👎 to provide feedback.
…folding The four new #[allow(dead_code)] items are deliberate projection-layer scaffolding (typed Setting facts) composed by the landing slice that follows this one. Visible raise per the gate's own procedure, justified in the PR body. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 3m 15s —— View job Review: Tideline Settings shell (typed row facts + one taxonomy)Todo list
SummaryI read the diff against Confirmed: motion-override edits can silently wipe a malformed
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 771cbc62c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let error = match row.facts.authority { | ||
| SettingAuthority::UserSettings => settings_error.as_deref(), | ||
| SettingAuthority::WorkspaceConfiguration => config_error.as_deref(), | ||
| _ => None, |
There was a problem hiding this comment.
When settings.toml is malformed or unreadable while a motion override such as NO_ANIMATIONS, VS Code, or SSH is active, motion_facts changes the low_motion and fancy_animations rows' authority to Environment/Terminal, so this match ignores settings_error. Those rows consequently remain editable and present default values as saved; committing either edit runs Settings::transact, which loads a parse failure as defaults and then serializes them, overwriting the user's file. Track the backing store independently from the effective authority when deciding whether to disable a row.
Useful? React with 👍 / 👎.
| let saved = row | ||
| .facts | ||
| .store_error | ||
| .is_none() | ||
| .then(|| self.saved_display_value(row)); |
There was a problem hiding this comment.
On a fresh install, or whenever a valid settings.toml/config.toml omits a particular key, deserialization fills row.value from defaults, and this expression nevertheless creates saved: Some(default) and copies it into startup. The new detail pane therefore claims values such as the default theme or an unset context window were read from disk even though SettingFact::saved is documented as the exact persisted value. Track per-key presence from the parsed documents and leave the saved lane unobserved when the key is absent.
Useful? React with 👍 / 👎.
| facts: ConfigRowFacts::saved_setting() | ||
| .authority(SettingAuthority::WorkspaceConfiguration) | ||
| .apply(SettingApplySemantics::ReloadRequired), |
There was a problem hiding this comment.
When CODEWHALE_MCP_CONFIG or DEEPSEEK_MCP_CONFIG is set, startup applies that environment value after loading config.toml, and app.mcp_config_path consequently contains the environment-owned path. This row still labels the source as workspace configuration, treats the effective path as saved/startup state, and remains editable; saving writes a root config value that the environment overrides again on the next launch. Detect this environment owner and separate the live value from the persisted root value, making the row read-only or clearly reporting the override.
Useful? React with 👍 / 👎.
| facts: motion_facts( | ||
| ConfigRowFacts::saved_setting() | ||
| .category(ConfigCategory::Motion) | ||
| .effective(app.fancy_animations.to_string()), |
There was a problem hiding this comment.
When both saved switches are true, MotionPolicy::from_settings resolves the session to Reduced mode because low_motion wins, but this row records the raw app.fancy_animations flag as the effective value. The detail pane and list consequently report fancy animations as currently on even though all such motion is disabled. Derive the effective lane through the motion policy—or retain the raw flag only as current and report the resolved result separately.
AGENTS.md reference: crates/tui/AGENTS.md:L15-L16
Useful? React with 👍 / 👎.
| let value_width = width.saturating_sub(10); | ||
| let fact_line = |name: MessageId, value: &str| { | ||
| Line::from(vec![ | ||
| Span::styled(format!("{:<10}", self.tr(name)), muted), |
There was a problem hiding this comment.
For CJK and combining-script locales, Rust's format width does not represent terminal display cells: for example, the two-character Chinese 当前 is four cells and then receives eight spaces here. The value is still truncated against width - 10, so each fact line exceeds the detail pane and wraps, consuming extra rows and hiding later facts or activation help on shorter wide terminals. Measure and pad the translated label with the existing terminal display-width helpers.
Useful? React with 👍 / 👎.
| }, | ||
| ConfigRow { | ||
| section: ConfigSection::Composer, | ||
| key: "bracketed_paste".to_string(), | ||
| value: settings.bracketed_paste.to_string(), | ||
| editable: true, | ||
| scope: ConfigScope::Saved, | ||
| facts: ConfigRowFacts::saved_setting(), |
There was a problem hiding this comment.
Changing this row through Settings only updates app.use_bracketed_paste; the terminal mode was enabled from the startup-captured TuiOptions::use_bracketed_paste, and no EnableBracketedPaste or DisableBracketedPaste sequence is emitted after this edit. The row nevertheless inherits Immediate, so users who disable bracketed paste as a terminal-compatibility workaround are told it applied while the terminal continues sending paste events until restart. Either reconfigure the terminal mode during the update or assign next-session semantics here.
Useful? React with 👍 / 👎.
| let description = if hint.is_empty() { | ||
| self.tr(MessageId::ConfigDescriptionDefault) | ||
| } else { | ||
| hint |
There was a problem hiding this comment.
For any action or diagnostic row without a key-specific hint—such as the dynamically keyed external-credential receipts or effective_auto_compact—this fallback says that Enter changes the value and that a scope badge is present. Those rows are read-only, suppress the scope badge, and ignore Enter, so the detail pane directly contradicts both the affordance and the following activation line. Choose fallback copy based on ConfigRowKind/SettingKind rather than applying the editable-setting instructions to every row.
Useful? React with 👍 / 👎.
| vec![tab_line, search_line] | ||
| } | ||
| SettingKind::Choice => "‹ ›", | ||
| SettingKind::Integer | SettingKind::Text => "✎", |
There was a problem hiding this comment.
With CODEWHALE_ASCII_SAFE=1, the backend replaces only glyphs registered in glyphs::ascii_fallback; ✎ has no entry there. Integer and text settings therefore still emit this Unicode pencil in the ASCII-safe Settings shell, unlike the other new affordances and navigation marks. Route this mark through the shared fallback mechanism or use an ASCII affordance when the compatibility tier is active.
Useful? React with 👍 / 👎.
…s authority Windows CI caught a real cross-platform bug in the Tideline settings shell: the broken-store marker matched on the row's resolved authority, so when the motion-override probe (legacy console host on Windows runners) relabeled the low_motion/fancy_animations rows to Terminal authority, a broken settings.toml produced a synthesized saved lane instead of the unavailable lane — different truth per host. Authority and store are different facts: the override wins the effective decision; the store is still broken. ConfigRowFacts gains a typed SettingStore (UserSettings / WorkspaceConfig / None), constructors and the authority builder set it, overrides keep it, and the marking loop keys on it. Tests: config_detail_never_synthesizes_current_from_saved pins its synthetic row authority (host-probe independent); new regression test overridden_motion_row_still_reports_a_broken_settings_store fails pre-fix (store_error missing under NO_ANIMATIONS override) and passes post-fix. Verified locally: 4/4 affected tests, 74/74 tideline family, fmt clean. Windows proof: this PR's CI. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 2m 23s —— View job Review: Tideline Settings shell (typed row facts + one taxonomy)Todo list
SummaryThis push ( The P1 fix is correct.
Still open (not addressed in this push, not blocking): the P2 items from the earlier Codex/Devin passes — implicit-default-presented-as-saved ( Provider/model/route safety (EPIC #2608) — No changes in this push touch provider/model selection; still no prefix-based inference anywhere in the diff. Dead-code budget — Note on verification: this sandbox required interactive approval for every Bash command beyond read-only No new blocking issues. The previously-reported blocking bug is fixed with a real regression test. Branch: |
1 participant
Lands the Tideline Settings shell: typed row facts (
SettingFact/SettingAuthority/SettingApplySemantics), one settings taxonomy, and the first visible Tideline surface. Branch was stacked on #5724 (now merged); main merged in, so CI runs on the truthful post-#5724 baseline.This lane also carries
crates/tui/src/tui/tideline.rs(UiSnapshot,ContextBudgetSnapshot,InteractionRegistry) — the typed-projection embryo the core/protocol extraction spec (Ops: CORE-PROTOCOL-EXTRACTION-SPEC-20260830) promotes intocodewhale-protocolas its Phase A3 prerequisite.Refs: Tideline acceptance matrix qa/tideline-acceptance-20260828/ACCEPTANCE_MATRIX.md; takeover prompt Tideline contract.
No-Issue: Tideline program slice tracked on the v0.9.12 milestone tracker #5573 and the Ops takeover plan.
Dead-code budget: raised 444 → 448 via scripts/check-dead-code-budget.py --update. Justification: the four new allows are deliberate stage scaffolding in the tideline.rs projection layer (
Setting*typed facts composed by the landing slice that follows), same pattern as the 372→377 raise on #5698.