Web UI: open a table's query in a new tab on middle click, Shift+click, or the platform's new-tab modifier click#109246
Conversation
In the databases/tables panel, clicking a table (or its size/rows/engine addendum) with the middle mouse button, or with Shift/Ctrl/Cmd held - the gestures that conventionally open a link in a new tab - now opens the corresponding query in a new Web UI tab instead of replacing the current query. A plain click keeps the previous behavior, and a second click on an already-loaded query still runs it, regardless of the button or modifiers. Middle clicks fire `auxclick` rather than `click`, so both events are wired up. On Linux/X11 the middle button additionally pastes the primary selection into the freshly-focused query editor; this is suppressed for exactly that one paste so no clipboard content is appended after the query. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [b06362d] Summary: ✅
AI ReviewSummaryThis PR teaches the Web UI databases panel to open table-related queries in a new Web UI tab for middle/Shift/modifier clicks, plus the Linux/X11 paste suppression needed around the focused editor. The event-routing and paste-guard fixes from the earlier review threads are present in the current code, but the remaining contract text is still inconsistent with the final macOS behavior: the new helper comment and the PR changelog wording still advertise PR Metadata
Missing context / blind spots
Final Verdict |
…focus Addresses the code review on #109246: the guard that swallows the Linux/X11 `PRIMARY` paste after a middle-click new-tab open was armed in `auxclick` and cleared by a fixed 500 ms timeout. That timeout was decoupled from the moment the new tab's editor is actually focused - `openQueryInNewTab` goes through `activateTab`, which awaits `restoreFromHistory` (and thus `updateQueryParams` /`tokenize`) before `query_area.focus` runs. On a cold first activation or a slow query, the window could expire before focus, so the unwanted paste still landed; and the flag also stayed armed on the already-loaded (`postOne`) path where no new-tab focus happens, so a later genuine `Ctrl+V` could be swallowed. Now the guard is armed right around the real `query_area.focus` in the new-tab activation path, only for a genuine middle click, and cleared by the paste itself or a short post-focus fallback. A slow activation can no longer let the guard expire before focus, and non-new-tab paths never arm it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d paste The X11 `PRIMARY` paste guard armed around `query_area.focus()` in the new-tab path relied on a 250ms fallback to disarm when no synthetic paste arrived. On platforms/cases where a middle click never produces a `PRIMARY` paste (macOS, Windows, empty selection), that window let the guard swallow a real `Ctrl+V` / `Cmd+V` issued within 250ms of a middle-click new-tab open. Discriminate by keystroke instead of timing: the X11 `PRIMARY` paste is mouse-driven and carries no keystroke, while a real `Ctrl+V` / `Cmd+V` is always preceded by its own paste-shortcut `keydown`. Disarm the guard on that `keydown` (which fires before the resulting `paste`), so a genuine keyboard paste is never eaten regardless of timing or platform. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The keydown-based discrimination for the X11 `PRIMARY` middle-click paste guard only disarmed `suppress_middleclick_paste` on `Ctrl`/`Cmd`+`V`. But `Shift+Insert` is also a standard keyboard paste shortcut on Linux/Windows, and it fires its own `keydown` before the resulting `paste` just like `Ctrl`+`V` does. Without disarming on it, a real `Shift+Insert` paste issued within the 250 ms post-focus fallback window after a middle-click new-tab open was swallowed, violating the invariant that the guard only ever eats the synthetic X11 `PRIMARY` paste and never a real keyboard paste. Disarm the guard on `Shift+Insert` as well. The change is additive: the X11 `PRIMARY` paste is mouse-driven and carries no keystroke, so it is still swallowed; only keyboard-originated pastes gain the extra shortcut. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On macOS, `Ctrl+Click` is the secondary (context-menu) click, not the "open in a new tab" gesture - that role belongs to `Cmd+Click`. The table-row activation handler in `play.html` treated any `ctrlKey` click as a new-tab open, so a normal macOS context click on a table row or its size/rows/engine addendum created a new Web UI tab instead of showing the context menu. Make the modifier-open check platform-aware, reusing the existing `isMac` constant: the new-tab modifier is `metaKey` (Cmd) on macOS and `ctrlKey` elsewhere, matching how browsers themselves choose the new-tab modifier. Middle click and Shift+click stay new-tab gestures on every platform. This addresses the AI-Review "Request changes" verdict on commit 906c6f2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Web UI gesture routing (plain click replaces the current tab; middle/Shift/Cmd/Ctrl click opens a new tab; re-activating an already-loaded query still runs it) and the Linux/X11 middle-click paste guard depend on browser- and OS-specific event ordering across `click`, `auxclick`, modifier dispatch, the focus-triggered `PRIMARY` paste, and the `keydown`/`paste` discrimination. There is no browser-level test harness for `play.html` in the repository (the only automated coverage, `01528_play.sh`, just checks that the `/play` endpoint serves the page), so this contract is verified by hand. Commit the manual test matrix next to `onTableActivate` so the exact gestures and their expected outcomes - including the case where a real keyboard paste right after a middle-click new-tab open must be preserved while only the synthetic `PRIMARY` paste is swallowed - are the recorded, re-runnable contract for anyone touching `set_query_or_run`, `openQueryInNewTab`/`activateTab`, or `suppress_middleclick_paste`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (`play.html`, `clickhouse-gh`): the X11 PRIMARY paste guard `suppress_middleclick_paste` is a single shared bit, but each middle-click new-tab open scheduled its own uncancelled 250ms fallback timer. Two overlapping opens (open table A, then table B within that window) both armed the same bit; A's stale timeout could then fire and clear the bit that B had re-armed, before B's focus-triggered PRIMARY paste arrived - so B's paste landed in the editor. Keep the handle of the single fallback timer (`middleclick_paste_timeout`) and cancel it whenever the guard is re-armed or consumed (`disarmMiddleclickPaste`), so an older open's timeout can never clear a newer open's guard. Also extended the committed manual test matrix with the overlapping-open case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@groeneai, what's with |
|
@alexey-milovidov Answers to the three questions: What's with it: the test asserts that a "crazy Who broke it: nobody, in code. It is a test-robustness gap under CI settings randomization, not a regression. CIDB shows 0 failures on master in 30 days and PR-only failures spread across 12 unrelated PRs. Deterministic local repro (debug): original test + What to revert: nothing. The fix is to pin both settings in the test. That is already in flight in #109330 (pins This PR (play.html only) does not touch the pipeline and cannot cause it. Example failing run: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=96130&sha=68fa461995b63b0bd29db51fccc36b5fbad79585&name_0=PR&name_1=Stateless%20tests%20%28arm_asan_ubsan%2C%20targeted%29 |
pamarcos
left a comment
There was a problem hiding this comment.
I've manually tested it and it works just as the description states 🚀
Address AI review (⚠️ Request changes): on macOS, `Ctrl+Click` is the secondary (context-menu) click, not a new-tab or activation gesture. The new-tab branch already used the platform-aware modifier (`metaKey` on macOS, `ctrlKey` elsewhere), so a macOS `Ctrl+Click` was not opening a new tab - but it still fell through to the plain-click `query_area.value = value` path, and a macOS `Ctrl+Click` on the already-loaded row still hit `postOne`. That contradicts the documented secondary-click contract. Short-circuit macOS `Ctrl+Click` at the top of `set_query_or_run` so it neither opens a new tab, replaces the editor, nor runs the query, leaving the gesture entirely to the browser (so the native context menu can appear). A middle click (button 1) is excluded so `Ctrl` + middle click still opens a new tab. Updated the in-file manual test matrix (case 6) and the branch comments to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109246&sha=52be366e5cc6bbd0fe2016d997940ceea424d62f&name_0=PR&name_1=Integration%20tests%20%28amd_tsan%2C%206%2F6%29 and provide a fix in a separate PR. If the fix is already in progress, link it here. All 8 parameterizations of |
|
Investigated. Fix in a separate PR: #110300. Root cause: not this PR. The session-scoped Fix: skip the startup access check for the s3 disks, mirroring the azure disks in the same config that already set No open tracking issue existed; no competing fix PR. |
Follow-up to ClickHouse#109246: the final implementation deliberately does not treat macOS `Ctrl+Click` as a new-tab gesture (it is the secondary/context-menu click there and is left to the browser), but the `openQueryInNewTab` helper comment still said "Shift/Ctrl/Cmd click". Spell out the actual contract: middle click, Shift+click, or the platform's new-tab modifier click - Cmd+click on macOS, Ctrl+click elsewhere - and point to the manual matrix above `onTableActivate`. Addresses the remaining review comment on ClickHouse#109246.
Duplicating a tab with a middle click focuses the query editor. On Linux/X11 the middle button additionally pastes the PRIMARY selection into the freshly focused editor, appending clipboard junk after the duplicated query. Guard against exactly that one paste, mirroring the mechanism in ClickHouse#109246: `activateTab` arms `suppress_middleclick_paste` around `query_area.focus()` for a genuine middle click, the editor's `paste` handler swallows that paste, and a real keyboard paste shortcut (Ctrl/Cmd+V or Shift+Insert) disarms the guard first so it is never eaten. A short fallback timer bounds how long a never-consumed guard can linger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In the Web UI (
play.html) databases/tables panel, clicking a table with the middle mouse button, with Shift held, or with the platform's new-tab modifier held (Cmd on macOS, Ctrl elsewhere) — the gestures that conventionally open a link in a new tab — now opens the corresponding query in a new Web UI tab instead of replacing the query in the current one. This also applies to the size/rows addendum (openssystem.tablesproperties) and the engine label (opensSHOW TABLE).A plain click keeps the previous behavior (loads the query into the current tab), and a second click on an already-loaded query still runs it, regardless of the button or modifiers used. On macOS,
Ctrl+Clickis the secondary (context-menu) click rather than a new-tab gesture, so it is left entirely to the browser: it neither opens a tab nor activates the query.Middle clicks fire
auxclickrather thanclick, so both events are wired up, and the middle-click autoscroll is suppressed. On Linux/X11 the middle button additionally pastes the primary selection into the freshly-focused query editor; this is suppressed for exactly that one paste, so no clipboard content is appended after the query. A normalCtrl+Vis unaffected.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
In the Web UI, clicking a table in the databases panel with the middle mouse button, with Shift, or with the platform's new-tab modifier (Cmd on macOS, Ctrl elsewhere) now opens its query in a new Web UI tab.
Version info
26.7.1.911(included in26.7and later)