Skip to content

fix: make build and test suite pass on Windows - #1139

Open
noises1990 wants to merge 3 commits into
rohitg00:mainfrom
noises1990:fix/windows-build-test
Open

fix: make build and test suite pass on Windows#1139
noises1990 wants to merge 3 commits into
rohitg00:mainfrom
noises1990:fix/windows-build-test

Conversation

@noises1990

@noises1990 noises1990 commented Aug 2, 2026

Copy link
Copy Markdown

What it does

Makes the project build and the full test suite pass on Windows without breaking macOS or Linux:

  • Replaces the Unix-only package.json build script (cp ... 2>/dev/null || true, mkdir -p ...) with a cross-platform Node script (scripts/copy-assets.mjs).
  • Fixes tests that assumed Unix-specific paths, HOME, and iii binary names:
    • test/cli-remove.test.ts — uses iii.exe on Windows, iii elsewhere.
    • test/compress-file.test.ts — uses path.resolve() and path.join() instead of literal /tmp/... paths.
    • test/copilot-plugin.test.ts — asserts resolveProject() result, not a hard-coded Windows absolute path.
    • test/obsidian-export.test.ts — uses os.tmpdir() + path.join() for export roots and file lookups.
    • test/slots-flag-gate.test.ts — sets/restores USERPROFILE alongside HOME because os.homedir() reads USERPROFILE on Windows.

Why

Windows contributors currently cannot run npm run build because the build pipeline relies on cp/mkdir shell tools that are not available in cmd.exe or PowerShell. Several unit tests also fail on Windows because they use /tmp/... literals, check for iii without the .exe extension, or only mock HOME while Node's os.homedir() prefers USERPROFILE on Windows. This PR removes those blockers.

How to verify

  1. On a Windows machine, run npm install.
  2. Run npm run build — it should complete without shell errors.
  3. Run npm test — the full suite (excluding test/integration.test.ts) should pass.

The change is additive and should not affect Linux/macOS behavior: the shell pipeline is replaced by Node's fs/promises and path APIs, and the test fixes use standard cross-platform helpers.

Notes

  • Does not address or introduce any source-code type errors. npx tsc --noEmit still reports the same pre-existing errors present on main.
  • No linked issue — this was discovered while setting up a Windows development environment.

Summary by CodeRabbit

  • Build

    • Improved distribution packaging by reliably copying viewer assets and optional configuration files during builds.
  • Bug Fixes

    • Improved cross-platform handling for executable paths, project locations, temporary files, and home-directory resolution.
    • Corrected backup path and content handling for compressed files, including original Markdown files.
  • Tests

    • Expanded platform-aware coverage for Windows and other operating systems.
    • Improved environment cleanup and path validation across export, plugin, and command-line tests.

Replace the Unix-only cp/mkdir-p/2>/dev/null || true shell pipeline
with a small Node script so npm run build works on Windows as well as
macOS and Linux.

Signed-off-by: Adrian Coroi <coroi_adrian@outlook.com>
Fix path, temp-dir, and homedir handling in tests so the full suite
passes on Windows. Changes include:
- Use path.resolve/path.join and os.tmpdir instead of Unix literals
- Set USERPROFILE alongside HOME where os.homedir() is used
- Use platform-aware iii.exe/iii binary names
- Resolve project names via resolveProject in copilot hook tests

Signed-off-by: Adrian Coroi <coroi_adrian@outlook.com>
@vercel

vercel Bot commented Aug 2, 2026

Copy link
Copy Markdown

@noises1990 is attempting to deploy a commit to the rohitg00's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6fc88643-7425-462a-8598-3f8d8b4282d3

📥 Commits

Reviewing files that changed from the base of the PR and between 8187978 and 03a568f.

📒 Files selected for processing (1)
  • test/copilot-plugin.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/copilot-plugin.test.ts

📝 Walkthrough

Walkthrough

The build now delegates asset packaging to scripts/copy-assets.mjs. Tests now use platform-aware executable, file, temporary-directory, vault, backup, home-directory, and project paths.

Changes

Portable build and test paths

Layer / File(s) Summary
Dedicated asset-copy build step
package.json, scripts/copy-assets.mjs
The build runs tsdown, then creates dist/viewer and copies required viewer assets and optional root files.
Platform-aware file and executable fixtures
test/cli-remove.test.ts, test/compress-file.test.ts, test/obsidian-export.test.ts
Tests now construct executable, fixture, backup, temporary, vault, and export paths with platform-aware helpers.
Windows environment and project resolution coverage
test/slots-flag-gate.test.ts, test/copilot-plugin.test.ts
Tests isolate HOME and USERPROFILE, and derive the Copilot project from a Windows-style working directory.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: Windows-compatible build and test updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/copilot-plugin.test.ts`:
- Line 6: Update the test around resolveProject and runHook to remove the
production resolver from expected-value construction. Clear
AGENTMEMORY_PROJECT_NAME for the test, then assert the platform-specific
expected project name directly, using "repo" for Windows while preserving the
appropriate non-Windows expectation.
🪄 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 Plus

Run ID: 2b5a264e-54d6-4fb2-9fb0-4f40ce4beabb

📥 Commits

Reviewing files that changed from the base of the PR and between d219763 and 8187978.

📒 Files selected for processing (7)
  • package.json
  • scripts/copy-assets.mjs
  • test/cli-remove.test.ts
  • test/compress-file.test.ts
  • test/copilot-plugin.test.ts
  • test/obsidian-export.test.ts
  • test/slots-flag-gate.test.ts

Comment thread test/copilot-plugin.test.ts Outdated
CodeRabbit review: do not use the production resolveProject() helper to compute the expected value. Pass AGENTMEMORY_PROJECT_NAME as empty so an env override cannot leak into the test, and assert the literal platform-correct project name ("repo" on Windows, "C:\repo" elsewhere).

Signed-off-by: Adrian Coroi <coroi_adrian@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant