gui: amend PKG/QA request (R1 escalation, R3 restated) + EXT grep-check note
Review feedback on the filed request: - R1: gui/CMakeLists.txt is on main now, so root's find_package(Qt6 ... Svg REQUIRED) is live — a missing SVG dev package is a hard configure failure for the whole project, not a skipped guard. Added the reason CI hasn't caught it: ubuntu-latest is 24.04 (where libqt6svg6-dev likely resolves), the project targets 26.04 (where it does not). Wrong name + runner/target release mismatch = the class of bug PKG/QA owns is currently unobservable in CI. That's the argument for R2, folded in. - R3: corrected — CI does build the GUI and runs its three ctests under the ci preset. What has no home is the non-unit-test DoD: 10k-row 60fps, flat RSS over a 10-minute run, and mockd --slow/--flaky/--drop-connection recovery. Asked for those specifically. - gui/docs/ext-requests-m1.md: CLAUDE.md §3 says the no-download-logic rule applies to extension/ too; GUI made its half an executable ctest, EXT's half is still prose. Suggested the ESLint equivalent for the existing extension-lint job. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_016Ne28kx4VreeBWZv82Nksd
This commit is contained in:
Claude Sonnet 5
@@ -0,0 +1,33 @@
|
||||
# GUI → EXT requests (M1)
|
||||
|
||||
Filed by lane GUI. `extension/` is EXT's; this is a suggestion, not a change.
|
||||
|
||||
## An executable version of the "no download logic in extension/" rule
|
||||
|
||||
CLAUDE.md §3:
|
||||
|
||||
> A grep for `curl|pwrite|sqlite` in `gui/` must come back empty. **Same for download
|
||||
> logic in `extension/`.**
|
||||
|
||||
GUI turned its half of that sentence into a gate: `gui/tests/no_download_logic.cmake`, run
|
||||
as the `gui_no_download_logic` ctest, greps `gui/src` for `curl_*` / `pwrite` / `sqlite` /
|
||||
`QSqlDatabase` / `QNetworkAccessManager` and fails the build on a hit. It was verified by
|
||||
dropping a `curl_`/`sqlite3_` file in and watching it go red.
|
||||
|
||||
The `extension/` half is still just prose. The extension is the more tempting place for
|
||||
download logic to creep in — a `fetch()` to grab bytes "just this once", a stream reader,
|
||||
a `Range` header assembled client-side — and none of that would fail any current check.
|
||||
|
||||
Suggested equivalent for EXT (adjust tokens to the TS/WebExtension surface):
|
||||
|
||||
* `fetch(` / `XMLHttpRequest` / `new Request(` used for anything but talking to the
|
||||
native host,
|
||||
* `Range:` / `Content-Range` header construction,
|
||||
* `ReadableStream` / `.getReader()` over response bodies,
|
||||
* `IndexedDB` / `chrome.downloads` / `browser.downloads` used to move bytes rather than
|
||||
hand off.
|
||||
|
||||
Cheapest form is an ESLint `no-restricted-syntax` / `no-restricted-globals` rule in the
|
||||
`extension-lint` CI job (already exists in `ci.yml`), so it runs per-PR with no new
|
||||
infrastructure. The point isn't the exact token list — it's that the rule fails a build
|
||||
instead of a review.
|
||||
@@ -6,7 +6,7 @@ CLAUDE.md §1. Everything below is apply-ready.
|
||||
|
||||
---
|
||||
|
||||
## R1 — `libqt6svg6-dev` is not a real package on 26.04 (blocks the GUI DoD)
|
||||
## R1 — `libqt6svg6-dev` is not a real package on 26.04 (hard configure failure now)
|
||||
|
||||
`apt-cache policy libqt6svg6-dev` on a clean Ubuntu 26.04 box gives `Candidate: (none)`.
|
||||
The Qt 6 SVG **dev** package in the 26.04 archive is **`qt6-svg-dev`** (currently
|
||||
@@ -16,10 +16,25 @@ CMake config, so `find_package(Qt6 COMPONENTS Svg)` fails without `qt6-svg-dev`.
|
||||
I checked the other 38 apt names in `bootstrap.sh` against `apt-cache policy` on 26.04 —
|
||||
`libqt6svg6-dev` is the only one with no candidate. Everything else resolves.
|
||||
|
||||
**Why it passed here anyway:** this dev box already has `qt6-svg-dev` installed (it came in
|
||||
with a `qt6-base-dev` recommends chain at some point), so `pkg-config --exists Qt6Svg`
|
||||
succeeds and `--check` is green. On a clean VM the `apt-get install` step aborts before
|
||||
`--check` ever runs.
|
||||
**This is no longer a latent bug.** `gui/CMakeLists.txt` is now on `main`, so the root
|
||||
`CMakeLists.txt` line
|
||||
|
||||
```cmake
|
||||
find_package(Qt6 6.6 REQUIRED COMPONENTS Widgets Svg Network LinguistTools)
|
||||
```
|
||||
|
||||
is live for every build. On a machine without the SVG dev package that is a hard configure
|
||||
failure for the whole project — not a skipped guard, not a GUI-only problem.
|
||||
|
||||
**Why nothing has gone red yet — and why that's R2's problem too:**
|
||||
|
||||
* This dev box already has `qt6-svg-dev` (pulled in by a `qt6-base-dev` recommends chain),
|
||||
so `pkg-config --exists Qt6Svg` succeeds and `--check` is green here.
|
||||
* CI runs on `ubuntu-latest`, which is **24.04**, where `libqt6svg6-dev` most likely still
|
||||
resolves. The project *targets* **26.04** (the `ci.yml` env comment says as much). So the
|
||||
one automated place that runs `bootstrap.sh` is on the wrong release to see this, and
|
||||
`--check` wouldn't catch it there anyway (R2). Wrong package name + runner-vs-target
|
||||
release mismatch means the class of bug PKG/QA owns is currently unobservable in CI.
|
||||
|
||||
### Fix — three files, same one-line change
|
||||
|
||||
@@ -97,19 +112,35 @@ Notes for whoever applies it:
|
||||
missing name. Downside: it needs the apt lists reasonably fresh and is noisier to parse.
|
||||
`apt-cache policy` is enough to catch the whole class of bug in R1; pick whichever you
|
||||
want to own.
|
||||
* CI: the `bootstrap-script` job already runs `./tools/bootstrap.sh --check` after the
|
||||
install. With this change that job would have gone red on `libqt6svg6-dev` **at the
|
||||
`--check` step on the 24.04 runner** even before the install step failed, because the
|
||||
name has no candidate there either.
|
||||
* **The runner mismatch is part of this.** Even with the name-validation loop, the
|
||||
`bootstrap-script` job on a 24.04 runner validates names against the *24.04* archive, not
|
||||
26.04. So R2 only fully closes the gap if the check also runs where the project ships —
|
||||
a 26.04 container step in that job (`container: ubuntu:26.04`), or a documented decision
|
||||
that 24.04 is close enough and why. Without that, a name that is valid on 24.04 and gone
|
||||
on 26.04 (exactly `libqt6svg6-dev`) still slips through.
|
||||
|
||||
---
|
||||
|
||||
## R3 (smaller) — a GUI CI job
|
||||
## R3 — the GUI DoD gates have nowhere to run in CI
|
||||
|
||||
There is no job that builds `velox-gui` on its own or runs its offscreen smoke. The
|
||||
`build` matrix will pick up the target and the `gui_downloadtablemodel` ctest once
|
||||
`gui/` merges, but the GUI DoD items — 10k rows at 60 fps, flat memory over 10 min,
|
||||
`--slow`/`--flaky`/`--drop-connection` recovery, `grep -r 'curl\|pwrite\|sqlite' gui/`
|
||||
empty, RTL layout — need somewhere to run. GUI can write the harness
|
||||
(`gui/tests/…` + a script); wiring it into `.github/workflows/ci.yml` is PKG/QA. Say the
|
||||
word and I'll send the harness as a follow-up request with the job stanza pre-written.
|
||||
To be precise about what already works: `VELOX_BUILD_GUI` defaults `ON`, the `ci` preset
|
||||
inherits `dev`, and once `gui/` is on `main` the `build` and `sanitizers` jobs configure
|
||||
and build `velox-gui` and run `ctest --preset ci`, which picks up all three GUI checks
|
||||
(`gui_downloadtablemodel`, `gui_rtl`, `gui_no_download_logic`). That part is covered.
|
||||
|
||||
What has no home is the part of the GUI M1 definition of done that isn't a unit test:
|
||||
|
||||
1. **10 000 rows scroll at 60 fps** (`mockd --tasks 10000`) — needs a frame-timing probe
|
||||
against the offscreen (or Xvfb) view; red when a scroll frame exceeds ~16 ms at the
|
||||
99th percentile.
|
||||
2. **Flat memory over 10 minutes of progress events** — needs RSS sampled across a
|
||||
10-minute `mockd --tasks 10000` run; red when RSS grows more than a small fixed slack
|
||||
(a leak in the progress-patch path is the thing this catches).
|
||||
3. **Unhappy-path recovery** — `mockd --slow`, `--flaky <f>`, `--drop-connection <s>`:
|
||||
the client must show the banner and recover without a freeze or crash; red on a crash,
|
||||
a hang (watchdog), or the connection state never returning to `Connected`.
|
||||
|
||||
GUI owns writing that harness (`gui/tests/` + a driver script, headless against `mockd`).
|
||||
Wiring it into `.github/workflows/ci.yml` as its own job — with the 10-minute one likely
|
||||
`nightly` rather than per-PR — is PKG/QA. Say the word and it comes over as a follow-up
|
||||
request with the job stanza pre-written.
|
||||
|
||||
Reference in New Issue
Block a user