Conversation
🦋 Changeset detectedLatest commit: cb18dc3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughA changeset was added declaring coordinated version bumps with release note "Vite 8 support." Multiple package.json files (root and examples) updated Marko, Vite, and various devDependencies. Peer dependency ranges for marko/run in adapters and explorer were extended to include v0.10. Internal code was migrated from Rollup to Rolldown types and build option keys. Tests were refactored to use a global promise-based loading coordination and revised snapshot capture flow. CI was changed to install a pinned npm version before running npm ci. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
package.json (1)
57-57: Consider adding a caret for consistency.
playwrightis pinned to an exact version (1.58.2) while all other dependencies use caret ranges (^). If this is intentional for test stability, consider adding a comment in the repo explaining the pinning rationale. Otherwise, use^1.58.2for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 57, The dependency entry for "playwright" is pinned to an exact version ("1.58.2") while other deps use caret ranges; update the package.json dependency for "playwright" from "1.58.2" to "^1.58.2" for consistency, or if pinning was intentional, add a short repository comment (e.g., in README or a deps.md) explaining the stability rationale; ensure you update the "playwright" dependency string and/or add the explanatory note accordingly.packages/run/src/__tests__/main.test.ts (1)
312-329: Consider removing or enabling the commented-out error detection.This commented block for detecting error responses is inactive. If it's no longer needed with the new approach, consider removing it to reduce code noise. If it's intentionally disabled for debugging, a brief comment explaining why would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/run/src/__tests__/main.test.ts` around lines 312 - 329, The commented-out error-detection block around stepContext.response.ok()/response.text() inside the HTML content handling (the block that also checks page.title() === "Error" and builds an Error with stack from page.locator("pre")) is left in-place and should be either removed or re-enabled with a clear justification; decide whether you want active error detection for failing responses in the test: if still needed, re-enable and wire it to the existing contentType check (use stepContext.response.ok() and throw the constructed Error when page.title() === "Error"), otherwise delete the entire commented block and add a one-line comment near the contentType/prevHtml/snapshot logic explaining why error detection was intentionally disabled to avoid confusion for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/node-express/package.json`:
- Around line 12-13: Add an "engines" field to package.json to require Node.js
18+ so the example clarifies runtime requirements; update the package.json JSON
object to include "engines": { "node": ">=18" } alongside the existing
dependencies (ensure the new key is added at top-level of the package.json that
contains "compression" and "express") and validate the JSON remains well-formed.
In `@packages/run/src/vite/utils/log.ts`:
- Line 6: The code imports the types OutputAsset, OutputBundle, and OutputChunk
from "rolldown" (seen in utils/log.ts and plugin.ts) but "rolldown" is not
declared as an explicit dependency; add "rolldown" to the run package's
package.json dependencies (the run package's package.json), then
reinstall/update lockfile so the dependency is recorded—this makes the type
imports (OutputAsset, OutputBundle, OutputChunk) explicit and prevents breakage
under strict package managers.
---
Nitpick comments:
In `@package.json`:
- Line 57: The dependency entry for "playwright" is pinned to an exact version
("1.58.2") while other deps use caret ranges; update the package.json dependency
for "playwright" from "1.58.2" to "^1.58.2" for consistency, or if pinning was
intentional, add a short repository comment (e.g., in README or a deps.md)
explaining the stability rationale; ensure you update the "playwright"
dependency string and/or add the explanatory note accordingly.
In `@packages/run/src/__tests__/main.test.ts`:
- Around line 312-329: The commented-out error-detection block around
stepContext.response.ok()/response.text() inside the HTML content handling (the
block that also checks page.title() === "Error" and builds an Error with stack
from page.locator("pre")) is left in-place and should be either removed or
re-enabled with a clear justification; decide whether you want active error
detection for failing responses in the test: if still needed, re-enable and wire
it to the existing contentType check (use stepContext.response.ok() and throw
the constructed Error when page.title() === "Error"), otherwise delete the
entire commented block and add a one-line comment near the
contentType/prevHtml/snapshot logic explaining why error detection was
intentionally disabled to avoid confusion for future maintainers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5545474d-66c8-4795-87c3-fff6e7cee7f3
⛔ Files ignored due to path filters (21)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/adapter-is-entry/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/adapter-is-entry/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/basic-client-component/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/basic-client-component/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/error-invalid-routes/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/external-routes/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/external-routes/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/micro-frame-fetch/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/micro-frame-fetch/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-edge/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/post-get-single-flight/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/post-get-single-flight/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/static-adapter-file-plain/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/static-adapter-file-plain/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/verb-specific-js-non-object-meta/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (18)
.changeset/breezy-horses-check.mdexamples/netlify/package.jsonexamples/node-express/package.jsonexamples/static/package.jsonexamples/zero-config/package.jsonpackage.jsonpackages/adapters/netlify/package.jsonpackages/adapters/node/package.jsonpackages/adapters/static/package.jsonpackages/explorer/package.jsonpackages/run/package.jsonpackages/run/src/__tests__/fixtures/micro-frame-fetch/src/routes/other+page.markopackages/run/src/__tests__/main.test.tspackages/run/src/adapter/dev-server.tspackages/run/src/adapter/utils.tspackages/run/src/cli/commands.tspackages/run/src/vite/plugin.tspackages/run/src/vite/utils/log.ts
18805d2 to
6ee0206
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/run/src/vite/plugin.ts (1)
449-450: Keep reading the deprecatedbuild.rollupOptionsalias here.The
confighook receives the raw user config, and Vite 8 still documentsbuild.rollupOptionsas an alias ofbuild.rolldownOptions. On Line 449, falling back toconfig.build?.rollupOptions?.outputkeeps this pre-merge path compatible with apps that haven't migrated yet. (vite.dev)♻️ Compatibility-safe fallback
- let rolldownOutputOptions = config.build?.rolldownOptions?.output; + let rolldownOutputOptions = + config.build?.rolldownOptions?.output ?? + config.build?.rollupOptions?.output;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/run/src/vite/plugin.ts` around lines 449 - 450, The code sets rolldownOutputOptions from config.build?.rolldownOptions?.output but misses the backwards-compatible alias; update the logic that computes rolldownOutputOptions (the variable assigned from rolldownOptions) to also fall back to config.build?.rollupOptions?.output so the plugin accepts the legacy rollupOptions shape (i.e., check config.build?.rolldownOptions?.output || config.build?.rollupOptions?.output when resolving the output options).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/run/src/vite/plugin.ts`:
- Around line 894-912: The loop that computes per-browser versions in
getBrowserslistTargets currently keeps the highest version and skips targets
when the digit index is 0; change the index check to "if (index !== undefined)"
and invert the comparison so you store the lowest version per browser (compare
using < and initialize the default to Infinity or check for undefined). Keep
using resolveToEsbuildTarget and the versions Map, and ensure targets are still
built by concatenating browser + version after updating the Map.
---
Nitpick comments:
In `@packages/run/src/vite/plugin.ts`:
- Around line 449-450: The code sets rolldownOutputOptions from
config.build?.rolldownOptions?.output but misses the backwards-compatible alias;
update the logic that computes rolldownOutputOptions (the variable assigned from
rolldownOptions) to also fall back to config.build?.rollupOptions?.output so the
plugin accepts the legacy rollupOptions shape (i.e., check
config.build?.rolldownOptions?.output || config.build?.rollupOptions?.output
when resolving the output options).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18fee79c-02a3-4c2e-aa4c-68324fe66a6c
⛔ Files ignored due to path filters (21)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/adapter-is-entry/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/adapter-is-entry/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/basic-client-component/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/basic-client-component/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/error-invalid-routes/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/external-routes/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/external-routes/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/micro-frame-fetch/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/micro-frame-fetch/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-edge/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/post-get-single-flight/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/post-get-single-flight/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/static-adapter-file-plain/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/static-adapter-file-plain/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/verb-specific-js-non-object-meta/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (18)
.changeset/breezy-horses-check.mdexamples/netlify/package.jsonexamples/node-express/package.jsonexamples/static/package.jsonexamples/zero-config/package.jsonpackage.jsonpackages/adapters/netlify/package.jsonpackages/adapters/node/package.jsonpackages/adapters/static/package.jsonpackages/explorer/package.jsonpackages/run/package.jsonpackages/run/src/__tests__/fixtures/micro-frame-fetch/src/routes/other+page.markopackages/run/src/__tests__/main.test.tspackages/run/src/adapter/dev-server.tspackages/run/src/adapter/utils.tspackages/run/src/cli/commands.tspackages/run/src/vite/plugin.tspackages/run/src/vite/utils/log.ts
✅ Files skipped from review due to trivial changes (11)
- packages/run/src/tests/fixtures/micro-frame-fetch/src/routes/other+page.marko
- packages/explorer/package.json
- .changeset/breezy-horses-check.md
- packages/adapters/node/package.json
- examples/static/package.json
- packages/adapters/static/package.json
- examples/zero-config/package.json
- packages/run/src/vite/utils/log.ts
- examples/node-express/package.json
- packages/run/package.json
- package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/run/src/cli/commands.ts
- examples/netlify/package.json
- packages/adapters/netlify/package.json
- packages/run/src/tests/main.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/run/src/__tests__/main.test.ts`:
- Around line 119-133: The onError handler currently appends the error container
into (document.getElementById("app") || document.body) which will throw if
<body> doesn't exist; update the onError function to avoid appendChild errors by
falling back to document.documentElement (or buffering the message until body
exists) when creating/appending the "error" <pre> (identify the logic around
errorContainer creation and the insertAdjacentText call). Ensure you locate the
onError function and the block that creates element.id = "error" and change the
append destination to use document.getElementById("app") || document.body ||
document.documentElement (or queue messages and flush when body is present) so
the handler never throws before the body exists.
- Around line 143-157: getHTML() currently only awaits the __loading__ promise,
which misses DOM updates from XHR/fetch; update getHTML to also wait for
DOM/network quiescence before snapshotting by adding one of two options: (A)
await a page-level idle indicator (e.g., await page.evaluate(() =>
window.__pendingRequests__ === 0 || new Promise(r => (window.__onIdle__ = r))))
so getHTML checks both __loading__ and window.__pendingRequests__, or (B)
perform a stable-DOM check inside getHTML: loop using requestAnimationFrame to
serialize the fragment twice (using defaultSerializer/defaultNormalizer) and
only return once two consecutive serializations match, ensuring no mid-flight
fetch/XHR or client navigation mutated the DOM; reference the getHTML function
and the __loading__ symbol when implementing.
- Around line 307-337: The non-HTML branch currently captures the wrong content
by calling page.content() before the response body is guaranteed to be complete;
update the else branch to await the response completion with
stepContext.response.finished() and then read the raw response body via await
stepContext.response.text() (instead of page.content()), so the snapshot
includes the actual response body like the HTML branch; keep the existing use of
waitUntil: "commit" but ensure you await finished() and use
stepContext.response.text() to build the snapshot.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab1d57e4-5765-4bbc-80de-564376f62008
⛔ Files ignored due to path filters (21)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/adapter-is-entry/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/adapter-is-entry/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/basic-client-component/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/basic-client-component/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/error-invalid-routes/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/external-routes/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/external-routes/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/micro-frame-fetch/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/micro-frame-fetch/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-edge/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/post-get-single-flight/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/post-get-single-flight/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/static-adapter-file-plain/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/static-adapter-file-plain/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/verb-specific-js-non-object-meta/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (18)
.changeset/breezy-horses-check.mdexamples/netlify/package.jsonexamples/node-express/package.jsonexamples/static/package.jsonexamples/zero-config/package.jsonpackage.jsonpackages/adapters/netlify/package.jsonpackages/adapters/node/package.jsonpackages/adapters/static/package.jsonpackages/explorer/package.jsonpackages/run/package.jsonpackages/run/src/__tests__/fixtures/micro-frame-fetch/src/routes/other+page.markopackages/run/src/__tests__/main.test.tspackages/run/src/adapter/dev-server.tspackages/run/src/adapter/utils.tspackages/run/src/cli/commands.tspackages/run/src/vite/plugin.tspackages/run/src/vite/utils/log.ts
✅ Files skipped from review due to trivial changes (12)
- packages/run/src/tests/fixtures/micro-frame-fetch/src/routes/other+page.marko
- examples/zero-config/package.json
- packages/run/src/vite/utils/log.ts
- packages/adapters/static/package.json
- packages/adapters/node/package.json
- .changeset/breezy-horses-check.md
- examples/static/package.json
- examples/netlify/package.json
- packages/adapters/netlify/package.json
- examples/node-express/package.json
- packages/run/package.json
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/explorer/package.json
- packages/run/src/cli/commands.ts
- packages/run/src/adapter/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 24: Pin the Node 20 entries in the CI test matrix to at least 20.17.0 so
npm@11.6.0's engine requirement is satisfied: update the matrix values currently
like "20" to "20.17.0" (or "20.17.x") wherever Node 20 appears (the three matrix
entries referenced) and keep the rest of the matrix (22, 24) unchanged; ensure
any setup-node/uses: actions/setup-node that consumes the matrix value will pick
up the pinned 20.17.0.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09007c14-a474-46cc-a156-5c2d74d97158
📒 Files selected for processing (1)
.github/workflows/ci.yml
2ec8cfe to
7b8c211
Compare
No description provided.