refactor(ui): view lifecycle, accessible dialogs, and one implementation per control - #28
Closed
vietanhdev wants to merge 9 commits into
Closed
refactor(ui): view lifecycle, accessible dialogs, and one implementation per control#28vietanhdev wants to merge 9 commits into
vietanhdev wants to merge 9 commits into
Conversation
Nothing proved the packages RUN. 'npm run tauri build exited 0' and 'the .deb is 8MB' are both true of a binary that dies before it draws a window -- and this app builds a tray icon unconditionally against libayatana-appindicator3, the classic dlopen-panic shape. The hard part is that ThinkUtils reads /proc/acpi/ibm/fan and /sys/class/power_supply/BAT*, none of which exist in a container. So the suite does not test hardware paths. It tests that the app starts, renders its full UI, and degrades cleanly when the hardware is absent, using three hardware-independent signals: index.html ships only two visible strings plus empty containers. Every other word on screen arrives because templateLoader.js fetched a template over tauri:// and injected it. Those labels are literal markup that no /proc or /sys read produces, so OCR finding them proves the JS ran, on any machine. OCR finding index.html's static text while finding NO injected label is the exact signature of 'WebKit loaded the page, the JS died' -- checked explicitly, because every other assertion passes in that state. The app now prints hw probe / hw mode, so 'no ThinkPad here' is an observed state rather than an inference. The frontend reports uncaught exceptions to the backend. A view dying on an absent sysfs path leaves the sidebar painted and the process alive; that error line is the only tell. Running it immediately found two real bugs, both fixed here: hw mode reported 'full' inside a container. Containers inherit the host's /sys, so bat0 and cpufreq were present and only the fan interface was missing. Keyed on the fan interface now -- a battery and cpufreq exist on every Linux laptop and prove nothing about ThinkPad support. The OCR assertion missed the first-run permissions dialog. On a machine that has never been set up -- every container -- the app correctly opens that dialog over the main view, so the sidebar is not what is on screen. It comes from templates/dialogs.html, so it is injected template text and proves the same thing. Verified locally against real builds: deb on ubuntu:24.04 PASSES, and rpm on fedora:41 PASSES -- the first time the .rpm has ever been tested by anything. Artifact selection resolves the CURRENT version rather than taking the only match or the first: three stale builds were sitting in the bundle directory, and release.yml selects with 'ls *.deb | head -n 1', which sorts 0.1.10 before 0.1.5 and would rename an old package to the new version's name and publish it. Also fixes .gitignore, where a missing newline had merged two entries into 'docs/.vitepress/cachebuild/' -- so neither the VitePress cache nor build output was ignored.
The Download nav link pointed straight at the GitHub releases list, which shows every asset for every version and leaves the reader to work out which file they want. docs/download.md resolves the latest release through the GitHub API at view time rather than baking a version in at build time -- the docs site and the release pipeline deploy independently, so a hard-coded version would go stale the moment a release ships without a docs rebuild. If the API is unreachable or rate-limits (60/hour unauthenticated), every button falls back to the releases page, which always works. Assets are matched by predicate rather than exact filename so a version bump needs no edit here, and each predicate pins the architecture suffix. That is load-bearing rather than tidiness: find() returns the FIRST match, so a loose predicate would silently hand out the wrong package the day a second architecture is added, and the failure is quiet and user-side -- the page looks right, the download works, the package refuses to install. getting-started.md is restructured as an ordered path rather than a list of prerequisites, and leads with the question a reader actually has -- 'will this work on my machine' -- answered by one command. Step 2 is now stated as the step people miss, because it is: the thinkpad_acpi module refuses every fan write unless loaded with fan_control=1, and that is fixed at module load, so granting permissions cannot fix it. Presenting it as one prerequisite among several is what produces the 'I granted permissions and nothing happened' report. Also documents the Ubuntu 22.04 polkit 0.105 limitation, which silently makes passwordless fan control not work there, and the fan safety behaviour -- revert on disable, on unreadable sensors, on exit, plus the firmware watchdog.
…anifests
Distro packaging was blocked by one thing: the fan helper installed to
/usr/local/bin at runtime. Debian Policy 9.1.2 and the Fedora guidelines
both forbid a package writing there, and a helper materialised by a
button click is not package-owned -- dpkg -L would not list the most
security-sensitive file the app uses, and uninstalling would leave a
root-owned binary and a polkit rule behind.
HELPER_PATH becomes HELPER_CANDIDATES, searched in order:
/usr/lib/thinkutils/... Debian and Arch convention
/usr/libexec/thinkutils/... Fedora convention
/usr/local/bin/... legacy self-install, kept so existing
installs keep working
setup_permissions() now skips installing the helper and rule entirely
when helper_is_packaged(), because overwriting those files puts the
package database out of sync with the filesystem.
The path had been duplicated in four places -- fan_control, mcp,
fan_curve, and inside the polkit rule text. The rule is now generated
from the constant, since a rule naming a path the helper is not at
grants nothing while looking correct.
Two security improvements fall out of generating it:
subject.local && subject.active is now required. Without it any SSH
session belonging to a wheel/sudo user inherited passwordless
hardware control, as did a background session the user had switched
away from.
Packages ship the rule to /usr/share/polkit-1/rules.d, not /etc.
/etc is the administrator's namespace; a package writing there
shadows their rules and is never cleaned up.
packaging/ adds the AUR PKGBUILD and COPR spec, plus the helper and rule
as generated artifacts (cargo run --example gen-packaging).
tests/packaging.rs is the point of all this: 7 tests asserting the
committed files match what the source generates, that each format
installs where the app actually searches, that nothing writes to
/usr/local, that the rule goes under /usr/share, and that versions
agree. The drift they prevent fails SILENTLY -- polkit denies, the app
falls back to a password prompt, and it reads as a permissions problem
rather than a packaging bug.
bump-version.sh now covers PKGBUILD and the spec too, and ci.yml calls
it rather than keeping its own copy of the file list, so adding a
packaging file cannot leave CI checking a stale subset.
Suite: 80 -> 87.
Closes the other half of the local privilege-escalation chain. The first half -- unvalidated governor into a root shell -- was fixed earlier; this is the part that let a local user reach it. monitor.js rendered proc.name straight into innerHTML, and that string is the COMMAND column of `ps aux`. Any local user can name a binary `<img src=x onerror=...>`. With csp:null and withGlobalTauri:true, the injected script got the full __TAURI__ API -- including commands that end in pkexec. escapeHtml existed but was private to security.js, so every other view rendering external strings had none. It moves to utils.js and is applied to process names and status, disk mount points and devices, network interface names, battery strings, and sensor labels. The CSP replaces null with default-src 'self'; script-src 'self'; object-src 'none'; frame-ancestors 'none'. script-src deliberately has no unsafe-inline or unsafe-eval, which would defeat the point. style-src does allow unsafe-inline, because the templates use inline style attributes -- verified rather than assumed. Verified by building the real packages and running the container launch test: the frontend still fetches its 12 templates, injects them and paints. A CSP that broke template loading would have looked identical to a working one in unit tests. Tests: csp_is_set_and_restrictive asserts the directives and that script-src stays strict; views_escape_untrusted_strings asserts escapeHtml is shared and that proc.name specifically is escaped.
Five call sites each had their own copy of: build a script, write it to a predictable /tmp path with plain fs::write, chmod it, hand it to pkexec bash. The copies had drifted, so only some had either fix. fs::write on a predictable path follows symlinks and will happily open a file another user pre-created. auth.rs was the worst: /tmp/thinkutils_auth.sh, a fixed name with no randomness at all, so any local user could plant that path and have their content executed as root. privileged::run_script() replaces all of them. Creation is O_EXCL with a random name and mode 0600, which fails rather than following a symlink or reusing a planted file, and the script is always removed -- including when pkexec fails to launch, which several copies leaked. Migrated: performance.rs governor/turbo/boost, battery.rs thresholds, auth.rs, and fan_control.rs's own fallback. fan_control's create_secure_temp_script is gone; it was a second implementation of the same idea, which is how the drift started. Honest about what this does not fix: the file is owned by the invoking user between write and root execution, so that user could swap its contents. That matters only where an administrator authenticates on behalf of a less-privileged user, and closing it means not handing root a user-owned script at all -- the shape the fan helper already uses. Said so in the module docs rather than implying the problem is gone. security.rs also calls pkexec but passes arguments directly with no script file, so it has no equivalent exposure. Tests: mode is 0600, consecutive calls get distinct paths, and create_new refuses an existing path -- the last being the property that actually defeats the planted-file attack.
…th port Two silent failures, both from the same cause: the same thing named in two places, drifting apart. BATTERY THRESHOLDS permissions.rs granted write access to charge_start_threshold and charge_stop_threshold, while battery.rs wrote charge_control_start_threshold and charge_control_end_threshold. On a ThinkPad BOTH pairs exist and report the same value -- confirmed on hardware, both 75/80 -- but they are separate sysfs files, so a chmod on one never affected the other. The result: 'Grant Permissions' reported success and battery thresholds stayed unwritable, so every change fell through to a password prompt with no explanation. mcp.rs named a third variant. battery::threshold_paths() is now the single source of truth, preferring the generic kernel names and falling back to the thinkpad_acpi spelling. permissions.rs and mcp.rs both go through it. Also removed /sys/devices/platform/thinkpad_hwmon/pwm1 from the required list: that path does not exist. The real attribute is under .../thinkpad_hwmon/hwmon/hwmonN/pwm1, and the exists() guard meant the wrong path was skipped rather than reported. It is discovered now. PORT COLLISION The MCP server defaulted to 8765, which is the port sync.rs binds for the OAuth callback. With MCP running the callback listener could not bind, so Google sign-in never completed and nothing said why. MCP moves to 8779. It was the one to move: its port is local config, while the callback port is registered as the redirect URI in Google Cloud Console and cannot change without updating the OAuth client. Tests pin both: that the two ports differ, that REDIRECT_URI still embeds the callback port (it is a literal, since a const cannot call format!), that the generic attribute names are preferred, and that a candidate pair never mixes naming schemes -- writing a generic start with a legacy stop would touch two different files. Docs and the MCP view updated to 8779, with a note explaining the change for anyone who configured a client against the old port.
navigation.js held a 9-branch hide block, a separate titles map, and a 9-case show switch. The titles map and the view templates had already drifted -- the MCP subtitle differed between them -- and every view repeated its own title and subtitle directly under the page header that already showed both. views/registry.js is now the single source of truth: id, title, subtitle, element, display mode, onShow, onHide. navigation.js reads it and is ~80 lines shorter. The duplicated headers are gone from seven templates. The missing piece was hiding. Nothing was ever torn down, so timers were either global-forever or had to re-check currentView on every tick. The fan sensor poll did neither: it started at app launch and polled /proc every second for the life of the process, on any view, on a battery utility. It now starts when the fan view is shown and stops when it is left. Monitor gains the same treatment, and the home refresh interval is tracked in state -- beforeunload listed two of three timers while reading as though it were complete. Dialogs were plain divs toggled with style.display. The About dialog registered a fresh Escape listener on document every time it opened but removed it only inside the Escape branch, so closing via the X button or the overlay left it attached -- open it five times and five handlers fired on the next Escape. The permission dialog had no Escape handler at all, which made it impossible to dismiss from the keyboard. dialog.js replaces both with one implementation: role=dialog, aria-modal, a Tab trap, Escape on the capture phase, focus moved in on open and restored on close, and re-opening an already-open dialog returns the existing closer rather than stacking handlers. Also: aria-current on the active sidebar item, since a CSS class says nothing to assistive technology, and role/aria-live on the status banner, which meant every success and error message was previously unannounced. Verified by building the real packages and running the container launch test -- a broken registry would have left the app painting nothing. ci: the launch-test container was missing jq, so VERSION resolved empty and every glob became thinkutils__amd64.deb, which matches nothing. The run reported 'no artifact' while the real cause stayed hidden. jq is installed now, and an unreadable version fails loudly instead of producing a pattern that silently matches nothing.
…anels Home and Performance implemented the same three controls twice, and the copies had drifted: Home disabled its governor buttons during the call and waited 500ms before reading back; Performance did neither, so a fast double-click could fire two governor changes -- each spawning a pkexec -- and the read-back could land before the kernel had applied the first. Home rebound its turbo handler on every render without removing the old one, so the toggle fired once per previous render. Performance bound once. The two used different wording for the same successful action. hardwareControls.js is now the single implementation. runAction() gives every privileged call the same status reporting, disables the controls it is told are busy, and always refreshes -- so a failure cannot leave the UI showing a state that was never applied. A failed turbo write flips the checkbox back, which only one of the two copies did. security.js carried two near-identical log panels, ~230 lines differing only in element prefix and reveal delay. They also shared a bug: each scheduled one setTimeout per log line and nothing cancelled them, so starting a second scan cleared the output while the first run's timers kept firing into it. A 2000-line scan queued 2000 timers spanning a minute. logPanel.js replaces both and cancels pending reveals on start and update. Lines are set with textContent -- they carry scanner output and file paths -- and the collapse button now reports aria-expanded. security.js 675 -> 469 lines; performance.js 125 -> 85. Verified with the container launch test rather than by inspection: a broken control binding still paints a full UI, so only running it proves anything.
The descriptions restated the system's own vocabulary -- 'Control CPU frequency scaling policy', 'System-wide power management profile' -- which tells someone who already knows what a governor is nothing they did not know, and someone who does not know nothing at all. Rewritten around what the user controls and the tradeoff they are making: powersave is quieter and lasts longer, performance responds faster and runs hotter. Turbo boost off caps peak speed but noticeably reduces heat and fan noise. Battery thresholds trade runtime per charge for battery lifespan, with the practical note to raise the limit before travelling. Fan control had no explanation at all, and it is the one that can damage hardware. Two additions: The control mode section now states that manual control overrides the firmware's thermal management, and that the app hands the fan back on mode change, on sensor failure and on exit, with the firmware watchdog armed meanwhile. That behaviour exists but was invisible, so users had no reason to trust manual mode. The slider says levels are 0-7 rather than a percentage -- the firmware picks the RPM -- and that level 0 stops the fan entirely, which is safe only at light load. Verified the templates still inject and paint via the container launch test; a malformed template would leave the view empty.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
thinkutils | 17a0ea7 | Jul 19 2026, 03:07 PM |
This was referenced Jul 20, 2026
Owner
Author
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.
Four related pieces of frontend work. All verified with the container launch test, not by inspection — a broken binding still paints a full UI, so only running it proves anything.
View lifecycle (#11)
navigation.jsheld a 9-branch hide block, a separate titles map, and a 9-case show switch. The titles map and the templates had already drifted — the MCP subtitle differed between them — and every view repeated its title and subtitle directly under the page header showing both.views/registry.jsis now the single source of truth. Duplicated headers removed from seven templates.The missing piece was hiding. Nothing was torn down, so timers were either global-forever or re-checked
currentViewevery tick. The fan sensor poll did neither — it started at launch and polled/procevery second for the life of the process, on any view, on a battery utility. It now starts on show and stops on hide.Accessible dialogs (#10)
The About dialog registered a fresh Escape listener on
documentevery open but removed it only inside the Escape branch — closing via the X or overlay left it attached, so five opens meant five handlers on the next Escape. The permission dialog had no Escape handler at all, making it keyboard-inescapable.dialog.jsreplaces both:role=dialog,aria-modal, Tab trap, Escape on capture, focus moved in and restored on close. Plusaria-currenton the active sidebar item andaria-liveon the status banner — every success and error message was previously unannounced.One implementation per control (#12)
Home and Performance implemented the same three controls twice, drifted:
Without the disable, a fast double-click fired two governor changes — each spawning a .
security.jsalso carried two near-identical log panels (~230 lines, differing only in ID prefix and delay) sharing a bug: each scheduled onesetTimeoutper line and nothing cancelled them, so a second scan cleared the output while the first run's timers kept firing into it. A 2000-line scan queued 2000 timers.security.js675 → 469 lines;performance.js125 → 85.Explanatory copy (#14)
Descriptions restated the system's vocabulary. Rewritten around the tradeoff being made. Fan control had no explanation at all — it now states that manual control overrides firmware thermal management, that the app hands the fan back on mode change, sensor failure and exit with the watchdog armed meanwhile, and that level 0 stops the fan entirely.
CI fix
The launch-test container lacked
jq, soVERSIONresolved empty, every glob becamethinkutils__amd64.deb, and the run reported "no artifact" while hiding the real cause. Now installed, and an unreadable version fails loudly.