fix(cli): preserve terminal contrast across light and dark themes - #729
fix(cli): preserve terminal contrast across light and dark themes#729loveRhythm1990 wants to merge 7 commits into
Conversation
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 9653874. No blocking correctness or regression findings.
The fix addresses the cause described in #728 at the shared rendering boundary. Removing the forced foreground colors lets the terminal choose readable text colors while retaining bold, italic, and heading emphasis. I inspected the locked termimad 0.23.2 implementation: its default skin provides those attributes without foreground overrides for these elements, and no_style removes the code/table styling as well. I also verified the separate crossterm versions: the renderer's 0.23.2 dependency does not implement the NO_COLOR handling present in the CLI's 0.29 dependency, so handling this explicitly in the renderer is justified.
I traced chat --no-color through process-environment initialization and checked both incremental rendering and the server-admission final-text caller. The presence-based NO_COLOR behavior, including an empty value, is consistent with the existing CLI theme/rendering checks. The change stays in the canonical renderer and does not introduce a terminal-specific theme detector or another rendering path.
The tests exercise useful observable behavior: vt100 cell foreground/background and emphasis for ordinary text, list labels, and headings; content preservation and absence of style escapes across code, quotes, lists, and tables; and subprocess-isolated environment cases that verify the selected test actually ran. Streaming cursor control remains outside the documented no-style contract.
Independent validation:
- Compiled the actual streaming_md, terminal_region, and output_sink modules in an isolated Cargo test harness, using the repository's test stdout macro and pinned direct dependencies: all 47 tests passed.
- git diff --check against the PR base passed.
- Reviewed both changed files and the relevant callers/dependency implementation.
Limits: this was focused module validation, not a full Astra Cargo/workspace build or a live CLI/model session. I did not visually validate Ghostty or other native terminal applications. The combined-status API returned no status entries, so this review does not claim that all CI checks are green.
Merge Queue Status
This pull request spent 18 minutes 4 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #729 has been dequeued Queue conditions are not satisfied:
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
…inal' into investigate/issue-728-light-terminal
loveRhythm1990
changed the title
Merge Queue Status
This pull request spent 2 minutes 46 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
2 participants
Summary
Several CLI/TUI surfaces force light foregrounds or choose a dark panel background while inheriting the terminal's foreground. On a light terminal, this makes Markdown labels, welcome text, login input, and user messages difficult to read.
Use terminal-default foregrounds for ordinary text and emphasis, reuse the existing TUI palette for welcome accents and syntax highlighting, and pair conversation-panel foregrounds with their backgrounds. Auto mode now preserves terminal defaults when background information is unavailable instead of assuming dark. Keep explicit light/dark profiles and make plain/NO_COLOR paths consistent in the affected renderers.
Related issue
Closes #728
Change type
User and compatibility impact
/agentand/messaginglabels, and turn-summary values no longer force white or selection-only foregrounds on the terminal background.ASTRA_TUI_THEME=lightordark. This intentionally changes the former dark fallback.astra chat --no-color/NO_COLORdisable Markdown styling, including code/table styling. Streaming cursor control remains available.Architecture and complexity delta
tui/theme.rspalette andtui/style.rsconversation surfaces;streaming_md.rsremains the CLI Markdown owner. The welcome renderer reuses the TUI palette and existing crossterm colour conversion through crate-local exports.Verification
rtk proxy make format-check— passed.cargo nextest run -p astra-cli --lib --offline -E 'test(tui::history_cell::user::tests)'— all 9 tests passed. The full-width user-message rendering test covers auto without background hints, light, dark, plain, and NO_COLOR in isolated processes, including foreground, full-row background, and breathing rows.rtk git diff --check github/main...HEAD— passed.cargo check -p astra-cli --tests --offline— passed before the dependency-only main merge.cargo clippy -p astra-cli --tests -- -D warnings— could not complete after the main merge: the local build volume ran out of space (ENOSPC). An earlier type-complexity finding was corrected with a type alias; the CI static-check job passed at6cef21c7d.rustc --testharness — 72 tests passed using actual Markdown, terminal-region, palette, theme, surface, and highlighting modules. Extended UI harness — 89 tests passed, including actual login-rendering tests through a minimal bottom-pane interface stub and extracted welcome styling/tests. These are focused render tests, not a full Cargo test-suite run.astra,/login,/agent,/messaging, andastra chat --no-colorinto their render owners; exercised theme selection through isolated process environments, actual render functions, ratatui buffers, and vt100 cells. No live model session or manual Ghostty/other-terminal visual test was run.Final checklist
explained why no test is needed.
change needs no documentation update.
generated files, and other sensitive information.