Skip to content

fix(progress): suppress UI escape codes and duplicate lines in text mode#86

Merged
jdx merged 2 commits intomainfrom
fix-text-mode-rendering
Apr 30, 2026
Merged

fix(progress): suppress UI escape codes and duplicate lines in text mode#86
jdx merged 2 commits intomainfrom
fix-text-mode-rendering

Conversation

@jdx
Copy link
Copy Markdown
Owner

@jdx jdx commented Apr 30, 2026

Summary

Two problems made ProgressOutput::Text output (CI logs, piped output) ugly:

  1. UI escape codes leaking into text-mode output. set_status(Done|Failed|Warn|DoneCustom) and stop() both call refresh_once() to force a synchronous render. refresh_once() did not check the output mode, so it always took the UI path through write_frame() — emitting ESC[NA, ESC[ND, ESC[0J cursor-control sequences. In a non-TTY log they show up as raw [9A[80D[0J text. Now refresh_once() returns early in ProgressOutput::Text; each update() already streamed the relevant line, so no information is lost.

  2. Same line printed multiple times. Updating multiple props on a single job in quick succession (e.g. prop("message", ...) followed immediately by an explicit update(), common when integrators set several fields per state transition) emitted the same rendered line repeatedly. Cache the last text-mode line per job (last_text_output: Mutex<Option<String>>) and skip writes that produce identical output.

Together these collapse a 30-line block of duplicates and escape codes into clean one-line-per-state-change text-mode logs. Triggered while debugging hk's CI output — see hk#890 for the before/after.

Test plan

  • cargo test --lib passes (116 tests, no regressions)
  • cargo build
  • Verified end-to-end via hk patch in CI

🤖 Generated with Claude Code


Note

Low Risk
Output-only changes scoped to progress rendering; main risk is unintentionally suppressing expected text-mode lines, mitigated by resetting the dedup cache on operation changes.

Overview
Improves ProgressOutput::Text (non-TTY/CI) output by making refresh_once() a no-op in text mode, preventing full-frame redraws that emit cursor-control escape codes.

Adds per-job text-mode output deduplication by caching the last rendered line (last_text_output) and skipping writes when repeated updates render the same string; the cache is cleared on next_operation() to ensure operation transitions still emit at least one line.

Reviewed by Cursor Bugbot for commit b92c083. Bugbot is set up for automated code reviews on this repo. Configure here.

Two problems made text-mode output (CI logs, piped output) ugly:

1. `set_status(Done|Failed|Warn|DoneCustom)` and `stop()` both call
   `refresh_once()` to force a synchronous render. `refresh_once()` did
   not check the output mode, so it always took the UI path through
   `write_frame()` — emitting ESC[NA, ESC[ND, ESC[0J cursor-control
   sequences that show up as raw `[9A[80D[0J` text in non-TTY logs.
   Make `refresh_once()` a no-op in `ProgressOutput::Text` since each
   `update()` already streamed the relevant line.

2. Updating multiple props on a single job in quick succession (e.g.
   `prop("message", ...)` followed immediately by `update()`, common when
   integrators set several fields per state transition) emitted the same
   rendered line repeatedly. Cache the last text-mode line per job and
   skip writes that produce identical output.

Together these collapse the duplicated/escape-littered CI output into
clean one-line-per-state-change text-mode logs.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a mechanism to suppress duplicate output in text-mode progress reporting, which helps reduce noise in CI logs. It introduces a last_text_output field to ProgressJob to track the most recently rendered line and updates render_text_mode to skip writing if the content hasn't changed. Additionally, refresh_once is now a no-op when using text output to avoid unnecessary redraws and escape sequences. I have no feedback to provide.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR fixes two text-mode (ProgressOutput::Text) rendering issues: refresh_once() now exits early in text mode to prevent cursor-movement escape codes from leaking into non-TTY logs, and render_text_mode caches the last emitted line per job to suppress consecutive identical writes. A previous comment about next_operation not resetting last_text_output was addressed in this revision.

Confidence Score: 5/5

Safe to merge — changes are scoped to text-mode rendering with no impact on UI/TTY paths.

No P0 or P1 issues found. The two fixes are logically correct and well-scoped; the previously flagged next_operation reset omission is addressed in this revision. Only a minor P2 observation about cache state after a failed write, which is an extremely unlikely edge case.

No files require special attention.

Important Files Changed

Filename Overview
src/progress/render.rs Adds early-return for Text mode in refresh_once() and per-job dedup cache in render_text_mode(); logic is sound, lock ordering is safe.
src/progress/job.rs Adds last_text_output: Mutex<Option<String>> field, initializes it to None in the builder, and resets it in next_operation() — correctly handling the case where a new operation renders identically to the last line of the previous one.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[job.update / prop / set_status] --> B{output mode?}
    B -- Text --> C[render_text_mode]
    C --> D{output empty?}
    D -- yes --> E[return Ok]
    D -- no --> F{identical to last_text_output?}
    F -- yes --> G[skip / return Ok]
    F -- no --> H[update last_text_output cache]
    H --> I[acquire TERM_LOCK]
    I --> J[term.write_line]
    B -- UI/Auto --> K[notify render loop]
    K --> L[write_frame with cursor codes]
    M[set_status Done/Failed/Warn] --> A
    M --> N[refresh_once]
    N --> O{Text or Quiet or disabled?}
    O -- yes --> P[no-op / return Ok]
    O -- no --> L
    Q[next_operation] --> R[reset progress/rate state]
    R --> S[clear last_text_output = None]
    S --> A
Loading

Fix All in Claude Code

Reviews (2): Last reviewed commit: "fix(progress): reset text-mode dedup cac..." | Re-trigger Greptile

next_operation already clears the per-operation progress values, rate
tracking, and operation start time. The text-mode dedup cache
(last_text_output) was missed: if the next operation's first render
happened to produce a byte-identical line (e.g. a body that only shows
message and message is unchanged), render_text_mode would suppress
that write, swallowing the operation transition from CI logs.

Clear last_text_output alongside the rest of the per-operation state
so the first render of every new operation always reaches the wire.
@jdx jdx merged commit 3dd2536 into main Apr 30, 2026
9 checks passed
@jdx jdx deleted the fix-text-mode-rendering branch April 30, 2026 15:06
jdx added a commit to jdx/hk that referenced this pull request Apr 30, 2026
## Summary

Cleans up hk's text-mode (CI / piped stderr) progress output. The
previous behavior was unreadable in GitHub Actions logs: raw
`[9A[80D[0J` cursor-control escapes leaked into the log, every status
change duplicated, failure stderr was suppressed, and a step matching
hundreds of files dumped ~4KB of paths into every progress line. After
this PR a 14-step pre-commit run reads as a clean append-only stream
with one full diagnostic block per failure.

## What changed

**Bumped clx 2.0 → 2.0.1**
([clx#86](jdx/clx#86))
- `refresh_once()` is now a no-op in `ProgressOutput::Text` so
terminal-state transitions and `stop()` don't leak UI escape codes
- `render_text_mode()` dedupes consecutive identical job lines per job
(multiple props updated in quick succession no longer print the same
line N times)
- `next_operation()` resets the dedup cache so multi-operation
transitions are never silently swallowed

**Failure summaries shown in text mode** ([src/hook.rs](src/hook.rs))
- `HookContext.failed_steps: Mutex<HashSet<String>>` tracks failures so
the end-of-run summary survives the `step_contexts.shift_remove` that
fires when each step finishes
- In text mode, the per-step output summary block is now emitted for
failed steps by default — successful steps stay quiet (their output
already streamed). `HK_SUMMARY_TEXT=1` still forces every step's summary
to print
- Docs updated for `HK_SUMMARY_TEXT` to reflect the new default behavior

**Bounded progress message rendering**
([src/step/runner.rs](src/step/runner.rs), [src/tera.rs](src/tera.rs),
[src/step_job.rs](src/step_job.rs))
- New `Context::for_display()` returns a tera context where `files` /
`workspace_files` are rewritten to `first_token …` when more than one
file matches. The runner renders twice — display version for the
progress message, full version for execution
- `truncate_progress_message()` caps the formatted progress message at
2048 printable chars with a trailing `…`, ANSI-aware so escape sequences
don't get split mid-cluster
- Dropped the `truncate_text` filter from `step_job.rs` body_text —
clx's filter clamped to (`term_width - 20`) which is `60` chars in
non-TTY environments, cutting exactly the diagnostic detail you need to
debug a CI failure

**Examples — `dbg` step matching 98 .rs files in CI:**
```
# before:
  dbg – 98 files – **/*.rs – ! rg -e 'dbg!' bin/generate_docs.rs build/generate_builtins.rs ... [continues for 98 paths] ...

# after:
  dbg – 98 files – **/*.rs – ! rg -e 'dbg!' bin/generate_docs.rs …
```

**Failed step in CI:**
```
# before (excerpt — full block was ~85 lines of escape-code-littered duplication):
[9A[80D[0J✔ files - Fetching all files in repo (671 files)
✗ demo-fail
✗ demo-fail
✗ demo-fail
✗ demo-fail – ERROR
[9A[80D[0J✔ files - Fetching all files in repo (671 files)
... (escape-code redraws repeat for every aborted sibling step)

# after:
  demo-fail – 98 files – **/*.rs – echo "..." (multi-line script)
  demo-fail – this step is intentionally failing so we can see what the output looks like
  demo-fail – line two of stderr
  demo-fail – line three of stdout
✗ demo-fail
✗ demo-fail – ERROR

demo-fail stderr:
this step is intentionally failing so we can see what the output looks like
line two of stderr
line three of stdout
```

## Test plan
- [x] `cargo test` — 149 pass (11 new across `src/tera.rs::tests` and
`src/step::runner::tests`)
- [x] `mise run test:bats test/git.bats` — 4 pass under both libgit2 and
nolibgit2 (assertions updated for `first_file …` display)
- [x] `mise run test:bats test/output_summary.bats
test/output_summary_no_duplicate.bats
test/output_summary_check_first.bats` — 11 pass
- [x] Verified end-to-end in CI via the (now-removed) `demo-fail`
scaffolding step — see runs
[25166877825](https://github.com/jdx/hk/actions/runs/25166877825)
(before) and
[25168039915](https://github.com/jdx/hk/actions/runs/25168039915)
(after)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant