Fix review findings across drawer, clipboard write-through, desktop and release automation - #32
Merged
Merged
Conversation
…nd release automation
Second review pass over the v0.11.6/v0.11.7 range, run as four sequential
workers over the areas the first pass did not reach.
Clipboard write-through (apps/web-client):
- The four status headlines the Copy button depends on are now defined once
in src/clipboard-status.ts and imported by both controller.ts and main.ts.
bindAndroidCopyWriteThrough recognises the controller's reply by matching
the status line against exact strings, so rewording one at a setStatus call
site silently stopped the Copy button writing to the PC clipboard -- with a
green suite, because the existing tests pinned only the main.ts side.
- The execCommand fallback had no generation guard, so a superseded copy still
stole focus and wrote its result over a newer request. It also copied the
live textarea selection rather than the text it had validated, so the string
placed on the clipboard could differ from the one the status claimed.
- On timeout the write-through overwrote whatever terminal status the
controller had produced. A disconnect ("Stream stopped") or an over-limit
clipboard ("Clipboard skipped ... exceeds N KiB") was replaced with "Copy
not confirmed", swapping an accurate diagnosis for a wrong one. It now only
claims failure when nothing else has explained the silence.
- Ctrl+C had no session guard, reinstating the "not confirmed" message that
controller.ts deliberately suppresses when no device is attached.
- The fallback now restores focus to wherever it was instead of assuming the
canvas, since parking focus there sends the next keystroke to Android.
- Added an event.repeat guard, and replaced a byte-for-byte copy of
clipboardShortcut with an import of the tested original.
Drawer:
- openGroup forced focus to the mirrored canvas two frames after the Clipboard
group opened. Its comment justified this by controller.ts focusing the
fallback textarea -- a listener deleted when Ctrl+V moved to a document-level
paste handler. The guard outlived its cause and now only steals focus from a
user typing into the textarea, injecting their keystrokes into Android.
- The @import of droidwebdisplay-main-drawer-core.css carried no ?v=, leaving
531 lines of drawer layout outside the cache-busting the entry sheet uses. A
stale copy after an upgrade renders every drawer group stacked and visible.
Desktop:
- The Summary "Local URL" was a live hyperlink in every server state while the
header Open button beside it was correctly gated, so stopping the server left
a blue link that leads to ERR_CONNECTION_REFUSED. It is now plain text unless
the server can serve it, and the colour comes from the palette instead of an
inline literal that silently overrode the QLabel#summaryUrl rule.
- _status_failed left the header Open button enabled indefinitely, because only
_apply_snapshot gated it and that path does not run on a probe failure.
- Added QPushButton#headerIconButton:disabled and :pressed. QSS specificity is
CSS2.1, so the id selector outranked the bare pseudo-class rules and a
disabled header button kept its enabled background with no press feedback.
- Removed setFixedSize(38, 38), which fought the 36px box in theme.py.
- Removed a no-op summary_layout.setAlignment(Qt.AlignTop): the one-argument
form positions a layout within a parent layout item, and this one is
installed directly on the card. addStretch(1) below is what actually works.
Release automation:
- pages.yml pinned four actions to mutable tags while every other workflow
pins to a commit SHA, and it holds pages: write plus id-token: write. All
four are now pinned.
- release.yml granted contents: write and actions: write workflow-wide. Both
are now job-scoped; actions: write reaches only publish, which needs it.
It also permits deleting the workflow runs and artifacts that constitute the
gate evidence.
- The release request was selected with `head -n 1` over path-sorted output.
Two request files in one push published only the alphabetically first, and a
push touching an old request file would attempt a version downgrade. It now
requires exactly one ADDED file and fails loudly otherwise.
- Added workflow_dispatch. prepare self-aborts once VERSION already matches, so
a failed publish previously had no re-entry point -- which is why v0.11.6
needed nine hand-written recovery commits.
- Added timeout-minutes to both jobs; publish holds a global concurrency slot
and could hold it for the six-hour default.
Tests and docs:
- Two new tests were vacuous. header-brand-layout used [\s\S]* spanning the
whole file, so deleting the property under test was satisfied by a copy of it
elsewhere; both are now anchored to the rule body. header-status-indicator
asserted drawerSource.includes("back"), which is unconditionally true because
the file contains callback, fallback and background.
- test_layout_regression pinned the no-op setAlignment call and guarded a
regression by exact local variable name, so reintroducing the footer button
as `open_btn` would have passed. It now asserts the user-visible string.
- Removed an assertion on a message that no longer exists anywhere in the repo.
- docs/contracts/CLIPBOARD.md described a manual-Copy flow the code does not
implement, and so failed to protect the one thing that keeps breaking. It now
documents the real mechanism, why the write must stay inside the gesture, the
status-string coupling, and the listener-ordering dependency.
verify_static_client returned FAIL on the committed tree before this change:
assets/main.js and assets/browser-support.js checksums were left stale by an
earlier commit. CI did not catch it because the gate runs npm run build first,
which rewrites both dist/ and the manifest. Regenerated, and the two new
clipboard-status files added, so the manifest now covers 95 files and passes.
NOT fixed, deliberately: release.yml still pushes the version bump to main
before any gate runs, which is what left v0.11.5 as a changelog entry with no
tag and no release. Fixing it means restructuring prepare/publish so the gate
runs against the prepared commit before it reaches main. That is a real
redesign of untestable-from-here automation that has already wedged main twice,
and it should be done deliberately rather than blind. The workflow_dispatch
added here at least provides the manual recovery path that was missing.
No node/npm or pytest available. dist/ was hand-applied and the manifest
regenerated. Executed directly and passing: 9 clipboard contract tests
(including a new one pinning the sentinel values), plus the clipboard
server-mode, toolbar layout, phase 9, desktop layout and Windows
creation-kwargs suites -- 26 fixture-free tests, 0 failures. Not run: anything
needing PyQt, node, or a browser. None of the UI changes have been seen
rendered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XfT5Z3Z24bjC8qtqERGpMQ
My own tightened assertion was wrong: the control names are unquoted
object keys (`back: 'navigation'`), so requiring surrounding quotes
failed for every key except 'running-app-icon'.
Both loops now extract the object literal they are about --
`actionPaths` and `controlActions` -- and match a key within it. That
fixes the failure and also closes the original hole properly: searching
the whole 600-line file made `includes("back")` unconditionally true via
callback/fallback/background, and a bare `${action}:` matched any object
key anywhere. Verified in both directions: every real key matches, and
`{ fallback: 1, callback: 2, background: 3 }` does not satisfy "back".
running-app-select is asserted separately since it is bound directly
rather than through the controlActions map.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XfT5Z3Z24bjC8qtqERGpMQ
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.
Second review pass over the v0.11.6/v0.11.7 range — the areas the first pass didn't reach, reviewed by four sequential workers. 19 of 30 confirmed findings fixed; one deliberately not, explained at the bottom.
The one that mattered most
verify_static_clientreturned FAIL on the committed tree before this change:assets/main.jsandassets/browser-support.jschecksums were left stale by an earlier commit. CI never caught it, because the release gate runsnpm run buildfirst — which rewrites bothdist/and the manifest, making the drift invisible in CI and real everywhere else.Clipboard write-through
The Copy button recognises the controller's reply by matching the status line against exact strings. Rewording one at a
setStatuscall site silently broke it — no exception, no console error, and a green suite, because the tests pinned only themain.tsside. Those four headlines now live insrc/clipboard-status.tsand both modules import them.Also fixed: the
execCommandfallback had nogenerationguard (a superseded copy stole focus and wrote over a newer request, and copied the live selection rather than the text it validated); the 1200 ms timeout overwrote real terminal statuses, replacing "Clipboard skipped — exceeds N KiB" with a misdiagnosis; Ctrl+C had no session guard, reinstating a message the controller deliberately suppresses; and the fallback now restores focus where it was instead of parking it on the canvas, which sends the next keystroke to Android.Drawer
openGroupforced focus to the canvas two frames after the Clipboard group opened. Its comment justified this bycontroller.tsfocusing the fallback textarea — a listener deleted when Ctrl+V moved to a document-level paste handler. The guard outlived its cause and now only steals focus from someone typing into the textarea, injecting their keystrokes into the phone.The
@importof-core.cssalso carried no?v=, leaving 531 lines of drawer layout outside cache-busting; a stale copy after upgrade renders every drawer group stacked and visible.Desktop
The Summary "Local URL" was a live hyperlink in every server state while the header Open button beside it was correctly gated — stop the server and a blue link leads to
ERR_CONNECTION_REFUSED. Plus_status_failedleft that Open button enabled indefinitely, missingQPushButton#headerIconButton:disabled/:pressedrules (QSS specificity is CSS2.1, so the id selector outranked the bare pseudo-class rules), asetFixedSize(38,38)fighting a 36px QSS box, and a no-opsetAlignment.Release automation
pages.ymlpinned four actions to mutable tags while every other workflow pins to SHAs, holdingpages: write+id-token: write. Now pinned.release.ymlhadcontents: write+actions: writeworkflow-wide (now job-scoped), selected the release request withhead -n 1over sorted paths (two requests published only the first; touching an old one would attempt a version downgrade), had noworkflow_dispatch, and no timeouts on a job holding a global concurrency slot.Tests
Three assertions were actively harmful and are fixed:
header-brand-layoutused[\s\S]*spanning the whole file, so deleting the property under test was satisfied by a copy elsewhere;header-status-indicatorasserteddrawerSource.includes("back"), unconditionally true (the file containscallback,fallback,background);test_layout_regressionguarded a regression by exact local variable name, so reintroducing the footer button asopen_btnwould have passed.Worth noting:
AGENTS.md:14andSTABILITY_CONTRACT.md:52both say "Prefer behavioral tests over source-text assertions" — and were added in the same range as the tests above.Not fixed, deliberately
release.ymlstill pushes the version bump tomainbefore any gate runs. That is what leftv0.11.5as a changelog entry with no tag and no release, and what forced nine hand-written recovery commits forv0.11.6. Fixing it means restructuringprepare/publishso the gate runs against the prepared commit before it reachesmain— a real redesign of automation that can't be tested from here and has already wedgedmaintwice. It should be done deliberately, not blind. Theworkflow_dispatchadded here at least restores the manual recovery path that was missing.Verification
No node/npm or pytest available.
dist/was hand-applied and the manifest regenerated (95 files, passing).Ran directly: 26 fixture-free tests, 0 failures — 9 clipboard contract tests including a new one pinning the sentinel values, plus clipboard server-mode, toolbar layout, phase 9, desktop layout and Windows creation-kwargs.
Not run: anything needing PyQt, node, or a browser. None of the UI changes have been seen rendered.
One trap found along the way:
.gitignore:125has a blanketdist/rule, so new build outputs are silently ignored while existing ones stay tracked. The two new files neededgit add -f.🤖 Generated with Claude Code
https://claude.ai/code/session_01XfT5Z3Z24bjC8qtqERGpMQ