Skip to content

refactor(cli): resolve platform-dependent paths inside the domains - #1409

Merged
Simon Ingeson (smonn) merged 1 commit into
mainfrom
wiz-11280-commands-resolves-platform-dependent-paths-beside-a-separate
Jul 30, 2026
Merged

refactor(cli): resolve platform-dependent paths inside the domains#1409
Simon Ingeson (smonn) merged 1 commit into
mainfrom
wiz-11280-commands-resolves-platform-dependent-paths-beside-a-separate

Conversation

@smonn

Copy link
Copy Markdown
Contributor

Relates to WIZ-11280

Overview of Changes

Every composite root resolved a platform-dependent binary path and then passed platform as a separate dep field, so a test could pair platform: "win32" with a path some other platform resolved. The domain forwarded that platform to the spawn, which routed a POSIX path through cmd.exe — a pairing production never produces. The deps now carry the platform plus the inputs a path needs (env dir, android home), and the domain resolves the path itself, so one value decides both the path and the spawn.

shell/playwright.ts is gone. Its win32 candidate list is now a pure playwrightCliCandidates(envDir, platform) in core/, and the domains pick the first existing candidate through their checkExists dep instead of calling existsSync behind a throw. That also drops the try/catch in the doctor handler. installUiautomator2Driver was not in the issue, but leaving it mismatchable while its sibling installAvds was fixed would have been worse.

Testing

bun run typecheck
bun run lint
bun run format:check
bun run knip
bun run test
bun run build

All pass; bun run test is 1221 pass / 0 fail. Five new tests assert win32 resolution at each seam — playwright candidate ordering, the doctor Playwright and Appium checks, install browsers, and install android. Each failed before its fix; the Appium one reproduced the issue exactly, pairing platform: "win32" with a POSIX path. Expected paths are built with join, so they stay correct on a win32 host (see WIZ-11279).

Checklist

  • Changes follow the code style of this project
  • Self-review completed
  • Tests added/updated (or not applicable)
  • No breaking changes (or described below)

Structural only — no behavior change. Follow-ups filed while reviewing: WIZ-11283 (the one surviving instance, in createAppiumServer) and WIZ-11284 (two different Playwright-not-found strings). WIZ-11281 should land on top of this; resolvePinned.ts keeps a second hardcoded shim-name list that must change with the candidate list, noted on that issue.

Every composite root resolved a platform-dependent binary path and passed
`platform` as a separate dep field. A test could pair `platform: "win32"`
with a path some other platform resolved, and the domain forwarded that
platform to the spawn — a pairing production never produces.

The deps now carry the platform plus the inputs a path needs, so one value
decides both the path and the spawn. `playwrightCliCandidates` in core/
replaces `shell/playwright.ts`; the domains pick the first existing
candidate via their `checkExists` dep.

WIZ-11280
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The refactor centralizes Playwright candidate discovery in playwrightCliCandidates, including Windows .cmd preference and fallback behavior. Doctor checks and browser installation now resolve Playwright from envDir using existence checks. Android doctor and installation flows derive Appium, SDK manager, and AVD manager paths from environment and platform inputs. Pre-resolved executable-path dependencies were removed, with tests updated for Windows behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • qawolf/cli#1400: Modifies the same browser installation wiring in src/commands/install/browsers.ts.
  • qawolf/cli#1403: Changes the doctor check dependency contract and platform handling.
  • qawolf/cli#1405: Overlaps with the Windows-aware Appium and Android SDK binary resolution changes.

Suggested reviewers: mateuszitelli

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Conventional Commits format is valid, the scope is acceptable, and the subject clearly matches the refactor.
Description check ✅ Passed The description includes the issue link, overview, testing commands, and checklist required by the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch wiz-11280-commands-resolves-platform-dependent-paths-beside-a-separate

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@src/domains/doctor/checks/androidAppium.ts`:
- Line 7: Move the pure resolveAppiumBin implementation from shell/appium into
core, preserving its deterministic path-building behavior and zero-I/O contract.
Update all consumers, including the Android Appium doctor check, to import the
core symbol, and remove or stop using the shell location so shell remains
limited to I/O.

In `@src/domains/install/android/driver.ts`:
- Line 4: Move the pure resolveAppiumBin implementation from the shell layer
into an appropriate src/core path-resolution module, preserving its behavior and
exports. Update the Android driver import and every other reference to use the
new ~/core/... location, and remove the obsolete shell definition.

In `@src/domains/runtimeEnv/resolvePinned.ts`:
- Around line 37-38: Update the Playwright resolution logic around
playwrightCliCandidates so POSIX environments require the extension-less
playwright shim and do not treat playwright.cmd alone as resolved. Keep Windows
support for the .cmd wrapper, using platform-aware validation consistent with
the candidates returned by playwrightCliCandidates.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 12523886-348c-42ca-b827-20cfd5c9ed40

📥 Commits

Reviewing files that changed from the base of the PR and between 3b36c29 and 9cfdc4e.

📒 Files selected for processing (28)
  • AGENTS.md
  • src/commands/doctor/handler.ts
  • src/commands/flows/buildFlowsRunDeps.ts
  • src/commands/install/android.ts
  • src/commands/install/browsers.fixtures.ts
  • src/commands/install/browsers.test.ts
  • src/commands/install/browsers.ts
  • src/core/messages/install.ts
  • src/core/playwrightBins.test.ts
  • src/core/playwrightBins.ts
  • src/domains/doctor/checks/android.fixtures.ts
  • src/domains/doctor/checks/android.test.ts
  • src/domains/doctor/checks/android.ts
  • src/domains/doctor/checks/androidAppium.ts
  • src/domains/doctor/checks/index.ts
  • src/domains/doctor/checks/playwright.test.ts
  • src/domains/doctor/checks/playwright.ts
  • src/domains/doctor/checks/runChecks.test.ts
  • src/domains/install/android/avd.test.ts
  • src/domains/install/android/avd.ts
  • src/domains/install/android/driver.test.ts
  • src/domains/install/android/driver.ts
  • src/domains/install/android/index.ts
  • src/domains/install/android/installAndroid.test.ts
  • src/domains/install/browsers.ts
  • src/domains/runtimeEnv/resolvePinned.ts
  • src/shell/playwright.test.ts
  • src/shell/playwright.ts
💤 Files with no reviewable changes (4)
  • src/shell/playwright.ts
  • src/shell/playwright.test.ts
  • src/commands/doctor/handler.ts
  • src/commands/install/android.ts

Comment thread src/domains/doctor/checks/androidAppium.ts
Comment thread src/domains/install/android/driver.ts
Comment thread src/domains/runtimeEnv/resolvePinned.ts
@smonn
Simon Ingeson (smonn) merged commit 9d600a6 into main Jul 30, 2026
7 checks passed
@smonn
Simon Ingeson (smonn) deleted the wiz-11280-commands-resolves-platform-dependent-paths-beside-a-separate branch July 30, 2026 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants