refactor(ui): apply the remaining code-review structural cleanups - #359
Merged
forketyfork merged 2 commits intoAug 15, 2026
Merged
Conversation
Issue: The selection-agent-context review (P0/P1 already fixed in the base branch) also flagged five P2 structural issues: two runtime handlers half-adopting SpawnSessionContext instead of taking it directly, nine call sites manually pairing selection-menu invalidation with terminal layout changes instead of one structural choke point, two new UI actions carrying a raw session slot index instead of the stable-ID pattern the pending-send mechanism already uses, hand-rolled modal chrome duplicated between two dialog components, and a "resolve a Pin to a cell" branch duplicated four times. Solution: handleExternalSpawnRequest and handleLaunchAgentWithContext now take a single *const SpawnSessionContext built once per frame. applyTerminalLayout/applyTerminalLayoutIfSizeChanged now invalidate selection menus internally, so every call site gets it for free instead of relying on a paired call. OpenSelectionAgentAction and LaunchAgentWithContextAction now carry the source session's stable id, resolved back to a slot via findSessionIndexById() when handled, so a grid reindex between queuing and handling can't retarget the wrong terminal. A new ui/components/modal_frame.zig extracts the shared scrim+panel rendering and Escape/Cmd+W dismiss check used by confirm_dialog and selection_agent_overlay. session_interaction.zig gains a single pinToCoords() helper for resolving a Pin to (x, y) in whichever page is visible, replacing four inline copies of the same viewport/active branch.
There was a problem hiding this comment.
Pull request overview
Refactors selection-agent and modal infrastructure to centralize shared behavior while preserving existing functionality.
Changes:
- Uses stable session IDs and shared spawn/layout contexts.
- Extracts shared modal chrome, dismissal handling, and pin-coordinate resolution.
- Documents the new module and stable-ID convention.
Validation was not run; the listed manual tests remain pending.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/ui/types.zig |
Changes selection-agent actions to stable session IDs. |
src/ui/components/session_interaction.zig |
Captures stable IDs and centralizes pin resolution. |
src/ui/components/selection_agent_overlay.zig |
Uses stable IDs and shared modal helpers. |
src/ui/components/modal_frame.zig |
Adds shared modal rendering and dismissal utilities. |
src/ui/components/confirm_dialog.zig |
Adopts shared modal helpers. |
src/app/runtime.zig |
Centralizes spawn context and layout invalidation. |
docs/ARCHITECTURE.md |
Documents the structural conventions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…to refactor/selection-agent-followups # Conflicts: # docs/ARCHITECTURE.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Follow-up structural cleanup on top of the selection-agent-context feature (#358):
handleExternalSpawnRequestandhandleLaunchAgentWithContexteach still took ~15 flat parameters and rebuilt aSpawnSessionContextlocally instead of accepting the struct directly, even though the struct existed specifically to avoid that duplication.runtime.zigmanually pairedhideSelectionMenus()with a terminal-layout-changing call, relying on every future call site remembering to do the same.OpenSelectionAgentAction/LaunchAgentWithContextActioncarried a raw session slot index captured at queue time, rather than the stable-ID patternPendingSessionSendalready uses to survive grid reindexing.selection_agent_overlay.zighand-rolled the same scrim/panel/dismiss-key chrome asconfirm_dialog.zig, with no shared code to keep them from drifting.session_interaction.zighad four inline copies of "resolve a Pin to (x, y) honoringis_viewing_scrollback".Solution
run()now builds oneSpawnSessionContextper frame and both handlers take*const SpawnSessionContextdirectly.applyTerminalLayout/applyTerminalLayoutIfSizeChangednow callhideSelectionMenus()internally (unconditionally / only when the layout actually changed, matching prior call-site behavior), so invalidation is structural instead of a convention every caller has to remember.applyRuntimeResizeForScaleChange, which duplicated the same resize logic inline, now delegates toapplyTerminalLayoutso the window-resize path gets the same guarantee.OpenSelectionAgentAction/LaunchAgentWithContextActionnow carry the source session's stableid; handlers resolve it back to a slot index viafindSessionIndexById()at handling time.ui/components/modal_frame.zigexportsrenderScrimAndPanel()andisDismissKey(), adopted by bothconfirm_dialog.zigandselection_agent_overlay.zig.pinToCoords()helper insession_interaction.zigreplaces the four inlinepointFromPinbranches inselectionCellForPin,selectWord,selectLine, andgetLinkMatchAtPin.No behavior changes are intended — this is a pure structural cleanup.
docs/ARCHITECTURE.mdupdated for the newmodal_frame.zigmodule and the stable-ID convention on these two actions.Context
Base branch:
feat/selection-agent-context(#358).Test plan
hideSelectionMenus()(closing the last terminal, closing a mid-grid terminal, font size change, window resize, grid reflow) with an active text selection, and confirm the selection pill/menu disappears in every case.