fix(desktop): omit invalid dashboard web dist env - #39554
Conversation
austinpickett
left a comment
There was a problem hiding this comment.
Hermes Agent Review — ✅ Approve
Verified locally vs origin/main; ran the new test suite (5/5 pass).
Correct semantic fix. On origin/main, resolveWebDist() falls back to APP_ROOT/dist, which under asar:true is a path inside app.asar — not a real filesystem dir the Python dashboard backend can serve over HTTP — so HERMES_WEB_DIST got set to an unservable path, causing static-route 404s (refs #41327/#39472). The new resolveDashboardWebDist returns null in that case and applyDashboardWebDist then deletes HERMES_WEB_DIST from the spawn env (including any inherited value), letting the backend use its own default.
Key correctness points I confirmed:
- Wiring updated at both spawn sites (
spawnPoolBackend,startHermes); both old inlineHERMES_WEB_DIST: webDistassignments removed. - Renderer path deliberately untouched —
resolveWebDist()/resolveRendererIndex()keep theAPP_ROOT/distfallback, which is correct because Electron's renderer can load index.html from asar while the HTTP backend cannot. The asar-OK-for-renderer / asar-forbidden-for-backend split is the right distinction. - Good guards: override
path.resolve'd + existence-checked;unpackedPathForregexapp\.asar(?=$|[\\/])correctly anchors the asar segment.
node --test apps/desktop/electron/web-dist.test.cjs => 5 pass, 0 fail
including the critical "does NOT fall back to asar-internal dist → null" case and both env strip/set cases. node --check clean on web-dist.cjs + main.cjs; test registered in test:desktop:platforms. No out-of-scope hunks (exactly the 4 declared files, +129/-11), no security surface.
Optional nit (non-blocking): origin/main's resolveWebDist logs a rememberLog warning when it lands on an asar-internal path; the new resolver returns null silently. A one-line breadcrumb when both override+unpacked tiers miss would aid future debugging. Not required.
Cluster note: same desktop module-extraction family as #37471/#38292/#38589/etc. — textual conflicts on main.cjs/package.json expected, no semantic overlap.
Reviewed by Hermes Agent (local node --test + origin/main verification).
353751e to
13452fe
Compare
13452fe to
e8df7f8
Compare
|
Thanks for the focused desktop hardening work. This is an automated hermes-sweeper review; the reported packaged-dashboard failure is already addressed on current
The fixing commit is contained in release |
Summary
HERMES_DESKTOP_WEB_DISTandapp.asar.unpacked/distonly when they exist as normal directoriesHERMES_WEB_DISTwhen no usable backend dist exists, sohermes_cli.web_servercan fall back to its packaged defaultresolveWebDist()path unchanged for Electron's own ASAR loadingFixes #39472.
Validation
node --test electron/web-dist.test.cjsfailed withCannot find module './web-dist.cjs'node --test electron/web-dist.test.cjsnpm run test:desktop:platformsnpx eslint electron/main.cjs electron/web-dist.cjs electron/web-dist.test.cjsnode --check electron/main.cjs; node --check electron/web-dist.cjs; node --check electron/web-dist.test.cjsnpm run type-checkgit diff --check