Conversation
Reviewer's GuideMigrates the project from Rollup/web-dev-server/Cypress to a Vite-based build and test setup, including new Vite configs for the main app and packages, Vitest unit tests, Playwright e2e harness with coverage, and updated HTML/bootstrap code for ES and IIFE editor entrypoints. Sequence diagram for Playwright-based e2e test execution with Vite previewsequenceDiagram
actor Dev
participant Npm as NpmTestE2E
participant RunE2E as RunE2EScript
participant Play as PlaywrightCLI
participant WebSrv as PlaywrightWebServer
participant VitePrev as VitePreviewServer
participant App as SVGEditorApp
participant Cov as NYC
Dev->>Npm: npm run test:e2e
Npm->>RunE2E: node scripts/run-e2e.mjs
RunE2E->>RunE2E: set COVERAGE=true
RunE2E->>RunE2E: sanitize env (unset ELECTRON_RUN_AS_NODE)
RunE2E->>Play: npx playwright --version
Play-->>RunE2E: version ok
RunE2E->>RunE2E: rimraf .nyc_output/*
RunE2E->>Play: npx playwright test
Play->>WebSrv: start configured web server
WebSrv->>VitePrev: npm run start:e2e (vite preview)
VitePrev->>App: serve dist/editor at http://localhost:8000
loop e2e test cases
Play->>App: HTTP requests to baseURL (editor pages)
App-->>Play: responses and DOM
end
Play-->>RunE2E: tests complete (exit code)
RunE2E->>Cov: npx nyc report (text-summary,json-summary)
Cov-->>Dev: coverage reports
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
vite.config.mjsthe vitesttestconfig excludestests/unit/**even though your migrated unit specs (e.g.,tests/unit/*.test.js) live there, so those tests will be skipped unless you adjust theexcludepattern. - The new
tests/unit/browser-bugs/removeItem-setAttribute.test.jstest usesassertwithout importing it or configuring a global assert for vitest; either import Node'sassertmodule or rewrite the expectations to useexpect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `vite.config.mjs` the vitest `test` config excludes `tests/unit/**` even though your migrated unit specs (e.g., `tests/unit/*.test.js`) live there, so those tests will be skipped unless you adjust the `exclude` pattern.
- The new `tests/unit/browser-bugs/removeItem-setAttribute.test.js` test uses `assert` without importing it or configuring a global assert for vitest; either import Node's `assert` module or rewrite the expectations to use `expect`.
## Individual Comments
### Comment 1
<location> `vite.config.mjs:72` </location>
<code_context>
+ emptyOutDir: true,
+ sourcemap: true,
+ lib: {
+ entry: resolve(__dirname, 'src/editor/Editor.js'),
+ name: 'Editor',
+ formats: ['es', 'iife'],
</code_context>
<issue_to_address>
**issue (bug_risk):** __dirname is not defined in ESM; this will fail at runtime under pure ESM execution.
Because `vite.config.mjs` runs as an ES module, `__dirname` isn’t available and this line will throw `ReferenceError: __dirname is not defined` when Vite loads the config.
Derive the directory from `import.meta.url` instead, e.g.:
```js
const __dirname = new URL('.', import.meta.url).pathname
export default defineConfig({
build: {
lib: {
entry: resolve(__dirname, 'src/editor/Editor.js'),
// ...
}
}
})
```
(adjust as needed for your path normalization requirements).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| emptyOutDir: true, | ||
| sourcemap: true, | ||
| lib: { | ||
| entry: resolve(__dirname, 'src/editor/Editor.js'), |
There was a problem hiding this comment.
issue (bug_risk): __dirname is not defined in ESM; this will fail at runtime under pure ESM execution.
Because vite.config.mjs runs as an ES module, __dirname isn’t available and this line will throw ReferenceError: __dirname is not defined when Vite loads the config.
Derive the directory from import.meta.url instead, e.g.:
const __dirname = new URL('.', import.meta.url).pathname
export default defineConfig({
build: {
lib: {
entry: resolve(__dirname, 'src/editor/Editor.js'),
// ...
}
}
})(adjust as needed for your path normalization requirements).
jfhenon
force-pushed
the
Vite
branch
2 times, most recently
from
111abd9 to
fc7b936
Compare
jfhenon
force-pushed
the
Vite
branch
2 times, most recently
from
fac2c1a to
0a1b0e2
Compare
extend test coverage
also update svgedit.css
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.
PR description
Lot of changes in the build but highly simplified, modern and much faster.
Summary by Sourcery
Migrate the project’s build and dev tooling from Rollup/web-dev-server/Cypress to a Vite-based setup with Vitest and Playwright, updating configs, scripts, and tests accordingly.
New Features:
Enhancements:
Documentation:
Tests:
Chores: