feat(ci): add windows arm64 e2e#1653
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds Windows ARM64 E2E support: packaged-app Playwright fixture, CI-specific Playwright config and npm script, ARM64 runtime injector changes, a manual ChangesWindows ARM64 E2E Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/features/windows-arm64-support/spec.md (1)
18-20: 💤 Low valueConsider varying sentence structure in the Non-Goals section.
All three non-goal items begin with "Do not," which slightly reduces readability. While acceptable for a bulleted list, consider rewording for variety.
✍️ Optional rewording suggestion
## Non-Goals - Do not enable Windows ARM64 in the production build/release matrix until the manual Windows ARM64 E2E workflow has passed. -- Do not guarantee every optional runtime is bundled on Windows ARM64. -- Do not use real provider API keys in CI. +- Not every optional runtime is guaranteed to be bundled on Windows ARM64. +- Real provider API keys must not be used in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/windows-arm64-support/spec.md` around lines 18 - 20, The three bullets in the Non-Goals section all start with "Do not," making the list repetitive; rephrase each item to vary sentence structure while preserving intent — e.g., change "Do not enable Windows ARM64 in the production build/release matrix until the manual Windows ARM64 E2E workflow has passed" to "Enable Windows ARM64 in the production build/release matrix only after the manual Windows ARM64 E2E workflow has passed," change "Do not guarantee every optional runtime is bundled on Windows ARM64" to "There is no guarantee that every optional runtime will be bundled on Windows ARM64," and change "Do not use real provider API keys in CI" to "Use mock or scoped provider API keys in CI rather than real keys"; apply these rewordings in the Non-Goals section and keep original meaning and negative constraints intact.docs/features/windows-arm64-support/plan.md (1)
5-7: 💤 Low valueConsider varying sentence structure in the Architecture section.
All three architecture bullet points begin with "Add," which slightly reduces readability. Consider restructuring for better flow.
✍️ Optional rewording suggestion
## Architecture -- Add a Windows ARM64 manual workflow that runs on GitHub's `windows-11-arm` runner and validates a packaged/unpacked ARM64 build with Playwright Electron smoke tests. -- Add a small runtime installer wrapper for Windows ARM64 that calls `tiny-runtime-injector` per runtime and treats failures as skipped optional artifacts. -- Add a CI-specific E2E mode that starts a local OpenAI-compatible mock server, injects a provider/model through existing typed routes, completes onboarding, and runs the existing smoke specs against an isolated profile. +- A Windows ARM64 manual workflow runs on GitHub's `windows-11-arm` runner and validates a packaged/unpacked ARM64 build with Playwright Electron smoke tests. +- A small runtime installer wrapper for Windows ARM64 calls `tiny-runtime-injector` per runtime and treats failures as skipped optional artifacts. +- A CI-specific E2E mode starts a local OpenAI-compatible mock server, injects a provider/model through existing typed routes, completes onboarding, and runs the existing smoke specs against an isolated profile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/windows-arm64-support/plan.md` around lines 5 - 7, Reword the Architecture section to vary sentence openings instead of repeating "Add" for each bullet; update the three bullets (the Windows ARM64 manual workflow bullet, the runtime installer wrapper bullet, and the CI-specific E2E mode bullet) to use diverse structures (e.g. start one with a goal like "Validate a packaged/unpacked ARM64 build...", another with an implementation phrase like "Introduce a small runtime installer wrapper...", and the third with an outcome like "Provide a CI-specific E2E mode that..."), keeping original details intact and preserving references to `windows-11-arm`, `tiny-runtime-injector`, and the OpenAI-compatible mock server where present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/windows-arm64-e2e.yml:
- Line 16: Replace tag-based action refs with pinned commit SHAs for all
external actions referenced (actions/checkout@v6, actions/setup-node@v6,
pnpm/action-setup@v6, actions/upload-artifact@v6) so the workflow uses immutable
commits instead of floating tags; specifically update each action ref to its
corresponding full commit SHA. Also add with: persist-credentials: false to the
actions/checkout step (the checkout action invocation) to disable credential
persistence after checkout. Ensure the modified workflow preserves existing
inputs (e.g., node-version, cache settings) while only changing the action refs
and adding the persist-credentials setting.
In `@test/e2e/README.md`:
- Around line 27-31: The README documents provider/model IDs
(`e2e-openai-compatible`, `deepchat-e2e-mock`) that don’t match the
implementation; open test/e2e/helpers/testData.ts, find the actual provider and
model IDs used by the CI mock path (look for constants/objects that define
provider or model names) and update test/e2e/README.md to those exact IDs, or
alternatively change the IDs in testData.ts to match the README so they are
consistent; reference the symbol names you find in testData.ts (e.g., the
provider/model constants or map keys) and ensure the README text exactly matches
those identifiers.
---
Nitpick comments:
In `@docs/features/windows-arm64-support/plan.md`:
- Around line 5-7: Reword the Architecture section to vary sentence openings
instead of repeating "Add" for each bullet; update the three bullets (the
Windows ARM64 manual workflow bullet, the runtime installer wrapper bullet, and
the CI-specific E2E mode bullet) to use diverse structures (e.g. start one with
a goal like "Validate a packaged/unpacked ARM64 build...", another with an
implementation phrase like "Introduce a small runtime installer wrapper...", and
the third with an outcome like "Provide a CI-specific E2E mode that..."),
keeping original details intact and preserving references to `windows-11-arm`,
`tiny-runtime-injector`, and the OpenAI-compatible mock server where present.
In `@docs/features/windows-arm64-support/spec.md`:
- Around line 18-20: The three bullets in the Non-Goals section all start with
"Do not," making the list repetitive; rephrase each item to vary sentence
structure while preserving intent — e.g., change "Do not enable Windows ARM64 in
the production build/release matrix until the manual Windows ARM64 E2E workflow
has passed" to "Enable Windows ARM64 in the production build/release matrix only
after the manual Windows ARM64 E2E workflow has passed," change "Do not
guarantee every optional runtime is bundled on Windows ARM64" to "There is no
guarantee that every optional runtime will be bundled on Windows ARM64," and
change "Do not use real provider API keys in CI" to "Use mock or scoped provider
API keys in CI rather than real keys"; apply these rewordings in the Non-Goals
section and keep original meaning and negative constraints intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ab240ba-72eb-455c-983f-8a2a51cfa4b9
📒 Files selected for processing (15)
.github/workflows/windows-arm64-e2e.ymldocs/features/windows-arm64-support/plan.mddocs/features/windows-arm64-support/spec.mddocs/features/windows-arm64-support/tasks.mdpackage.jsonscripts/install-optional-runtimes.mjssrc/main/index.tstest/e2e/README.mdtest/e2e/fixtures/electronApp.tstest/e2e/helpers/mockProvider.tstest/e2e/helpers/testData.tstest/e2e/playwright.ci.config.tstest/main/lib/runtimeHelper.test.tstest/main/presenter/skillPresenter/skillExecutionService.test.tstest/main/scripts/install-optional-runtimes.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/fixtures/electronApp.ts`:
- Line 160: The fixture currently forwards the whole host environment via the
env: process.env line which allows the app to write into the developer's real
profile; change the electronApp fixture to create a temporary isolated user-data
directory and set env to a shallow clone of process.env with profile-related
variables overridden (e.g., HOME and/or USERPROFILE and XDG_CONFIG_HOME or the
Electron-specific user-data-dir env/flag) so the launched app uses that temp dir
instead of the host profile; update the code at the env: process.env site to use
the cloned-and-overridden env object and ensure the temp dir is created and
cleaned up by the fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6fe9023-c9c3-489d-b4c0-9cbdb141ed26
📒 Files selected for processing (9)
.github/workflows/windows-arm64-e2e.ymldocs/features/windows-arm64-support/plan.mddocs/features/windows-arm64-support/spec.mddocs/features/windows-arm64-support/tasks.mdpackage.jsontest/e2e/README.mdtest/e2e/fixtures/electronApp.tstest/e2e/helpers/mockProvider.tstest/e2e/helpers/testData.ts
✅ Files skipped from review due to trivial changes (3)
- docs/features/windows-arm64-support/tasks.md
- docs/features/windows-arm64-support/spec.md
- docs/features/windows-arm64-support/plan.md
| args: ['.'] | ||
| }), | ||
| cwd: REPO_ROOT, | ||
| env: process.env, |
Contributor
There was a problem hiding this comment.
Restore isolated E2E user-data profile for launched app.
Line 160 forwards the host environment unchanged, and this fixture writes onboarding/provider/model config. In packaged smoke runs, that can persist into a real/default DeepChat profile and cause state leakage/flaky reruns.
Suggested fix
+ const launchEnv: NodeJS.ProcessEnv = { ...process.env }
+ if (process.env.CI === 'true' || shouldUseMockProvider()) {
+ launchEnv.DEEPCHAT_E2E = '1'
+ launchEnv.DEEPCHAT_E2E_USER_DATA_DIR =
+ launchEnv.DEEPCHAT_E2E_USER_DATA_DIR ??
+ resolve(testInfo.outputDir, `deepchat-e2e-profile-${currentLaunch}`)
+ }
+
const electronApp = await electron.launch({
...(packaged
? {
executablePath: resolvePackagedExecutable(),
args: []
}
: {
args: ['.']
}),
cwd: REPO_ROOT,
- env: process.env,
+ env: launchEnv,
timeout: 120_000
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/fixtures/electronApp.ts` at line 160, The fixture currently forwards
the whole host environment via the env: process.env line which allows the app to
write into the developer's real profile; change the electronApp fixture to
create a temporary isolated user-data directory and set env to a shallow clone
of process.env with profile-related variables overridden (e.g., HOME and/or
USERPROFILE and XDG_CONFIG_HOME or the Electron-specific user-data-dir env/flag)
so the launched app uses that temp dir instead of the host profile; update the
code at the env: process.env site to use the cloned-and-overridden env object
and ensure the temp dir is created and cleaned up by the fixture.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/windows-arm64-support/spec.md`:
- Line 11: Update the acceptance criterion that currently references the
runner’s default profile to instead require the isolated "CI smoke" profile so
the spec matches the PR intent; locate the acceptance criterion wording that
mentions "runner’s default profile" and replace it with language specifying the
isolated "CI smoke" profile (or equivalent phrase used elsewhere in the doc) and
ensure the acceptance criteria explicitly state that CI smoke runs use an
isolated profile to avoid coupling CI results to runner state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9055f7fc-f34e-4e74-9e63-74f79e801fe9
📒 Files selected for processing (8)
docs/features/windows-arm64-support/plan.mddocs/features/windows-arm64-support/spec.mddocs/features/windows-arm64-support/tasks.mdpackage.jsontest/e2e/README.mdtest/e2e/fixtures/electronApp.tstest/e2e/helpers/testData.tstest/e2e/playwright.ci.config.ts
✅ Files skipped from review due to trivial changes (2)
- docs/features/windows-arm64-support/tasks.md
- test/e2e/README.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.
Summary
windows-11-armTests
pnpm run formatpnpm run format:checkpnpm run i18npnpm run lintpnpm run typecheckpnpm exec vitest --config vitest.config.ts test/main/scripts/install-optional-runtimes.test.js test/main/lib/runtimeHelper.test.ts test/main/presenter/skillPresenter/skillExecutionService.test.ts --runNotes
Summary by CodeRabbit
New Features
Documentation
Tests