Skip to content

fix(tui): show complete edit previews in details - #4722

Closed
nightt5879 wants to merge 3 commits into
Hmbown:mainfrom
nightt5879:nightt5879/issue-4659-edit-preview
Closed

fix(tui): show complete edit previews in details#4722
nightt5879 wants to merge 3 commits into
Hmbown:mainfrom
nightt5879:nightt5879/issue-4659-edit-preview

Conversation

@nightt5879

Copy link
Copy Markdown
Contributor

What

  • Keep the compact edit_file approval card bounded.
  • Build and render the complete localized -/+ search/replace preview lazily in the Alt+V details pager.
  • Add a regression test covering both sides beyond the previous three-line summary.

Why

The card preview intentionally stays compact, but the details pager previously fell back to raw JSON. That left multiline replacement boundaries difficult to review. Building the full preview only when the pager opens avoids adding unbounded work to the per-frame render path.

Verification

  • cargo fmt --all -- --check
  • cargo test -p codewhale-tui --bin codewhale-tui --locked edit_file — 21 passed
  • cargo test -p codewhale-tui --bin codewhale-tui --locked tui::approval::tests:: — 99 passed
  • cargo check -p codewhale-tui --all-targets --locked
  • cargo test --workspace --doc --locked
  • Workspace unit suite otherwise passed locally; isolated Windows baselines were echo being a shell built-in, PowerShell redirection producing UTF-16, and a user-home instruction fixture. The parallel-only external approval count failure passed in isolation.
  • The exact CI Clippy command reaches two unrelated Windows-only main-branch lints; upstream main's Linux Lint job is green.

Fixes #4659

Keep the compact edit_file approval card bounded, but build and render the complete search and replacement preview lazily in the Alt+V details pager. This gives reviewers a readable full replacement boundary without adding work to the per-frame render path.

Add regression coverage for search and replacement bodies beyond the three-line card summary.

Fixes Hmbown#4659

Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com>
@nightt5879
nightt5879 requested a review from Hmbown as a code owner July 23, 2026 03:08

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec75b9a1d0

ℹ️ 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".

Comment thread crates/tui/src/tui/approval.rs Outdated
Comment thread crates/tui/src/tui/approval.rs Outdated
Render Alt+V edit preview chunks with explicit escapes for spaces, tabs, quotes, backslashes, LF, and CRLF so PagerView wrapping cannot collapse source-significant whitespace.

Exercise the rendered pager path with mixed indentation and line endings.

Addresses Codex review on Hmbown#4722.

Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

@codex review

Follow-up commit a475b65 addresses both earlier P2 findings. Please re-review the updated head.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a475b65991

ℹ️ 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".

Comment thread crates/tui/src/tui/approval.rs Outdated
Derive the Alt+V exact edit preview headings from the active locale without changing the compact approval card path.

Add regression coverage for every shipped locale.

Addresses Codex review on Hmbown#4722.

Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

@codex review

Follow-up commit d180cb5 addresses the localization finding and keeps the compact card path unchanged. Please perform a final review of the updated head.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: d180cb571d

ℹ️ 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".

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: d180cb571d

ℹ️ 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".

Hmbown pushed a commit that referenced this pull request Jul 24, 2026
Render Alt+V edit preview chunks with explicit escapes for spaces, tabs, quotes, backslashes, LF, and CRLF so PagerView wrapping cannot collapse source-significant whitespace.

Exercise the rendered pager path with mixed indentation and line endings.

Addresses Codex review on #4722.

Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com>
(cherry picked from commit a475b65)
Hmbown pushed a commit that referenced this pull request Jul 24, 2026
Derive the Alt+V exact edit preview headings from the active locale without changing the compact approval card path.

Add regression coverage for every shipped locale.

Addresses Codex review on #4722.

Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com>
(cherry picked from commit d180cb5)
@Hmbown

Hmbown commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Thanks @nightt5879 — landed, and #4659 is closed.

This reached main across 3db71c781 (escape/boundary work, carrying your Signed-off-by and (cherry picked from commit a475b6599…)) and 4b12aa4b7 ("fix(tui): localize exact edit preview headers"), which picked up your follow-up head. Everything is present in crates/tui/src/tui/approval.rs: edit_file_preview_lines(params, 3) at :788, exact_edit_file_preview_lines at :821, exact_preview_body_lines at :831, and the details-pager injection at :1411. Both tests are there — edit_file_details_pager_includes_complete_search_replace_preview (:3105) and edit_file_details_pager_localizes_preview_headers_for_every_locale (:3154).

Not merging the branch itself is deliberate: the only remaining delta is main-side improvement your PR predates — selection now wraps via list_nav::wrap_index (#4755) where this branch still clamps with saturating_sub. Merging would regress that.

@Hmbown Hmbown closed this Jul 26, 2026
@nightt5879
nightt5879 deleted the nightt5879/issue-4659-edit-preview branch July 30, 2026 01:57
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.

edit_file 审批预览仅展示3行,多行改动无法完整审查

2 participants