refactor(desktop): rename visual-smoke to e2e-fixture - #1314
Merged
Conversation
Mechanically rename the visual-smoke identifier family to e2e-fixture across files, directories, TypeScript identifiers, IPC channels, env vars, DOM data attributes, and docs, and refresh the stale "screenshot pipeline / auto-capture" rationale comments in the fixture code so they describe the fixture's current role: Playwright E2E seeding, the CI alignment audit, and interactive fixture use. The screenshot capture/baseline/diff layer was removed in #1308; this brings the surviving fixture mechanism's text in line with that reality. Casing follows existing repo convention (E2E_USER_DATA_DIR, E2E_SCENARIOS, createCuE2eFixture): E2E stays uppercase; the env family uses the MAKA_E2E_FIXTURE_* prefix to stay distinct from the generic E2E_* runtime vars. DOM dataset access is makaE2eFixture per the HTML spec kebab-to-camel conversion. No behavior change. Refs #1303.
Addresses codex review of #1314. - Rename the data-maka-visual-smoke CSS selectors in base.css and sidebar.css to data-maka-e2e-fixture so the deterministic-render hooks (decorative animation pause, running-status spinner stop) actually match the attribute the renderer writes. The first-pass rename grep had missed .css, so fixture renders silently went non-deterministic. - Add a contract test asserting the e2e-fixture CSS selectors exist. base.css is skipped wholesale by the !important audit (isA11yOnlyFile), which is why the missing selector was not caught. - Rename the leftover `smoke` local variables (the e2eFixture.getState() return values) to `fixtureState`, and update the two source-grep contract tests that asserted the old name. - Finish the stale-rationale sweep the first pass missed: Visual-smoke comments, fixture-context "captures / screenshot / baseline" wording, and the reduced-motion env name in base.css. Unrelated smoke terms (voice device smoke test, real-window smoke, prompt smoke) are left intact. No behavior change beyond restoring the CSS hooks the renderer already expected. Refs #1303.
…op dead allowlist Addresses a second fresh-eye codex review of #1314. - TS casing: rename E2EFixture* -> E2eFixture* to match the repo's existing TypeScript acronym convention (withE2eWindow, createCuE2eFixture, isE2e). SCREAMING E2E_FIXTURE constants/env stay uppercase; the DOM dataset makaE2eFixture already followed this rule. The prior E2EFixture introduced a second, inconsistent casing and clashed with the dataset read. - Comment accuracy: the first refresh had rewritten "screenshot" as "E2E/audit", but Playwright only binds 7 scenarios and many fixtures have no E2E spec at all. Reworded to the real consumer — the CI alignment audit, which covers every fixture — or to a neutral "the fixture renders X" where no external consumer exists (e.g. turn-control-history). - Residual sweep the prior passes missed: docs/frontend-css-governance (.md + .zh-CN.md), chat-empty-hero.tsx, stale-sessions.ts, the smokeGate/smokeWin test-local vars, and the "smoke-only" test title. - Revert one over-rename: use-onboarding-snapshot said "covered by smoke", meaning the Playwright first-run E2E (emptyWindow, no fixture), not the e2e-fixture. Now reads "Playwright E2E". - Drop the dead app-ipc-main.ts entry from check-console.mjs's allowlist; the file has no console.* and the entry referenced the removed capture tooling. No behavior change. Refs #1303.
Addresses a third fresh-eye codex review of #1314. The prior comment refreshes had replaced "screenshot" with "alignment audit" / "E2E" without checking which scenarios those consumers actually cover. The CI alignment audit iterates only 12 scenarios and Playwright binds only 6, so most fixture comments were naming a consumer that never runs them. Reworded every comment to one of: - the real consumer, where the scenario is actually covered (module-skills, module-mcp, plan-reminders, settings-bots-onboarding -> alignment audit; long-transcript -> scroll-geometry Playwright spec); - a neutral "the fixture renders/exposes X" with no consumer claim, everywhere else. Also cleared the last screenshot/baseline/PNG-diff residue the prior passes missed (chat.tsx, chat-empty-hero, the PNG-diff note in e2e-fixture.test, search-modal-lifecycle, turn-control-matrix, visible-copy-hygiene), the "visual fixture" term in three spots, and the "Visual smoke screenshots" reference in the real-window smoke runner (it contrasted the removed automated capture layer). 2761 desktop tests + format/lint/audit green. No behavior change. Refs #1303.
Lynskylate
pushed a commit
to Lynskylate/maka-agent
that referenced
this pull request
Jul 22, 2026
Spawn the user's login shell at Electron startup to recover a full PATH for apps launched from Finder/Dock/Spotlight (port of VS Code's shellEnv.ts). Review (apache#1316) feedback folded in: - short-circuit resolveShellEnv when launched from a terminal (TERM/COLORTERM) - drop the dead xonsh capture branch (intentionally unsupported) - shell-escape process.execPath per quoting context (POSIX/pwsh/nu) - strip XDG_RUNTIME_DIR from the resolved env (microsoft/vscode#22593) - preserve the Electron trio + MAKA_* via a prefix rule; drop MAKA_RESOLVING_ENVIRONMENT - export mergeEnv/buildCaptureCommand/buildMarkerRegex + add unit tests - E2E fixtures set MAKA_SKIP_SHELL_ENV=1 so the probe never taints the sanitized env (P1) Rebased onto main (incorporates apache#1314 visual-smoke -> e2e-fixture rename).
Astro-Han
pushed a commit
to Lynskylate/maka-agent
that referenced
this pull request
Jul 25, 2026
Spawn the user's login shell at Electron startup to recover a full PATH for apps launched from Finder/Dock/Spotlight (port of VS Code's shellEnv.ts). Review (apache#1316) feedback folded in: - short-circuit resolveShellEnv when launched from a terminal (TERM/COLORTERM) - drop the dead xonsh capture branch (intentionally unsupported) - shell-escape process.execPath per quoting context (POSIX/pwsh/nu) - strip XDG_RUNTIME_DIR from the resolved env (microsoft/vscode#22593) - preserve the Electron trio + MAKA_* via a prefix rule; drop MAKA_RESOLVING_ENVIRONMENT - export mergeEnv/buildCaptureCommand/buildMarkerRegex + add unit tests - E2E fixtures set MAKA_SKIP_SHELL_ENV=1 so the probe never taints the sanitized env (P1) Rebased onto main (incorporates apache#1314 visual-smoke -> e2e-fixture rename).
Astro-Han
added a commit
that referenced
this pull request
Jul 25, 2026
* fix(desktop): resolve login-shell environment at Electron startup Spawn the user's login shell at Electron startup to recover a full PATH for apps launched from Finder/Dock/Spotlight (port of VS Code's shellEnv.ts). Review (#1316) feedback folded in: - short-circuit resolveShellEnv when launched from a terminal (TERM/COLORTERM) - drop the dead xonsh capture branch (intentionally unsupported) - shell-escape process.execPath per quoting context (POSIX/pwsh/nu) - strip XDG_RUNTIME_DIR from the resolved env (microsoft/vscode#22593) - preserve the Electron trio + MAKA_* via a prefix rule; drop MAKA_RESOLVING_ENVIRONMENT - export mergeEnv/buildCaptureCommand/buildMarkerRegex + add unit tests - E2E fixtures set MAKA_SKIP_SHELL_ENV=1 so the probe never taints the sanitized env (P1) Rebased onto main (incorporates #1314 visual-smoke -> e2e-fixture rename). * fix(desktop): allow-list shell-env.ts console diagnostics in check-console PR #1316's test job failed the workspace console.* audit: shell-env.ts adds three startup diagnostics (capture-failure warn, shell-stderr debug, PATH-entry-count log). Add the file to the ALLOW map — it matches the sibling main-process diagnostic modules (config-file-watcher, daily-review, app-lifecycle) and leaks no secrets (only PATH count, shell name, error class). * fix(desktop): contain shell probe environment * fix(desktop): import only login shell PATH * docs(desktop): align shell PATH comments * fix(desktop): contain shell probe descendants --------- Co-authored-by: yiling <yiling@ebay.com> Co-authored-by: AstroHan <lei.yuhan@outlook.com>
9 tasks
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.
Summary
Closes prerequisite checkbox 2 of #1303. PR #1308 removed the Electron screenshot capture/baseline/diff layer; this PR brings the surviving fixture mechanism's text in line with that reality. Two textual changes, no behavior change beyond restoring CSS hooks the renderer already expected:
visual-smokeidentifier family toe2e-fixtureacross files, directories, TypeScript identifiers, IPC channels, env vars, DOM data attributes, CSS selectors, and docs. Casing follows existing repo convention (E2E_USER_DATA_DIR,E2E_SCENARIOS,createCuE2eFixture):E2Estays uppercase, the env family usesMAKA_E2E_FIXTURE_*to stay distinct from genericE2E_*runtime vars, and DOM dataset access ismakaE2eFixtureper the HTML spec kebab-to-camel conversion.audit-alignment.mjsaudit, and interactive fixture use. Historical PR markers (PR-IR-01b,PR108j, etc.) are preserved.Out of scope (tracked separately): #1311, #1312, #1304.
Verification
All green locally:
npm run build(full workspace) — passesnpm run format:check+npm run lint(Biome) — cleannpm --workspace @maka/desktop run test:dist— 2761/2761 pass (includes a new contract test asserting the e2e-fixture CSS selectors exist)node scripts/run-workspace-tests-parallel.mjs— 1169/1169 passnode scripts/audit-alignment.mjs— all fixtures cleanscroll-geometry,settings,permission-takeover,locale-renderer,bot-onboarding) — 15/15 passnpm run build-storybook— passesCodex review follow-up
A pre-merge codex review (high effort) flagged the first pass as incomplete. The second commit addresses every accepted finding:
.css, so thedata-maka-visual-smokeselectors inbase.css/sidebar.csswere not renamed while the renderer wrotedata-maka-e2e-fixture. Deterministic-render hooks (decorative animation pause, running-status spinner stop) silently stopped matching. Renamed the selectors and added a contract test that asserts they exist (the!importantaudit skipsbase.csswholesale viaisA11yOnlyFile, which is why the gap was not caught).smokelocal variables (thee2eFixture.getState()return values) renamed tofixtureState, with the two source-grep contract tests that asserted the old name updated. RemainingVisual-smokecomments and fixture-context "captures / screenshot / baseline" wording swept. Unrelated smoke terms (voice device smoke test, real-window smoke, prompt smoke) left intact.app-ipc-main.tsentry incheck-console.mjs's allowlist references the removed capture tooling. The file no longer has anyconsole.*, so the entry is dead, but allowlist cleanup belongs to the refactor: remove the screenshot capture layer #1308 capture-layer removal rather than this rename, and another entry (main-window.ts) still covers real real-window smoke diagnostics. Tracked for a PR1 follow-up.