#553: [retry] #448-F: GTK tab click (activate/close) dead when only ONE tab group exists - #652
Merged
JDonaghy merged 3 commits intoAug 10, 2026
Conversation
… unify click-side tab-bar hit derivation #553 reports that with a single tab group, clicking a tab does not activate it and clicking its × does not close it, while 2+ groups work fine. Verification first: the reported behaviour is **already fixed** on develop. `8fbbf85` (#546 FAILED-3) removed the actual defect — `screen_zone_hit_test`'s single-group arm hardcoded the tab row's top at `y >= 0.0` instead of deriving it from the window rects the way the split arm did, so once #552 gave GTK a persistent menu/title-bar band (`main_content_bounds.y > 0`) single-group tab clicks stopped matching any real pixel. `3d81663` (#549) then unified the draw-loop half. What #553 never got was the acceptance test, and the click-side duplication that let the two shapes drift in the first place is still there. Both are added here. Black-box tests (the #553 acceptance criterion) ----------------------------------------------- `gtk::testing` (the #646 headless GTK harness — the GTK twin of TuiDriver) gains `Harness::tab_center` / `tab_close_center`, which read the absolute tab-slot and close-button geometry the *rasteriser* reported this frame (`Backend::tab_bar_layout`, via the existing `cached_tab_slots_abs` / `cached_tab_close_abs`). Same rule as the pre-existing `window_center`: aim events at the rect the renderer actually painted, never at hardcoded pixels. On top of those, two tests open three tabs in the default single group and drive real `dispatch_click` through the production `App`: - `single_group_tab_click_activates_that_tab` - `single_group_tab_close_button_closes_that_tab` Both were confirmed against a matrix of single-group configurations (breadcrumbs on/off × per-window vs global status line × each of three tab indices) plus a split-group control before being narrowed to the two committed cases. Click-side unification (the #549 counterpart #553 asks for) ------------------------------------------------------------ `screen_zone_hit_test` derived its tab-bar hit band in two independent `if let Some(split) = ... else { ... }` arms — exactly the divergence that produced the "works with 2+ groups, dead with 1" asymmetry. Extracted `render::tab_bar_hit_bands()` (the click-side mirror of #549's `tab_bar_draw_targets`) so both shapes come out of one derivation, with the band top always `window_content_top - tab_bar_height`. Behaviour-preserving: the two new guards (`bounds.width > 0.0` in the split arm, `width <= 0.0` in the single arm) only skip bands a point-in-rect test could never match anyway, and they align the hit side with the zero-width filter `tab_bar_draw_targets` already applies on the draw side. `test_tab_bar_hit_bands_single_and_split_share_one_derivation` pins both shapes against the same chrome-offset content origin. Note: built and tested with `VIMCODE_QUADRAUI_UNPINNED=1` — the sibling quadraui checkout is on `e31258f`, two commits behind vimcode's pin `f6d27c2`, and is shared with other worktrees so it was not re-checked-out. The one resulting failure (`tui_main::shell_app::tests::menu_reveal_then_search_icon_click_opens_search_not_explorer`, the activity-bar hit-row guard `43349b5` un-ignored *because of* that pin bump) reproduces identically on a stashed tree and is unrelated to this change. Everything else is green: 2031 lib + 2259 vimcode-bin + 2246 vcd-bin + all integration tests. `cargo clippy --all-targets` emits the same 121 pre-existing warnings before and after this diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…document Harness::tab_center's cross-map invariant Addresses the blocking review finding on 6b67cfd: no FAIL-then-PASS proof was shown that the render.rs change this PR ships actually guards against the pre-8fbbf85 defect. RED-FIRST evidence (the render.rs unit tests, which are what this PR's code change actually touches) ------------------------------------------------------------------------ Reverted `tab_bar_hit_bands`'s single-group arm in place to the literal pre-8fbbf85 bug (hardcode the bar's top at the coordinate-system origin instead of deriving it from the window rects), then ran the two render.rs tests that pin this derivation — the NEW `test_tab_bar_hit_bands_ single_and_split_share_one_derivation` (this PR) and the PRE-EXISTING `test_single_group_tab_bar_hit_test_with_editor_offset` (8fbbf85, already on develop) — via `cargo test --bin vcd` (fast TUI-lane build, no display, no gtk needed since render.rs is backend-shared): FAIL (bug reinstated): thread 'render::tests::test_single_group_tab_bar_hit_test_with_editor_offset' panicked at src/render.rs:16474:22: expected TabBar zone, got None thread 'render::tests::test_tab_bar_hit_bands_single_and_split_share_one_derivation' panicked at src/render.rs:16530:9: assertion `left == right` failed: the single-group band must start at the *content* origin minus the bar height, not at 0.0 (#546 FAILED-3 / #553): [TabBarHitBand { group_id: GroupId(0), x: 50.0, y: 0.0, width: 800.0, height: 32.0 }] left: 0.0 right: 100.0 test result: FAILED. 0 passed; 2 failed; 0 ignored; ... PASS (fix restored, `git checkout -- src/render.rs`): test render::tests::test_single_group_tab_bar_hit_test_with_editor_offset ... ok test render::tests::test_tab_bar_hit_bands_single_and_split_share_one_derivation ... ok test result: ok. 2 passed; 0 failed; 0 ignored; ... An important, harder finding from doing this properly ------------------------------------------------------------------------ The two *GTK black-box* tests from 6b67cfd (`single_group_tab_click_ activates_that_tab`, `single_group_tab_close_button_closes_that_tab`) do NOT go red against this same reinstated bug, under either of two conditions I tried: 1. As committed (production click routing): GTK's `pixel_to_click_target` resolves clicks primarily via `quadraui::FrameHitMap` (#449, landed *after* 8fbbf85) and only falls back to `screen_zone_hit_test` / `tab_bar_hit_bands` when the frame hit-map misses. Confirmed via a temporary `eprintln!` that `tab_bar_hit_bands` is never even called during either GTK test — FrameHitMap resolves the click every time. 2. Forcing the fallback path (temporarily hardcoding `cached_frame_hit_map` to always be `None`, simulating pre-#449 routing) *plus* the reinstated bug: both tests still passed. Root cause: this harness's default `ShellConfig::with_title_bar(1.0)` chrome only offsets the content origin by ~23px (`min_y=65`, `tab_bar_height=42`), and the actual painted click y (32.5, read from `tab_close_abs`) happens to fall inside *both* the correct band `[23, 65)` and the buggy hardcoded band `[0, 42)` — the offset isn't large enough to separate them at this window size. The render.rs unit tests above use a synthetic 100px offset specifically to avoid this coincidence; the GTK harness's default chrome does not reproduce it. So: literal historical RED-FIRST for the GTK black-box tests (checkout the commit before 8fbbf85, apply the tests, watch them fail) is not achievable in this repo — the GtkDriver harness (`c36db7e`) postdates both 8fbbf85 and #449 chronologically, and even reproducing the fallback path by hand in the current tree doesn't flip these two tests red at this harness's default window/chrome geometry. That doesn't mean the acceptance criterion is unmet — both tests do exercise real production click dispatch and pass against the shipped behavior, satisfying #553's literal ask ("assert a tab click changes the active tab and the × closes it") — but they are not proof of catching *this specific* regression the way the render.rs tests above are. Documenting this rather than asserting false confidence. Non-blocking: confirming scope ------------------------------------------------------------------------ The `tab_bar_hit_bands` extraction is confirmed in scope: the issue's own root-cause hint frames this exact click-side derivation as "the click-side counterpart of #549" (the draw-loop unification), and the reviewer's note agreed this reading is plausible. It touches only the two inline arms of `screen_zone_hit_test` that already existed (split arm's loop body, single arm's math) — behavior-preserving, no new hit-test semantics. Non-blocking: tab_center/tab_close_abs invariant ------------------------------------------------------------------------ Documented in `src/gtk/testing.rs`: `Harness::tab_center` reads y from `tab_close_abs` and x from the separate `tab_slots_abs` map. Both are populated back-to-back per group in the same tab-bar draw loop (`App::render_content`), so they can't currently disagree — now stated explicitly at the call site per the review note. Verification ------------------------------------------------------------------------ cargo test — exit 0 (2031 lib + 2246 vcd-bin + 2259 vimcode-bin, all green) cargo test --no-default-features — exit 0 (2031 lib + 2246 vcd-bin, all green) Both runs skip `tui_main::shell_app::tests::menu_reveal_then_search_icon_ click_opens_search_not_explorer` — confirmed (by stashing this branch's diff and rerunning just that test) to fail identically on unmodified develop, an unrelated pre-existing artifact of the sibling quadraui checkout being two commits behind the `quadraui-pin.txt` pin in this shared environment (`VIMCODE_QUADRAUI_UNPINNED=1` used throughout, per the prior worker's note — the sibling checkout is shared across worktrees and was not touched). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_click_target) Addresses the blocking review finding on 8b8556d: the RED-then-GREEN evidence existed only for the render.rs unit tests, which bypass `pixel_to_click_target` — the actual GTK click-dispatch entry point the issue's root-cause hint names (`src/gtk/click.rs:44`). That gap is now closed at the layer the issue points at. New: gtk::click::single_group_tab_click_dispatch_tests ------------------------------------------------------------------------ Two tests driving the production `pixel_to_click_target` / `handle_mouse_click` against a single-group layout with three tabs: - single_group_tab_click_activates_that_tab_via_click_dispatch - single_group_tab_close_click_targets_and_closes_that_tab_via_click_dispatch Two deliberate choices make them discriminate where the black-box `gtk::testing` pair does not: 1. `frame_hit_map: None`, forcing the `screen_zone_hit_test` fallback branch — the precedented technique from this file's own `cross_split_drag_focus_tests::drag_continuation_does_not_steal_focus_to_ neighboring_group`, and independent of harness chrome geometry. 2. A synthetic 100px content offset (matching the render.rs test's), because the headless harness's default ~23px title-bar chrome is smaller than the tab bar's own height and so does not separate the correct band from the buggy origin-anchored one. They are two separate `#[test]`s on purpose: with the bug reinstated BOTH go red independently, which one test with two sequential assertions could not show (it would panic on the first and never reach the second). Also addressed (both non-blocking review notes) ------------------------------------------------------------------------ - `src/gtk/testing.rs`: the "these black-box tests do not catch this specific regression" finding — previously only in 8b8556d's commit message — is now a doc comment on the tests themselves, with both reasons and pointers to the tests that DO guard it. Also restated in the PR description below. - `src/render.rs`: `tab_bar_hit_bands`'s doc gains a "How much of the live click path this actually covers" section, tightening the earlier framing: `screen_zone_hit_test` has exactly one caller (gtk::click), where it is the FALLBACK behind #449's `FrameHitMap`; TUI never calls it (it has its own hit-test in tui_main/mouse.rs). So the unification removes the shape of divergence that produced #546/#553 plus fixes the pre-paint/miss path — it is not an everyday-break repair for GTK users. No production code changed in this commit — tests and documentation only. PR DESCRIPTION (coordinator: please include the block below verbatim in the PR body) ==================================================================================== ## RED-FIRST evidence — GTK click-dispatch layer (`pixel_to_click_target`) Method: reinstate the literal pre-`8fbbf85` bug in `render::tab_bar_hit_bands`'s single-group arm (`y: 0.0` instead of `y: min_y - tab_bar_height`), run, restore, run again. Command: `cargo test --bin vimcode single_group_tab`. ### FAIL (bug reinstated) — exit 101 ``` running 6 tests test tui_main::render_impl::tests::test_tui_single_group_tab_drag_reorder_not_split_477 ... ok test render::tests::test_single_group_tab_bar_hit_test_with_editor_offset ... FAILED test gtk::click::single_group_tab_click_dispatch_tests::single_group_tab_click_activates_that_tab_via_click_dispatch ... FAILED test gtk::click::single_group_tab_click_dispatch_tests::single_group_tab_close_click_targets_and_closes_that_tab_via_click_dispatch ... FAILED test gtk::testing::tests::single_group_tab_close_button_closes_that_tab ... ok test gtk::testing::tests::single_group_tab_click_activates_that_tab ... ok ---- gtk::click::single_group_tab_click_dispatch_tests::single_group_tab_click_activates_that_tab_via_click_dispatch stdout ---- panicked at src/gtk/click.rs:1601:9: a single-group click on tab 0's body must resolve as a tab-bar hit, got None (pre-fix this was ClickTarget::None — the click missed the bar entirely) ---- gtk::click::single_group_tab_click_dispatch_tests::single_group_tab_close_click_targets_and_closes_that_tab_via_click_dispatch stdout ---- panicked at src/gtk/click.rs:1621:9: assertion `left == right` failed: a single-group click on tab 1's × must resolve to CloseTab for tab 1 (pre-fix: ClickTarget::None, so nothing ever closed) left: None right: CloseTab(GroupId(0), 1) ---- render::tests::test_single_group_tab_bar_hit_test_with_editor_offset stdout ---- panicked at src/render.rs:16489:22: expected TabBar zone, got None test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 2265 filtered out ``` ### PASS (fix restored) — exit 0 ``` running 6 tests test tui_main::render_impl::tests::test_tui_single_group_tab_drag_reorder_not_split_477 ... ok test render::tests::test_single_group_tab_bar_hit_test_with_editor_offset ... ok test gtk::click::single_group_tab_click_dispatch_tests::single_group_tab_close_click_targets_and_closes_that_tab_via_click_dispatch ... ok test gtk::click::single_group_tab_click_dispatch_tests::single_group_tab_click_activates_that_tab_via_click_dispatch ... ok test gtk::testing::tests::single_group_tab_close_button_closes_that_tab ... ok test gtk::testing::tests::single_group_tab_click_activates_that_tab ... ok test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 2265 filtered out ``` `cargo test --bin vimcode tab_bar_hit_bands` (the layout-level guard added by this PR) — exit 0: ``` test render::tests::test_tab_bar_hit_bands_single_and_split_share_one_derivation ... ok test result: ok. 1 passed; 0 failed ``` Its own FAIL output against the same reinstated bug is in commit `8b8556d`. ## Known limitation of the two GtkDriver black-box tests Note in the FAIL output above that `gtk::testing::tests::single_group_tab_click_ activates_that_tab` and `..._close_button_closes_that_tab` stay **green** with the bug reinstated. They are the acceptance tests #553 asks for (a real click through production `dispatch_click`, asserting the engine's active tab / tab count changed) but they are NOT regression guards for this specific defect, for two independent reasons: 1. GTK's `pixel_to_click_target` resolves clicks primarily via the cached `quadraui::FrameHitMap` (#449, which postdates `8fbbf85`), falling back to `screen_zone_hit_test` / `tab_bar_hit_bands` only on a miss — so a driver-level click never reaches the regressed code. Confirmed with a temporary `eprintln!`: `tab_bar_hit_bands` is never called during either test. 2. Even with the fallback forced, the harness's default `ShellConfig::with_title_bar(1.0)` chrome offsets the content origin by only ~23px, less than `tab_bar_height` (42px), so the painted click y (32.5) falls inside *both* the correct band `[23, 65)` and the buggy band `[0, 42)`. Hence the new dispatch-level tests, which pass `frame_hit_map: None` and use a synthetic 100px offset for exactly these reasons. This limitation is now documented in-code on the black-box tests themselves, not just in git history. ## Scope of the production change (`render::tab_bar_hit_bands`) `screen_zone_hit_test` has exactly one caller — `gtk::click` (`pixel_to_click_target` / `resolve_tab_right_click`) — and there it is the **fallback** behind #449's `FrameHitMap`, into which every TabBar surface is pushed on each `render_content` pass. TUI does not call it at all (`tui_main/mouse.rs` has independent hit-test logic). So this refactor is best read as removing the *shape* of divergence that produced #546/#553 — one derivation instead of two arms that can drift — plus correctness on the pre-first-paint / hit-map-miss path. It is not repairing an everyday break for GTK users; #449's hit map already covers the steady-state click path. This is the "click-side counterpart of #549" the issue text asks for. ## Verification | Lane | Command | Exit | |---|---|---| | GUI feature on, no DISPLAY | `cargo test` | **0** (0 failed, 46 suites) | | TUI/core | `cargo test --no-default-features` | **0** (0 failed) | | Format | `cargo fmt -- --check` | **0** | No new compiler or clippy warnings from the changed files (`src/gtk/click.rs`, `src/gtk/testing.rs`, `src/render.rs`). Remaining warnings in the run are pre-existing on `develop` (non-snake-case test names and unused imports in `src/core/engine/tests.rs`, `git.rs`, `tests/*/mod.rs`; one `popup.line_text.get(0)` clippy lint at `src/render.rs`, present on develop at line 16121). Note: the sibling quadraui checkout is now AT the pinned rev (`f6d27c239203e28525ac1798d07a31a9dbe9729a`), so unlike the earlier commits on this branch these runs needed no `VIMCODE_QUADRAUI_UNPINNED=1` escape hatch and no `--skip`. `tui_main::shell_app::tests::menu_reveal_then_search_icon_click_ opens_search_not_explorer` passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Closes #553
Automated PR opened by coordinator for review of issue #553.