Conversation
📝 WalkthroughWalkthroughThe changes introduce a new viewport dumping capability to the application by adding a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a Key changes:
Minor observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Key binding triggers\nwrite_viewport_file action] --> B[performBindingAction\nin Surface.zig]
B --> C[writeScreenFile\nloc = .viewport]
C --> D[Acquire renderer_state mutex]
D --> E[pages.getTopLeft .viewport]
D --> F[pages.getBottomRight .viewport]
E --> G[terminal.Selection.init\ntopLeft, bottomRight, false]
F --> G
G -->|sel is null\nnever happens for viewport| H[Cleanup tmp_dir\nreturn early]
G -->|sel is non-null| I[ScreenFormatter\nemit = plain / vt / html]
I --> J[Write to tmp file\nviewport.txt or viewport.html]
J --> K{WriteScreen.Action}
K -->|copy| L[Copy file path\nto clipboard]
K -->|paste| M[Paste file path\ninto terminal]
K -->|open| N[Open file in\ndefault OS editor]
Last reviewed commit: 3058878 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .viewport => viewport: { | ||
| break :viewport terminal.Selection.init( | ||
| pages.getTopLeft(.viewport), | ||
| pages.getBottomRight(.viewport) orelse | ||
| break :viewport null, | ||
| false, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
orelse branch is dead code for .viewport
getBottomRight(.viewport) is guaranteed to return a non-null Pin — its implementation unconditionally computes a pin via br.down(self.rows - 1).? (which itself asserts non-null internally) and then returns it. This means orelse break :viewport null will never be taken.
This is consistent with how .screen is handled (also using orelse defensively), so it's not a correctness issue, but it is worth noting that unlike .history — where the null return is possible when the history region is empty — the viewport region always exists in a live terminal. A comment clarifying this behavior (similar to the one on .history about alternate screens) would improve readability.
| .jump_to_prompt, | ||
| .write_scrollback_file, | ||
| .write_screen_file, | ||
| .write_viewport_file, |
There was a problem hiding this comment.
No parsing test added for write_viewport_file
The sibling actions write_screen_file and write_scrollback_file both have dedicated parsing tests (around line 4503) that exercise both the shorthand form (e.g. write_screen_file:copy) and the extended form (e.g. write_screen_file:copy,html). There are no corresponding tests for write_viewport_file.
Since parsing is shared via the WriteScreen type, the behaviour is almost certainly correct, but adding test coverage would be consistent with the existing test patterns and would guard against future regressions, e.g.:
{
const binding = try parseSingle("a=write_viewport_file:copy");
try testing.expect(binding.action == .write_viewport_file);
try testing.expectEqual(Action.WriteScreen.copy, binding.action.write_viewport_file);
}
{
const binding = try parseSingle("a=write_viewport_file:copy,vt");
try testing.expect(binding.action == .write_viewport_file);
try testing.expectEqual(Action.WriteScreen{
.action = .copy,
.emit = .vt,
}, binding.action.write_viewport_file);
}Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/input/command.zig (1)
342-408: Add a focused regression test for the new viewport export commands.This matrix is all hand-written, and the current
command defaultstest only smoke-tests the aggregate arrays. A small assertion aroundactionCommands(.write_viewport_file)for the plain/HTML/VT variants would make copy/paste drift between these export tables much easier to catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/input/command.zig` around lines 342 - 408, Add a focused regression test that asserts the specific entries returned by actionCommands(.write_viewport_file) include the nine expected variants (three actions: copy/paste/open × three emits: default/plain, .html, .vt) and their titles/descriptions (or at least their .action/.emit discriminants) to prevent silent drift; update the existing "command defaults" test suite by adding a new test case that calls actionCommands(.write_viewport_file) and verifies each returned command has the correct .action (.copy/.paste/.open) and .emit (absent/default, .html, .vt) combinations in the same order as the matrix shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/input/command.zig`:
- Around line 342-408: Add a focused regression test that asserts the specific
entries returned by actionCommands(.write_viewport_file) include the nine
expected variants (three actions: copy/paste/open × three emits: default/plain,
.html, .vt) and their titles/descriptions (or at least their .action/.emit
discriminants) to prevent silent drift; update the existing "command defaults"
test suite by adding a new test case that calls
actionCommands(.write_viewport_file) and verifies each returned command has the
correct .action (.copy/.paste/.open) and .emit (absent/default, .html, .vt)
combinations in the same order as the matrix shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 610d9614-648f-4a62-b609-11352ead97ff
📒 Files selected for processing (3)
src/Surface.zigsrc/input/Binding.zigsrc/input/command.zig
1 participant
Summary
Testing
zig build -Demit-xcframework=true -Dxcframework-target=universal -Doptimize=ReleaseFast -Dversion-string=1.3.0-devSummary by cubic
Adds a viewport-only export action that can emit plain text, HTML, or VT/ANSI to a temp file, enabling ANSI preservation for visible-viewport reads. Addresses Linear issue ghostty-org#1073.
write_viewport_filebinding and command entries to export only the visible viewport; supports.copy,.paste, and.open.writeScreenFilewith a.viewportselection using visible bounds; full-screen and scrollback exports are unchanged.Written for commit 3058878. Summary will update on new commits.
Summary by CodeRabbit