Conversation
🦋 Changeset detectedLatest commit: d4fae7c 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 |
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a changeset marking 🚥 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 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: 4
🤖 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-virtual-file-hmr/test.config.ts`:
- Around line 2-8: The hmr fixture only replaces the stylesheet but provides no
post-update steps to make the CSS change observable; update the exported hmr
array (export const hmr) to include a steps array for each change that triggers
a UI reflow/read (e.g., simulate a click on the page element with id
"#clickable") after the stylesheet swap so the test snapshot in main.test.ts
observes the new computed color; specifically add a steps entry alongside
changes in the hmr object so the test runner executes hmrStep.steps (simulate
click "#clickable" after each update).
In `@src/__tests__/fixtures/isomorphic-virtual-file-hmr/server.mjs`:
- Around line 12-16: The request listener passed to createServer should catch
and handle errors and must not rely solely on res.headersSent to know if the
handler finished; wrap the body of the async callback in try/catch, call await
handler(req, res) and treat its return value as an explicit completion flag
(e.g., handler returns true when it fully handled the response or a Promise that
resolves when template.render completes), and only call await serve(req, res,
serveOpts) when handler indicates it did not complete the response; on any
caught error send a 500 response (or use res.writeHead(500)/res.end) and log the
error so rejections from handler or serve are not unhandled.
In `@src/__tests__/main.test.ts`:
- Around line 414-424: Replace the risky use of the possibly already-resolved
window.__nextHmr by installing a fresh deferred promise in the page context
before awaiting HMR: evaluate in the page to create a new deferred and assign
its promise to window.__nextHmr so Promise.any races the newly-created HMR
promise and the DOM-change watcher (referencing page.evaluate, window.__nextHmr,
page.waitForFunction, prevRawHtml, Promise.any). Also remove the broad .catch(()
=> {}) that swallows all errors—only treat the DOM wait timeout as optional (so
DOM timeout can be handled locally) but let HMR promise rejections surface so
test failures are detectable.
In `@src/index.ts`:
- Around line 150-160: The virtual file isn't being registered in
virtualFilesForTemplate on the first compilation because the current logic only
adds id when prev exists and is not a deferred promise; update the handler so
that for the given normalizedFrom you always ensure a Set exists and add id to
virtualFilesForTemplate (i.e., move/create the files Set and files.add(id)
unconditionally for this normalizedFrom), while preserving the existing behavior
of resolving deferred promises via isDeferredPromise(prev) (call
prev.resolve(virtualFile) when applicable).
🪄 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: 4091f4cb-f9d7-4178-b92a-2af69aadc27e
⛔ Files ignored due to path filters (6)
src/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-virtual-file-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-virtual-file-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-virtual-file-hmr/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (19)
.changeset/happy-rooms-burn.mdsrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/server.mjssrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/src/tags/child-tag.markosrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/src/tags/layout.markosrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-tags-api-virtual-file-hmr/test.config.tssrc/__tests__/fixtures/isomorphic-virtual-file-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-virtual-file-hmr/server.mjssrc/__tests__/fixtures/isomorphic-virtual-file-hmr/src/components/class-component.markosrc/__tests__/fixtures/isomorphic-virtual-file-hmr/src/components/implicit-component.markosrc/__tests__/fixtures/isomorphic-virtual-file-hmr/src/components/layout-component.markosrc/__tests__/fixtures/isomorphic-virtual-file-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-virtual-file-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-virtual-file-hmr/test.config.tssrc/__tests__/main.test.tssrc/__tests__/utils/inject-hmr-events-plugin.tssrc/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/__tests__/main.test.ts (1)
421-435:⚠️ Potential issue | 🟠 MajorInstall a fresh HMR promise before waiting on the next update.
This still reads whichever deferred is currently sitting on
window.__nextHmr. If the previous cycle already resolved it,Promise.any(...)can return immediately and the snapshot gets taken before the current HMR cycle finishes. The DOM fallback is not enough for the new server-only HMR cases, so this remains flaky. Create a new deferred in the page before mutating files, then await that specific promise here.🤖 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 421 - 435, The test is racing on a possibly-stale window.__nextHmr deferred; before mutating files, inject a fresh deferred into the page (e.g. create and assign a new Promise resolver to window.__nextHmr via page.evaluate) and capture a handle/reference to that specific promise, then replace the Promise.any(...) block so it awaits that captured promise plus the existing DOM fallback (page.waitForFunction using prevRawHtml) instead of relying on whatever deferred happens to be on window.__nextHmr; update the code locations referencing window.__nextHmr, Promise.any, page.evaluate and prevRawHtml accordingly to await the newly created promise.
🤖 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/index.ts`:
- Around line 151-157: virtualFilesForTemplate currently only appends with
files.add(id), causing stale ids to accumulate; instead, during a single compile
pass collect the current compile's virtual ids for a given normalizedFrom into a
fresh Set and replace the stored entry (use
virtualFilesForTemplate.set(normalizedFrom, currentSet)) so the map reflects
only ids emitted this compile; if no ids were emitted, remove the entry
(virtualFilesForTemplate.delete(normalizedFrom)); make this change around the
code that currently reads virtualFilesForTemplate.get(normalizedFrom) and calls
files.add(id) (leave the isDeferredPromise(prev) handling intact).
---
Duplicate comments:
In `@src/__tests__/main.test.ts`:
- Around line 421-435: The test is racing on a possibly-stale window.__nextHmr
deferred; before mutating files, inject a fresh deferred into the page (e.g.
create and assign a new Promise resolver to window.__nextHmr via page.evaluate)
and capture a handle/reference to that specific promise, then replace the
Promise.any(...) block so it awaits that captured promise plus the existing DOM
fallback (page.waitForFunction using prevRawHtml) instead of relying on whatever
deferred happens to be on window.__nextHmr; update the code locations
referencing window.__nextHmr, Promise.any, page.evaluate and prevRawHtml
accordingly to await the newly created promise.
🪄 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: ee9dc67f-62c2-4f9c-a840-77a615975466
⛔ Files ignored due to path filters (7)
src/__tests__/fixtures/isomorphic-basic-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-server-only-hmr/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-server-only-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-server-only-hmr/__snapshots__/dev.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-virtual-file-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-virtual-file-hmr/__snapshots__/dev-hmr.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (10)
src/__tests__/fixtures/isomorphic-server-only-hmr/dev-server.mjssrc/__tests__/fixtures/isomorphic-server-only-hmr/server.mjssrc/__tests__/fixtures/isomorphic-server-only-hmr/src/components/class-component.markosrc/__tests__/fixtures/isomorphic-server-only-hmr/src/components/implicit-component.markosrc/__tests__/fixtures/isomorphic-server-only-hmr/src/components/layout-component.markosrc/__tests__/fixtures/isomorphic-server-only-hmr/src/index.jssrc/__tests__/fixtures/isomorphic-server-only-hmr/src/template.markosrc/__tests__/fixtures/isomorphic-server-only-hmr/test.config.tssrc/__tests__/main.test.tssrc/index.ts
✅ Files skipped from review due to trivial changes (3)
- src/tests/fixtures/isomorphic-server-only-hmr/src/components/implicit-component.marko
- src/tests/fixtures/isomorphic-server-only-hmr/src/template.marko
- src/tests/fixtures/isomorphic-server-only-hmr/src/components/class-component.marko
rturnq
force-pushed
the
virtual-file-hmr
branch
from
0e6e843 to
d4fae7c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
+ Coverage 77.53% 81.41% +3.88%
==========================================
Files 14 15 +1
Lines 819 931 +112
Branches 221 246 +25
==========================================
+ Hits 635 758 +123
+ Misses 142 127 -15
- Partials 42 46 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
No description provided.