chore: sync the shared template, fix a handler leak and translation strings, and align the docs - #19
Merged
Merged
Conversation
Copy the files every extension keeps identical (template.list, template.sha256, scripts/template-check.sh), add project.just for the recipes only QuickTS needs, and add tests/docs.config.js for the config-driven docs.spec.js. Merge scripts/headless-check.sh by hand onto the recommended hook frame, noting whether tailscaled was reachable during the run rather than asserting on it — that is scripts/localapi-check.sh's job, run separately via live-extra. Update labelsOf in tests/support/panel.js for the stub change that gives every PopupSeparatorMenuItem a text of '' instead of leaving it undefined: an empty string is still "no label" for a separator neither modules/navigable-section.js nor modules/panel.js gives one to.
_remeasureOnceLaidOut disconnected whatever id was in this._allocationId at fire time rather than the id it was itself given. A second request before the first fired overwrote that field, so the first handler's own disconnect call released the second handler instead of itself and stayed connected to 'notify::allocation' forever. _onOpenStateChanged always cancels a pending remeasure before starting another, so this needs two direct, overlapping requests to show — a future call site that forgets to, or two opens racing the cancel.
modules/panel.js's problem row and subtitle, and modules/taildrop-section.js's ineligible-node reason and failed-save message, all passed a variable straight to gettext: the already-composed English from modules/errors.js's messageFor or modules/taildrop.js's reasonFor. Neither ever appears as a literal in a _() call, so xgettext can never extract it, and the string looked fine under the identity gettext this suite runs against while carrying no msgid at all. Add menu-items.js's problemMessage(reason, _), switching on REASON to a literal per case, and taildrop-section.js's taildropReason(status, _), switching on TaildropTargetStatus the same way. modules/health.js's problemOf now also returns the raw reason for panel.js to switch on, and modules/model.js's saveFile returns a REASON code instead of a composed message, for taildrop-section.js to translate the same way. tests/support/i18n.js's extractableMsgids() scans modules/ for every literal actually reachable by _() and _n(), and three new tests spy on gettext through each of these paths and assert every message it was asked to translate is one of them. modules/device-section.js:194 has the identical `_(result.error)` bug over modules/model.js's ping() error, left alone: it is out of this fix's stated scope and touching model.js's ping() return shape would need a matching change there too.
Super, Print SysRq and the like reached GNOME itself instead of the capture dialog, which had no way to offer or refuse them. Same approach as quickclip's prefs.js: inhibit_system_shortcuts on the dialog's surface once presented, feature-checked since only a Gdk.Toplevel surface has it, and restored on 'close-request' — which fires on every way out, since Escape, Backspace and an assigned key all call dialog.close(). No new test: prefs.js is Adw and Gtk widget construction, excluded from this suite's coverage (vitest.config.js) because there is no gi://Gtk or gi://Adw stub to test it against — asserting against one would test the stub, not this code. quickclip's own inhibitSystemShortcuts is untested for the same reason.
problemMessage and taildropReason had branches no existing scenario drove: menu-items.js and taildrop-section.js dropped to 90% and 92% branch coverage after the previous commit added them. Test each directly over every REASON and TaildropTargetStatus, the same way tests/errors.test.js and tests/taildrop.test.js already cover messageFor and reasonFor. Export taildropReason for the same reason. Coverage: 98.79%/95.61% (stmts/branches) before this commit, 99.56%/97.07% after.
modules/device-section.js's ping row passed model.js's ping() result straight to _(result.error): composed English from messageFor for a transport failure, and modules/ping.js's own 'No response'/'No reply' markers for the other two cases — none of them a literal in the source. modules/ping.js's describePing now returns an untranslated `issue` alongside `error`, naming which of PING_ISSUE.NO_RESPONSE/NO_REPLY it is, or '' when `error` is the daemon's own text (still shown as data, never translated) or empty (a success). modules/model.js's ping() sets issue: PING_ISSUE.TRANSPORT and error: reasonOf(error) — a REASON code, as saveFile's error already is — for the one case describePing itself cannot see: the request to tailscaled failing outright. Drop the now-unused messageFor import. modules/device-section.js's row keys off `issue`: problemMessage for TRANSPORT, a literal _() for NO_RESPONSE/NO_REPLY, the raw text otherwise. Extractability test added through the ping path with REASON.PERMISSION_DENIED — the one reason whose composed message differs from its own literal, so a regression back to `_(result.error)` cannot hide behind text that happens to read the same either way.
tests/taildrop-section.test.js's save and ineligible-node scenario tests passed even with `_(error)`/`_(reason)` restored, because every REASON and every TaildropTargetStatus except REASON.PERMISSION_DENIED composes to text identical to its own literal — a scenario test comparing output under `gettext: message => message` cannot tell `_(reason)` apart from a switch that happens to choose the same string. tests/support/i18n.js's nonLiteralGettextCalls() scans modules/ for every _()/_n() call whose argument is not a string literal, the same authority extractableMsgids already relies on for the reverse question. tests/i18n.test.js asserts there are none. Verified against the pre-fix sources at b90edf7: it finds all four — taildrop-section.js:79, taildrop-section.js:248, panel.js:311, panel.js:712 — plus device-section.js:194, which the scenario tests never covered at all. The save test also switches to REASON.PERMISSION_DENIED, so it does not depend on the source check alone to catch a regression.
With the handler leak fixed, two overlapping _remeasureOnceLaidOut requests both now fire and each schedules its own BEFORE_REDRAW later — but both still wrote the id through the shared this._laterId, so the second overwrote the first's there too. A _cancelRemeasure() landing between the fire and the next redraw (the menu closing again before BEFORE_REDRAW runs) would then remove only the later this._laterId still named, leaking the first's forever — the same shape of bug as the handler leak, one field later. _remeasureOnceLaidOut now starts with _cancelRemeasure(), so a second request disconnects and unschedules the first before it ever gets the chance to fire: at most one handler and one later are ever outstanding. The per-request captured allocation id stays as defense in depth for a future call site that skips _cancelRemeasure the way this one used to. Test extended to assert this._allocationId is 0 and exactly one later is scheduled right after the (now single) handler fires, before draining it.
Said this script has to pass without Tailscale installed so that 'just test-live' still runs without one — true of this script alone, false of test-live as a whole, which also runs scripts/localapi-check.sh through live-extra and fails outright without a reachable daemon. Also said localapi-check.sh "skips itself with a clear FAIL", which names two different things: it exits 1, it does not skip.
tests/model.test.js's two saveFile failure tests asserted only result.error.toMatch(/\S/), which the old messageFor(...) text and the new REASON code both satisfy — neither pins the shape the previous commit's fix actually returns. Assert the exact REASON instead. tests/health.test.js gains a direct assertion on problemOf().reason, alongside the existing ones on .message and .actionable. modules/health.js's JSDoc for problemOf now describes `reason` and drops the "untranslated for a log" phrasing: nothing here logs it, and modules/menu-items.js's problemMessage is what actually reads it.
nonLiteralGettextCalls() only ever looked at a call's first argument, so
_n('one thing', pluralVar, n) passed clean even though pluralVar is exactly
as invisible to xgettext -k_n:1,2 as a non-literal first argument is.
Also accept a double-quoted literal, which is just as extractable as the
single-quoted style this codebase happens to use.
"quick settings" appears lowercase in a translatable string (prefs.js, the gschema's shortcut description) and in several module and test comments. Quick Settings is capitalized everywhere else in this codebase; make these consistent with it.
…ance pass - README.md and metadata.json's description now open with the one-liner shared across every Ghost Assembly listing of QuickTS; README gains the Wayland log-out note next to the install snippet, and "## Develop" becomes "## Development" to match the other extensions. - docs/index.html and tests/docs.config.js's `site` move from the old ghost-assembly.github.io host to ghost-assembly.com (the extension uuid, a different string, is untouched); the tagline and og:description now carry the one-liner too. - The "Keyboard shortcut" section is renamed "Keyboard", in the TOC (both copies), the Preferences table's cross-reference and the heading itself. - The Architecture section named only 10 of the 27 modules under modules/; a new "Every module" table lists all 27, with each one's job and which other modules it imports. - The release example now uses `git tag -a` with a commit message and an unreleased version (v0.1.1), matching the other extensions' docs.
The rules an agent working in this repository must not break: the LocalAPI contract, the Tailscale-operator requirement, Taildrop's file-name and exclusive-create guarantees, the every-string-is-a-literal gettext check, and which files template-check locks. CLAUDE.md is exactly `@AGENTS.md`.
|
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.
Code
just template-checkguarding drift;localapi-checkmoves toproject.justand runs throughtest-live._remeasureOnceLaidOutno longer leaks a handler (or a scheduled later) when two remeasures overlap.tests/i18n.test.js) fails on any_()/_n()call with a non-literal argument.Docs
Verification
just cigreen (template-check, 754 unit tests, 50 docs tests, security scans, build);just test-livePASS against a real tailscaled (headless-check, pack-check, localapi-check).