GTK: present confirmation dialogs through quadraui's native seam, not the in-canvas Dialog primitive

[JDonaghy]

Summary

vimcode's "Unsaved Changes" confirmation is an in-canvas box painted by quadraui::Dialog; VS Code's equivalent is a real OS dialog. Once quadraui grows a native message-dialog seam (quadraui#666), route GTK's confirmation dialogs through it. TUI keeps the in-canvas primitive — that is the point of the split, not a limitation.

Depends on quadraui#666. Needs a src/quadraui_pin.rs bump; do not start until that has landed.

The edge-trigger problem — this is the actual work

vimcode's dialogs are frame-driven state. engine.dialog: Option<Dialog> (src/core/engine/mod.rs:363) is converted to a quadraui::Dialog by dialog_panel_to_quadraui_dialog (src/render.rs:4887) and repainted every frame via backend.draw_dialog (src/gtk/mod.rs:9333). A native dialog cannot be re-presented every frame — it must be presented once, on the transition from no-dialog to dialog.

So:

1. Present once, keyed on the dialog's identity, on the no-dialog to dialog edge.
2. While a native dialog is in flight, suppress the in-canvas draw.
3. Map the returned WidgetId back to exactly the action the in-canvas DialogHit::Button(id) path already produces at src/gtk/mod.rs:4399, so the engine sees one code path and one set of outcomes.

The blocking call shape is already established here — backend.services().show_file_open_dialog(...) at src/gtk/mod.rs:1773 blocks on quadraui's nested-mainloop pump, and PendingFileDialog (src/gtk/mod.rs:1089) already models "a dialog is in flight". Follow that, don't invent a second mechanism.

What stays in-canvas

• TUI, always. src/tui_main/** is untouched by this issue and its snapshots must not move.
• Any dialog quadraui reports as not natively expressible — i.e. carrying a table or a text input. That covers the help dialog (src/render.rs:8177 sc_help_dialog, a DialogTable), the text-input dialogs (src/render.rs:4915), and source control's (src/core/engine/source_control.rs:269). These must keep working exactly as today.

The native/in-canvas decision comes from quadraui's native_dialog_options returning None — do not hand-maintain a list of dialog kinds in this repo.

Acceptance

• Closing a modified buffer in the GTK build shows a real OS dialog. Save / Don't Save / Cancel produce the same engine outcomes as today's in-canvas buttons.
• The dialog is presented exactly once per open, not once per frame — assert the present count stays at 1 across N frames with the dialog unchanged.
• Both entry points go native: :q with unsaved changes, and the tab-x close path (Msg variants at src/gtk/mod.rs:1380-1388).
• TUI behaviour byte-identical.
• Help dialog and text-input dialogs still render in-canvas on GTK.

Files

• src/gtk/mod.rs — present-once edge trigger, response-to-action mapping, in-canvas suppression while native is in flight
• src/quadraui_pin.rs — pin bump
• src/render.rs — only if the descriptor must carry the native/in-canvas decision through

Supersedes

#209, option 3 ("native widget escape hatch"). When this lands, either close #209 or retitle it to the styling-only options that remain.

Surfaced by

Operator smoke against VS Code, 2026-09-01.

[JDonaghy]

Coordinator Assignment

Machine: dell64
Repo: vimcode
Files: (unspecified)
Do not touch: (nothing else in flight)

Briefing

Issue #727: GTK: present confirmation dialogs through quadraui's native seam, not the in-canvas Dialog primitive

Summary

vimcode's "Unsaved Changes" confirmation is an in-canvas box painted by quadraui::Dialog; VS Code's equivalent is a real OS dialog. Once quadraui grows a native message-dialog seam (quadraui#666), route GTK's confirmation dialogs through it. TUI keeps the in-canvas primitive — that is the point of the split, not a limitation.

Depends on quadraui#666. Needs a src/quadraui_pin.rs bump; do not start until that has landed.

The edge-trigger problem — this is the actual work

vimcode's dialogs are frame-driven state. engine.dialog: Option<Dialog> (src/core/engine/mod.rs:363) is converted to a quadraui::Dialog by dialog_panel_to_quadraui_dialog (src/render.rs:4887) and repainted every frame via backend.draw_dialog (src/gtk/mod.rs:9333). A native dialog cannot be re-presented every frame — it must be presented once, on the transition from no-dialog to dialog.

So:

1. Present once, keyed on the dialog's identity, on the no-dialog to dialog edge.
2. While a native dialog is in flight, suppress the in-canvas draw.
3. Map the returned WidgetId back to exactly the action the in-canvas DialogHit::Button(id) path already produces at src/gtk/mod.rs:4399, so the engine sees one code path and one set of outcomes.

The blocking call shape is already established here — backend.services().show_file_open_dialog(...) at src/gtk/mod.rs:1773 blocks on quadraui's nested-mainloop pump, and PendingFileDialog (src/gtk/mod.rs:1089) already models "a dialog is in flight". Follow that, don't invent a second mechanism.

What stays in-canvas

• TUI, always. src/tui_main/** is untouched by this issue and its snapshots must not move.
• Any dialog quadraui reports as not natively expressible — i.e. carrying a table or a text input. That covers the help dialog (src/render.rs:8177 sc_help_dialog, a DialogTable), the text-input dialogs (src/render.rs:4915), and source control's (src/core/engine/source_control.rs:269). These must keep working exactly as today.

The native/in-canvas decision comes from quadraui's native_dialog_options returning None — do not hand-maintain a list of dialog kinds in this repo.

Acceptance

• Closing a modified buffer in the GTK build shows a real OS dialog. Save / Don't Save / Cancel produce the same engine outcomes as today's in-canvas buttons.
• The dialog is presented exactly once per open, not once per frame — assert the present count stays at 1 across N frames with the dialog unchanged.
• Both entry points go native: :q with unsaved changes, and the tab-x close path (Msg variants at src/gtk/mod.rs:1380-1388).
• TUI behaviour byte-identical.
• Help dialog and text-input dialogs still render in-canvas on GTK.

Files

• src/gtk/mod.rs — present-once edge trigger, response-to-action mapping, in-canvas suppression while native is in flight
• src/quadraui_pin.rs — pin bump
• src/render.rs — only if the descriptor must carry the native/in-canvas decision through

Supersedes

#209, option 3 ("native widget escape hatch"). When this lands, either close #209 or retitle it to the styling-only options that remain.

Surfaced by

Operator smoke against VS Code, 2026-09-01.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 28m 34s
Log: /home/john/.coord/logs/6e4d1bdaed50.log

[JDonaghy]

Coordinator: Assignment Failed

Machine: dell64
Status: failed
Exit code: 1
Duration: 3s
Log: /home/john/.coord/logs/6a5ea0066f17.log

Error

api_error

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 3m 14s
Log: /home/john/.coord/logs/508e0d8c4670.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: precision
Status: done
Exit code: 0
Duration: —

Summary

Verdict: request-changes

Review findings

Blocking findings

• src/gtk/testing.rs: the two new black-box tests (native_dialog_presented_exactly_once_across_repeated_frames and text_input_dialog_stays_in_canvas_not_native) assert exclusively on internal bookkeeping state — h.dialog_layout.borrow().is_none()/is_some(), h.native_dialog_shown.get(), h.pending_native_dialog.take() — never on painted content. dialog_layout is documented in its own added doc comment as a "cache", which is precisely the anti-pattern this repo's CLAUDE.md Testing section forbids verbatim: "reject a black-box test that asserts ... a layout cache is Some ... The assertion must read painted content -- screen text, screen_contains, or pixels." The exact tool needed already exists and is used for this identical suppression pattern elsewhere in this same file (e.g. !h.driver.screen_contains("New Tab") at testing.rs:4500), but neither new test calls screen_contains to confirm the in-canvas dialog's title/button text (e.g. "Unsaved Changes", "Save All & Quit") is actually absent from the Cairo surface when the native path is taken, or present when the text-input path is taken. As written, both tests would stay green even if render_content's suppression logic set dialog_layout to None while still calling frame.draw(backend) underneath (a real paint bug of exactly the Command palette fails to open silently (broken on develop) #587/GTK live render path silently drops 13 populated ScreenLayout fields (rest of #587's blast radius) #592 shape this rule exists to catch).
• The PR (commit message + test doc comments) never states that the new tests were observed RED against unfixed code, which CLAUDE.md's Testing section makes MANDATORY for any behaviour-changing PR ("Remove the fix, re-run, confirm red, restore — then say so in the PR ... No such statement is a request-changes"). The native_dialog_presented_exactly_once_across_repeated_frames doc comment argues by inspection which guard-line removal would make it fail, but that is not the same as a stated, performed red/green cycle, and no such statement appears anywhere in the diff or commit body.

Non-blocking concerns

• Edge-trigger reset fragility: native_dialog_shown is only flipped back to false when a later render_content call observes screen.dialog == None. If any future process_dialog_result arm ever opens a new dialog synchronously within the same call that closed the previous one (a plausible extension — e.g. a chained "save failed, retry?" dialog after quit_unsaved's save_quit), the new dialog would be invisible: it's natively-expressible so it takes the Some(opts) branch (which unconditionally sets dialog_layout = None, suppressing in-canvas paint) but the stale native_dialog_shown == true skips queuing a native present too. I checked the current process_dialog_result match arms and none chain a second show_dialog/show_quit_confirm call today, so this isn't live, but it's a latent trap worth guarding against (e.g. reset the flag explicitly inside dialog_click_button/dialog_cancel's callers, or at the top of run_pending_native_dialog, rather than only lazily on the next paint).
• Dialogs whose buttons have no action == "cancel" entry (e.g. the file-changed-on-disk "Yes"/"No" dialog and the "Reload"/"Keep" dialog in src/core/engine/buffers.rs) get is_cancel: false on every button, so GtkAlertDialog::set_cancel_button is never invoked for them. The end-to-end outcome is still correct (GTK's own dismiss-without-choosing path returns None → Engine::dialog_cancel(), matching in-canvas Escape), but these dialogs lose the native "Escape activates a specific labeled button" affordance that dialogs with an explicit Cancel button get. Worth a one-line note in is_cancel's doc comment (in render.rs) calling this out as accepted, if it is.
• Narrow timing window: while a natively-expressible dialog is queued (pending_native_dialog set) but not yet drained by tick(), dialog_layout is already None, so the dialog is never pushed onto modal_stack_handle(). A click landing in that window falls through handle_mouse_click_msg's "outside" branch and silently clears engine.dialog, cancelling it without going through dialog_cancel()/process_dialog_result. In practice this window is a single event-loop tick and GTK's own modal barrier should block canvas input once the real AlertDialog is up, so the practical exposure looks small, but it's not covered by the new tests either way.

Nits

• apply_dialog_action's extraction out of handle_mouse_click_msg's dialog-button block is a clean, behavior-preserving refactor — no issue there.
• dialog_btn_index dedup between the in-canvas hit-test path and the new native response mapping is a good reuse; nothing to flag.
• Pin bump (Cargo.toml rev + [patch.crates-io] vt100 rev + Cargo.lock in both places) is consistent and matches the documented pinning convention.

[JDonaghy]

Review Complete — ⚠️ Changes Requested

Reviewer findings could not be posted directly to PR #738 (gh pr review was rejected — likely a self-review restriction). Findings are reproduced here.

Note: a completion comment was posted earlier but findings could not be extracted at that time. These are the retroactive findings.

Blocking findings

• src/gtk/testing.rs: the two new black-box tests (native_dialog_presented_exactly_once_across_repeated_frames and text_input_dialog_stays_in_canvas_not_native) assert exclusively on internal bookkeeping state — h.dialog_layout.borrow().is_none()/is_some(), h.native_dialog_shown.get(), h.pending_native_dialog.take() — never on painted content. dialog_layout is documented in its own added doc comment as a "cache", which is precisely the anti-pattern this repo's CLAUDE.md Testing section forbids verbatim: "reject a black-box test that asserts ... a layout cache is Some ... The assertion must read painted content -- screen text, screen_contains, or pixels." The exact tool needed already exists and is used for this identical suppression pattern elsewhere in this same file (e.g. !h.driver.screen_contains("New Tab") at testing.rs:4500), but neither new test calls screen_contains to confirm the in-canvas dialog's title/button text (e.g. "Unsaved Changes", "Save All & Quit") is actually absent from the Cairo surface when the native path is taken, or present when the text-input path is taken. As written, both tests would stay green even if render_content's suppression logic set dialog_layout to None while still calling frame.draw(backend) underneath (a real paint bug of exactly the Command palette fails to open silently (broken on develop) #587/GTK live render path silently drops 13 populated ScreenLayout fields (rest of #587's blast radius) #592 shape this rule exists to catch).
• The PR (commit message + test doc comments) never states that the new tests were observed RED against unfixed code, which CLAUDE.md's Testing section makes MANDATORY for any behaviour-changing PR ("Remove the fix, re-run, confirm red, restore — then say so in the PR ... No such statement is a request-changes"). The native_dialog_presented_exactly_once_across_repeated_frames doc comment argues by inspection which guard-line removal would make it fail, but that is not the same as a stated, performed red/green cycle, and no such statement appears anywhere in the diff or commit body.

Non-blocking concerns

• Edge-trigger reset fragility: native_dialog_shown is only flipped back to false when a later render_content call observes screen.dialog == None. If any future process_dialog_result arm ever opens a new dialog synchronously within the same call that closed the previous one (a plausible extension — e.g. a chained "save failed, retry?" dialog after quit_unsaved's save_quit), the new dialog would be invisible: it's natively-expressible so it takes the Some(opts) branch (which unconditionally sets dialog_layout = None, suppressing in-canvas paint) but the stale native_dialog_shown == true skips queuing a native present too. I checked the current process_dialog_result match arms and none chain a second show_dialog/show_quit_confirm call today, so this isn't live, but it's a latent trap worth guarding against (e.g. reset the flag explicitly inside dialog_click_button/dialog_cancel's callers, or at the top of run_pending_native_dialog, rather than only lazily on the next paint).
• Dialogs whose buttons have no action == "cancel" entry (e.g. the file-changed-on-disk "Yes"/"No" dialog and the "Reload"/"Keep" dialog in src/core/engine/buffers.rs) get is_cancel: false on every button, so GtkAlertDialog::set_cancel_button is never invoked for them. The end-to-end outcome is still correct (GTK's own dismiss-without-choosing path returns None → Engine::dialog_cancel(), matching in-canvas Escape), but these dialogs lose the native "Escape activates a specific labeled button" affordance that dialogs with an explicit Cancel button get. Worth a one-line note in is_cancel's doc comment (in render.rs) calling this out as accepted, if it is.
• Narrow timing window: while a natively-expressible dialog is queued (pending_native_dialog set) but not yet drained by tick(), dialog_layout is already None, so the dialog is never pushed onto modal_stack_handle(). A click landing in that window falls through handle_mouse_click_msg's "outside" branch and silently clears engine.dialog, cancelling it without going through dialog_cancel()/process_dialog_result. In practice this window is a single event-loop tick and GTK's own modal barrier should block canvas input once the real AlertDialog is up, so the practical exposure looks small, but it's not covered by the new tests either way.

Nits

• apply_dialog_action's extraction out of handle_mouse_click_msg's dialog-button block is a clean, behavior-preserving refactor — no issue there.
• dialog_btn_index dedup between the in-canvas hit-test path and the new native response mapping is a good reuse; nothing to flag.
• Pin bump (Cargo.toml rev + [patch.crates-io] vt100 rev + Cargo.lock in both places) is consistent and matches the documented pinning convention.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 7m 14s
Log: /home/john/.coord/logs/e3b8529ef2af.log