fix: keep the daemon reachable on a refused change, classify local save errors, and make every string translatable - #20
Merged
Merged
Conversation
A received file that could not be written here (a full disk, a
read-only or missing download directory, a name too long for the file
system) was reported as the Tailscale daemon being unreachable, because
io.js mapped every Gio code as if it came from the socket.
Add REASON.LOCAL_FILE ("Could not save the file.") and move the Gio
code to REASON mapping into modules/errors.js as reasonForIoError,
which is told what the failed operation touched. saveFile now blames
the file for every create_async failure, for a splice_async failure
with NO_SPACE, READ_ONLY, PERMISSION_DENIED or NOT_FOUND, and for
running out of candidate names.
One refused PATCH (an HTTP error or an unusable answer) went through
#fail and marked the whole daemon unreachable: the subtitle said the
daemon refused the request and the tile read off, over a menu that was
otherwise working. It was also the only thing that set a flipped
switch back, so a second refusal with the same reason changed nothing
in the state and left the switch showing what the daemon refused.
#patch now calls #fail only for SOCKET_MISSING, CONNECTION_REFUSED,
UNKNOWN and PERMISSION_DENIED, the last so the operator row still
shows. Any other reason is recorded with applyRefusal, which sets
refusedReason and bumps refusedCount so that every refusal, a repeat
included, is a change the panel hears about; _syncOptions then sets
the switch back from the preferences that still hold, and the problems
section says why. applyPrefs clears refusedReason. #patch resolves to
{ error } with the reason, or '' once applied.
prefs.js handed gettext variables: every settings row title and subtitle from SETTINGS' own English, and every routes failure as messageFor's composed sentence. xgettext only extracts literals, so none of them could ever reach the translation template, and the i18n check never saw it because it scanned modules/ only. Move problemMessage into import-free modules/errors.js, which prefs.js can reach, and re-export it from menu-items.js for the menu sections. Give modules/settings.js a settingText(key, _) of literal _() calls, leaving SETTINGS with the key and its type. The i18n check now also reads prefs.js and extension.js. messageFor and problemOf's message field had no reader left but their own tests, so both are gone. AGENTS.md now says what the i18n check scans, no longer claims Vitest runs prefs.js, and names settingText in the settings-key procedure.
io.js is excluded from coverage on the grounds that it makes no decisions, but it still decided which HTTP status means a missing operator and whether a Content-Type is JSON, so neither was covered by anything but the live daemon check. Move them into pure functions with tests: reasonForStatus in modules/errors.js (2xx is no error, 401 and 403 are PERMISSION_DENIED, anything else HTTP) and isJsonContentType in modules/localapi.js, which now also owns JSON_TYPE. io.js keeps the Soup and Gio calls, and its header says what is left in it. The coverage comments in vitest.config.js and sonar-project.properties no longer claim the two exclusion lists are identical: vitest's also has tests/**.
A change that failed with the reason already recorded, such as CONNECTION_REFUSED while tailscaled is stopped or a 403 for a user who is not the operator, went through applyError into a state identical to the one before. No subscriber was told, so the switch that was flipped kept showing a value the daemon never applied, from the very first flip. Count every failed change in refusedCount, not only refusals. A transport failure now commits applyChangeError, which is applyError plus the count in one snapshot, so the panel always re-syncs and sets the switch back. refusedReason is still set only for a refusal.
…ections libsoup 3.6 allows two connections per host by default, and the IPN bus holds one of them for as long as the extension is enabled. One more long request beside it, such as a Taildrop send or a ping to a peer that never answers, took the other, and every /status, /prefs and change queued behind it until that finished. Raise max-conns-per-host and max-conns to 8 on the session. There is only one host, the Unix socket, so the two limits are the same. localapi-check now opens two bus streams against the real daemon and requires four parallel /status requests to answer within five seconds.
…ally Copy DNS name joined the display name to this tailnet's MagicDNS suffix. A node shared in from another tailnet, or one of Mullvad's, does not live under that suffix, so it copied a name that resolves nowhere, such as laptop.other-tailnet.example-tailnet.ts.net. A peer with no DNSName got one built from its HostName. normalizePeer now keeps dnsName, the peer's own DNSName without its trailing dot, or '' when there is none. The device section copies it and hides the row when it is empty, and sameNodes compares it so a changed name redraws the menu.
_loginRequested was cleared only when the menu opened the auth URL itself or the login request failed. A login that went through without the URL reaching the menu, such as `tailscale up` in a terminal, left it set, and the next AuthURL to appear for any reason, a reauth hours later, opened a browser nobody asked for. sync() now clears the flag whenever the backend no longer needs a login.
…lues
Every translated sentence was filled with .replace('%s', value), and a
replacement string reads $&, $` and $' as patterns. A received file
named draft$'s.txt was reported as saved to .../drafts.txt, and a node
or file name with $& or $` came out spliced with the sentence around
it. The chained .replace calls also rescanned what they had already
inserted, so a first value containing %s took the second value.
Add modules/text.js with fill(template, ...values), which imports
nothing and makes one pass with a function replacer. Its body is the
one QuickClip's listing.js and QuickMusic's panel.js carry, so the
copies agree. Every call site in modules/ and prefs.js uses it now,
including problemMessage in errors.js, which still imports nothing but
text.js. The _() arguments stay literals, so the i18n check is
unchanged.
derive() sorted the node list on every apply, so each preferences, profiles or error update, and every status read, allocated and sorted a new array that could not have changed order, and changed() then walked it field by field to find nothing had moved. Map first, and keep state.nodes itself when no node was re-marked: the list already arrives sorted, from normalizePeers or the last derive. sameBy now treats the same array as equal before comparing elements, so an unchanged list costs nothing to compare.
isValidBinding refused every Shift-only combination, so Shift+F5 was as unbindable as Shift+A even though F5 types nothing on its own. Adopt QuickClip's rule, which QuickTiler now shares: Shift alone is fine when the key's code point is absent or a control character, and refused otherwise. prefs.js passes Gdk.keyval_to_unicode(keyval) alongside the other Gdk/Gtk values the rule needs, and the docs site states the rule.
The capture dialog inhibited system shortcuts synchronously right after present(), which does nothing when the window has no surface yet, so a GNOME-bound combination could still fire instead of being captured. It restored them only on close-request. Inhibit on the dialog's map, through get_surface() and a Gdk.Toplevel check, and restore on unmap, the timing QuickTiler's prefs.js uses. Every way out of the dialog unmaps it. The comment no longer claims QuickClip's approach, which presents an in-window dialog instead.
An actor Clutter destroys from C never calls back into an overridden JS destroy() method, only the 'destroy' signal every actor emits either way. QuickTSToggle's destroy() override left the menu the Shell parents into the Quick Settings overlay, its rows' handlers and any pending re-measure behind whenever that happened. Connect to 'destroy' instead, QuickClip's pattern.
…cket README.md said that without the operator set the daemon refuses the socket, and comments in errors.js and health.js described an empty menu. The socket is open to every local user and reads succeed; what tailscaled refuses is every change, with a 403. Word it as AGENTS.md and the docs site already do.
The docs site's table of every module and what it imports predates modules/text.js. Add its row, and add text.js to the imports of errors.js, device-section.js, exit-node-section.js, menu-items.js, panel.js and taildrop-section.js, which now fill their translated sentences through it.
applyError and applyChangeError left refusedReason as it was. After a refused change, losing the daemon and then reading /status again brought back the "refused the request" row for a change nobody had made since. Both now clear refusedReason; applyPrefs still clears it on the next preferences read.
text.js said it lets errors.js stay import-free, while errors.js now imports text.js; say instead that errors.js imports nothing else. AGENTS.md said Vitest runs every module under modules/ as shipped, but io.js is not among them.
…s own The rule is shared with QuickClip and QuickTiler and resembles GNOME Settings' check without matching it exactly; the comments no longer claim it is the same rule.
…holders fill() fills placeholders positionally, so a translation that put %s before %d would swap the count and the device name. A Translators comment directly above the _n() call, which xgettext extracts, says to keep the count first.
|
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.
Fixes the QuickTS findings from a code-review sweep of the quick* extensions.
Bugs
model.js,state.js,panel.js):reachableand recordsrefusedReason, which the problems row shows. Only transport failures still count as lost contact.refusedCount, so a switch always flips back, even on a repeated identical failure or while the daemon is already unreachable.errors.js,io.js): a read-only, missing or full Downloads folder now reads "Could not save the file." (REASON.LOCAL_FILE), not a Tailscale install or operator problem.io.js): libsoup's default of 2 connections per host meant thewatch-ipn-buslong poll left one connection for everything else, so a big Taildrop transfer blocked toggles. The session now allows 8.scripts/localapi-check.jschecks parallel reads while long polls are open.DNSName, rather than rebuilding it. The rebuilt name was wrong for shared-in and Mullvad nodes.$expansion (newmodules/text.jsfill, byte-identical to QuickClip's and QuickMusic's). A file nameda$'b.txtnow renders as itself.i18n
problemMessageandsettingTexthold literal_()strings in import-free modules.prefs.jsuses them instead of_(variable), and the literal-string scanner now also readsprefs.jsandextension.js. The unusedmessageForandproblem.messageare gone.Other
derive()skips re-sorting the nodes when no node was re-marked.reasonForStatus,isJsonContentType).'destroy'signal.Verification
just cipasses: template-check, lint, 851 Vitest tests, 50 Playwright docs tests, the security scans and the build.just test-livepasses in a headless gnome-shell.just localapi-checkpasses against a live tailscaled.just prefs, capture a shortcut while pressing a GNOME-bound combo, close the dialog with Esc and with the close button, and confirm the system shortcuts still work.Noted, not fixed (pre-existing): if the Taildrop DELETE fails after the file was written (
model.jssaveFile), the saved path is discarded, and a retry saves "name (1)".