Merged
Conversation
🦋 Changeset detectedLatest commit: c14b43a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (11)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds a changeset declaring a patch release for the Marko Vite plugin. Introduces two HMR fixture suites ( 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/main.test.ts (1)
331-344: Consider using forward slashes for Vite module paths.Vite internally uses forward slashes for module paths. Using
path.joinon Windows could produce backslashes that may cause issues withssrLoadModule.♻️ Proposed fix
const { handler } = await devServer.ssrLoadModule( - path.join(dir, "./src/index.js"), + `${dir}/src/index.js`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main.test.ts` around lines 331 - 344, The SSR middleware builds a module path with path.join which can produce backslashes on Windows and break devServer.ssrLoadModule; change the module path construction so it uses forward slashes (e.g., use path.posix.join or compute the path and replace backslashes with "/") when calling devServer.ssrLoadModule in the devServer.middlewares.use block (where config.ssr and devServer.ssrLoadModule are referenced) so the module path is always Vite-compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/fixtures/isomorphic-tags-api-hmr/dev-server.mjs`:
- Around line 33-43: Exported value from this module is a Connect middleware
(devServer.middlewares.use(...)) but tests expect an http.Server with a .listen;
wrap the middleware in an http.Server before default-exporting it. Concretely,
create an http.Server that calls the middleware (the function returned by
devServer.middlewares.use or the middleware handler built from
devServer.ssrLoadModule and handler) and export that server instead of the raw
middleware; reuse the same pattern you applied to
isomorphic-basic-hmr/dev-server.mjs or extract a shared helper to construct an
http.Server from a Connect middleware to avoid duplicating the wrapper logic
across fixtures.
---
Nitpick comments:
In `@src/__tests__/main.test.ts`:
- Around line 331-344: The SSR middleware builds a module path with path.join
which can produce backslashes on Windows and break devServer.ssrLoadModule;
change the module path construction so it uses forward slashes (e.g., use
path.posix.join or compute the path and replace backslashes with "/") when
calling devServer.ssrLoadModule in the devServer.middlewares.use block (where
config.ssr and devServer.ssrLoadModule are referenced) so the module path is
always Vite-compatible.
🪄 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: 64fa8771-376a-4a2b-966c-12d01df898de
⛔ Files ignored due to path filters (6)
src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-hmr/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (18)
.changeset/light-dolls-remain.mdsrc/__tests__/fixtures/isomorphic-basic-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-basic-hmr/server.mjssrc/__tests__/fixtures/isomorphic-basic-hmr/src/components/class-component.markosrc/__tests__/fixtures/isomorphic-basic-hmr/src/components/implicit-component.markosrc/__tests__/fixtures/isomorphic-basic-hmr/src/components/layout-component.markosrc/__tests__/fixtures/isomorphic-basic-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-basic-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-basic-hmr/test.config.tssrc/__tests__/fixtures/isomorphic-tags-api-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-tags-api-hmr/server.mjssrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/tags/child-tag.markosrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/tags/layout.markosrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-tags-api-hmr/test.config.tssrc/__tests__/main.test.tssrc/index.ts
DylanPiercey
force-pushed
the
fix-live-reload-tags-api
branch
3 times, most recently
from
194fd89 to
1080c1a
Compare
DylanPiercey
force-pushed
the
fix-live-reload-tags-api
branch
from
1080c1a to
c14b43a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/__tests__/main.test.ts (3)
410-422: Silently swallowing timeout may hide HMR failures.The
.catch(() => { ... })swallows the timeout error with only a comment. Consider logging when the timeout occurs to help diagnose HMR issues during test development.🔧 Suggested improvement
.catch(() => { // Timeout is acceptable — the HMR update may not change the `#app` content, // or the page may have been fully reloaded. + console.warn(`HMR wait timed out for step ${hmrIdx}`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main.test.ts` around lines 410 - 422, The test currently swallows timeouts from the page.waitForFunction call (invocation that uses prevRawHtml) which can hide HMR failures; update the .catch handler to log a diagnostic message (e.g., console.warn or console.error) that includes the error details (error.name/message/stack), the prevRawHtml value and some context like the current page URL or a short identifying string, then re-use the comment behavior (do not throw) so tests remain tolerant but produce useful logs for debugging HMR issues.
393-406:String.replaceonly replaces the first occurrence.The
content.replace(find, replace)call only replaces the first match offind. If the intent is to replace all occurrences (e.g., when the same pattern appears multiple times), consider usingreplaceAllor a regex with the global flag.If replacing all occurrences is needed:
- const newContent = content.replace(find, replace); + const newContent = content.replaceAll(find, replace);Otherwise, if single replacement is intentional, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main.test.ts` around lines 393 - 406, The loop handling HMR changes uses content.replace(find, replace) which only replaces the first occurrence; to replace all matches update the replacement logic in the hmrStep.changes handler (the block that reads filePath, content, newContent) to use replaceAll when find is a string (content.replaceAll(find, replace)), or convert/find to a RegExp with the global flag (new RegExp(escapedFind, "g")) before calling content.replace, ensuring newContent truly reflects all occurrences; keep the existing error check that compares newContent === content and update handling accordingly.
460-466: Consider ensuringdevServer.close()is called even if file restoration fails.If
fs.writeFileSyncthrows during restoration (e.g., permission error), thedevServer.close()call will be skipped, potentially leaving the server running.🔧 Suggested fix with nested finally
} finally { // Restore all mutated files. - for (const [filePath, content] of originalFiles) { - fs.writeFileSync(filePath, content); + try { + for (const [filePath, content] of originalFiles) { + fs.writeFileSync(filePath, content); + } + } finally { + await devServer.close(); } - await devServer.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main.test.ts` around lines 460 - 466, The finally block currently restores mutated files via fs.writeFileSync in a loop then awaits devServer.close(), which means a thrown error during file restoration will skip closing the dev server; wrap the restoration in its own try/catch (or move the devServer.close() into an outer finally) so that devServer.close() is always awaited regardless of fs.writeFileSync failures — specifically adjust the finally that uses originalFiles and fs.writeFileSync so errors during restoration are caught/handled and then ensure devServer.close() is called in a guaranteed cleanup path referencing originalFiles, fs.writeFileSync, and devServer.close().src/index.ts (1)
779-794: Consider capturing the module aftertransformRequestto avoid timing issues.The current code captures
entryModbefore callingtransformRequest, but if the module doesn't exist yet, it will be created bytransformRequest. In that case,entryModwould beundefinedeven though the module exists after the transform, and the invalidation would be skipped.🔧 Suggested fix
let markoAPI: string | undefined; if ("transformRequest" in this.environment) { - const entryMod = - this.environment.moduleGraph.getModuleById(fileName); await this.environment.transformRequest(fileName); markoAPI = this.getModuleInfo(fileName)?.meta.markoAPI; // transformRequest above re-populates the module's transformResult // which prevents the SSR module runner from detecting that it was // invalidated. Re-invalidate so the runner properly re-evaluates it. - - if (entryMod) { + const entryMod = + this.environment.moduleGraph.getModuleById(fileName); + if (entryMod) { this.environment.moduleGraph.invalidateModule(entryMod); } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 779 - 794, The code captures entryMod before calling transformRequest, so when transformRequest creates the module entryMod remains undefined and invalidateModule is skipped; change the logic in the block that checks "transformRequest" in this.environment to fetch the module after awaiting this.environment.transformRequest(fileName) (e.g., call this.environment.moduleGraph.getModuleById(fileName) again or reassign entryMod after the await) and then call this.environment.moduleGraph.invalidateModule(entryMod) if present; ensure markoAPI is still read from this.getModuleInfo(fileName)?.meta.markoAPI after the transform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/fixtures/isomorphic-basic-hmr/dev-server.mjs`:
- Around line 20-27: Remove the invalid server option by deleting the `ws:
false` entry from the server config in the dev server setup (the block that
contains `server: { ... }` with `hmr: false`, `middlewareMode: true`, and
`watch`). Vite 8 does not support `ws`; WebSocket behavior is controlled via
`server.hmr` which is already set to `false`, so simply remove the `ws: false`
line to fix the config.
---
Nitpick comments:
In `@src/__tests__/main.test.ts`:
- Around line 410-422: The test currently swallows timeouts from the
page.waitForFunction call (invocation that uses prevRawHtml) which can hide HMR
failures; update the .catch handler to log a diagnostic message (e.g.,
console.warn or console.error) that includes the error details
(error.name/message/stack), the prevRawHtml value and some context like the
current page URL or a short identifying string, then re-use the comment behavior
(do not throw) so tests remain tolerant but produce useful logs for debugging
HMR issues.
- Around line 393-406: The loop handling HMR changes uses content.replace(find,
replace) which only replaces the first occurrence; to replace all matches update
the replacement logic in the hmrStep.changes handler (the block that reads
filePath, content, newContent) to use replaceAll when find is a string
(content.replaceAll(find, replace)), or convert/find to a RegExp with the global
flag (new RegExp(escapedFind, "g")) before calling content.replace, ensuring
newContent truly reflects all occurrences; keep the existing error check that
compares newContent === content and update handling accordingly.
- Around line 460-466: The finally block currently restores mutated files via
fs.writeFileSync in a loop then awaits devServer.close(), which means a thrown
error during file restoration will skip closing the dev server; wrap the
restoration in its own try/catch (or move the devServer.close() into an outer
finally) so that devServer.close() is always awaited regardless of
fs.writeFileSync failures — specifically adjust the finally that uses
originalFiles and fs.writeFileSync so errors during restoration are
caught/handled and then ensure devServer.close() is called in a guaranteed
cleanup path referencing originalFiles, fs.writeFileSync, and devServer.close().
In `@src/index.ts`:
- Around line 779-794: The code captures entryMod before calling
transformRequest, so when transformRequest creates the module entryMod remains
undefined and invalidateModule is skipped; change the logic in the block that
checks "transformRequest" in this.environment to fetch the module after awaiting
this.environment.transformRequest(fileName) (e.g., call
this.environment.moduleGraph.getModuleById(fileName) again or reassign entryMod
after the await) and then call
this.environment.moduleGraph.invalidateModule(entryMod) if present; ensure
markoAPI is still read from this.getModuleInfo(fileName)?.meta.markoAPI after
the transform.
🪄 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: c086bf61-9f2d-499a-bae0-d7c4060918cd
⛔ Files ignored due to path filters (6)
src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-hmr/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (18)
.changeset/light-dolls-remain.mdsrc/__tests__/fixtures/isomorphic-basic-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-basic-hmr/server.mjssrc/__tests__/fixtures/isomorphic-basic-hmr/src/components/class-component.markosrc/__tests__/fixtures/isomorphic-basic-hmr/src/components/implicit-component.markosrc/__tests__/fixtures/isomorphic-basic-hmr/src/components/layout-component.markosrc/__tests__/fixtures/isomorphic-basic-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-basic-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-basic-hmr/test.config.tssrc/__tests__/fixtures/isomorphic-tags-api-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-tags-api-hmr/server.mjssrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/tags/child-tag.markosrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/tags/layout.markosrc/__tests__/fixtures/isomorphic-tags-api-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-tags-api-hmr/test.config.tssrc/__tests__/main.test.tssrc/index.ts
✅ Files skipped from review due to trivial changes (10)
- src/tests/fixtures/isomorphic-basic-hmr/src/components/implicit-component.marko
- src/tests/fixtures/isomorphic-tags-api-hmr/src/template.marko
- src/tests/fixtures/isomorphic-tags-api-hmr/src/tags/child-tag.marko
- .changeset/light-dolls-remain.md
- src/tests/fixtures/isomorphic-tags-api-hmr/src/tags/layout.marko
- src/tests/fixtures/isomorphic-basic-hmr/src/components/layout-component.marko
- src/tests/fixtures/isomorphic-basic-hmr/src/template.marko
- src/tests/fixtures/isomorphic-tags-api-hmr/test.config.ts
- src/tests/fixtures/isomorphic-basic-hmr/test.config.ts
- src/tests/fixtures/isomorphic-basic-hmr/src/components/class-component.marko
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tests/fixtures/isomorphic-basic-hmr/src/index.js
- src/tests/fixtures/isomorphic-tags-api-hmr/server.mjs
- src/tests/fixtures/isomorphic-tags-api-hmr/src/index.js
- src/tests/fixtures/isomorphic-basic-hmr/server.mjs
- src/tests/fixtures/isomorphic-tags-api-hmr/dev-server.mjs
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.
inputusage.