Skip to content

Commit 12b34db

Browse files
committed
fix: correct the contradicting comment, the -1 comparison, and the action step
Second delta review found three things, two of them mine from the round before. The block comment above the shim guard still ended with "swallowing here restores that behaviour", which the previous commit made false on both halves: the code no longer swallows, and swallowing was never the restored behaviour. Two adjacent comments asserted opposite things about the same line, and the stale one came first. Merged into one statement of what is actually being prevented, which is detachment and not reporting. The ordering assertion compared indexOf results directly, and indexOf returns -1 for a missing needle, which is less than any real index. So with the guard removed entirely, the one case the assertion exists for, it passed. Both indices are asserted present first now. The comment above also claimed three assertions where five follow it, so it named the wrong set; it now says how each was checked instead of counting them. The architecture page's new step 2 said everything under /__webjs/* is answered before root middleware. The server-action RPC endpoint is dispatched in handleCore, which is reached through next() after middleware runs, so that was wrong in the direction that matters: it would tell someone gating actions with auth or rate-limit middleware that their middleware does not run. Step 2 now names the set it actually covers, and the action endpoint is back in the routing step with a note that middleware does run for it.
1 parent 82e1fd2 commit 12b34db

3 files changed

Lines changed: 34 additions & 22 deletions

File tree

packages/server/src/dev.js

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3084,24 +3084,26 @@ function __webjsDirectEvents() {
30843084
const scope = {};
30853085
startReloadWorker(scope, EventSource, ${eventsUrl});
30863086
scope.onconnect({ ports: [{ start() {}, postMessage(m) {
3087-
// Nothing may throw out of here. The relay's fanout deletes a port whose
3088-
// postMessage throws, which is the right read for a REAL MessagePort (a
3089-
// throw there means the tab is gone) and the wrong one for this shim,
3090-
// whose postMessage runs application code synchronously: an overlay
3091-
// render that threw would permanently unsubscribe this tab and silently
3092-
// kill live reload for the rest of the page's life. The old fallback
3087+
// Nothing may throw out of here, and nothing may be silently dropped.
3088+
//
3089+
// The relay's fanout deletes a port whose postMessage throws, which is the
3090+
// right read for a REAL MessagePort (a throw there means the tab is gone)
3091+
// and the wrong one for this shim, whose postMessage runs application code
3092+
// synchronously: an overlay render that threw would permanently
3093+
// unsubscribe this tab and silently kill live reload for the rest of the
3094+
// page's life. So the throw is contained here.
3095+
//
3096+
// Contained, NOT swallowed, and the difference matters. The old fallback
30933097
// attached to the EventSource directly, where a handler throw detached
3094-
// nothing, so swallowing here restores that behaviour rather than adding
3095-
// a new one.
3098+
// nothing but still reached the console, and so does a throw out of the
3099+
// SharedWorker path's onmessage below. Only the DETACHMENT is being
3100+
// prevented; discarding the error would make this the one path where a
3101+
// dev-overlay bug leaves no trace, which would be a new behaviour rather
3102+
// than a restored one.
30963103
try {
30973104
if (m.type === 'reload') __webjsReloadWhenReady();
30983105
else if (m.type === 'webjs-error') __webjsApplyError(m.data);
30993106
} catch (_) {
3100-
// Reported, never swallowed. Detachment is the only thing being
3101-
// prevented: a throw out of an EventSource listener (the old shape) and
3102-
// out of the SharedWorker path's onmessage below both surface to the
3103-
// console, so discarding it here would hide dev-overlay bugs on this one
3104-
// path, which would be a new behaviour rather than a restored one.
31053107
console.error('[webjs] dev reload handler threw', _);
31063108
}
31073109
} }] });

packages/server/test/dev/reload-shared-connection.test.js

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,14 @@ test('dev serves the reload SharedWorker, and the client uses it with a direct E
6363
// The slice must end at the function's OWN closing brace, not at the
6464
// bootstrap that follows it: ending at `indexOf('if (typeof SharedWorker')`
6565
// still trails `}\ntry {`, which leaves the bootstrap's `try` inside the
66-
// slice and the first assertion below vacuous again. Verified by running the
67-
// counterfactual against a reconstructed client: each of the three
68-
// assertions below fails with the guard removed.
66+
// slice and the guard assertion vacuous again.
67+
//
68+
// Each assertion below was checked INDIVIDUALLY against the counterfactual,
69+
// not just the file as a whole. Checking the file is what let two vacuous
70+
// assertions through earlier here: the run went red on a later assertion and
71+
// the earlier one was recorded as discriminating without being looked at.
72+
// Removing the guard fails the `try`-present and ordering assertions;
73+
// removing only the `console.error` fails the reporting one.
6974
const fallbackStart = clientSrc.indexOf('function __webjsDirectEvents()');
7075
assert.notEqual(fallbackStart, -1, 'the fallback function is in the client');
7176
const fallbackEnd = clientSrc.indexOf('\n}\n', fallbackStart);
@@ -79,10 +84,15 @@ test('dev serves the reload SharedWorker, and the client uses it with a direct E
7984
);
8085
assert.match(fallbackBody, /\}\s*catch\s*\(_\)/, 'and catches the throw so the relay cannot drop the tab');
8186
assert.match(fallbackBody, /console\.error\(/, 'and reports it rather than discarding it');
82-
assert.ok(
83-
fallbackBody.indexOf('try {') < fallbackBody.indexOf('__webjsReloadWhenReady()'),
84-
'the guard opens BEFORE the reload call, not around something else',
85-
);
87+
// Both indices are asserted present FIRST. `indexOf` returns -1 for a
88+
// missing needle, and -1 is less than any real index, so a bare `<`
89+
// comparison passes when the guard is gone entirely, which is the one case
90+
// this assertion exists for.
91+
const tryAt = fallbackBody.indexOf('try {');
92+
const reloadAt = fallbackBody.indexOf('__webjsReloadWhenReady()');
93+
assert.notEqual(tryAt, -1, 'the guard is present at all');
94+
assert.notEqual(reloadAt, -1, 'the reload call is present at all');
95+
assert.ok(tryAt < reloadAt, 'the guard opens BEFORE the reload call, not around something else');
8696
assert.match(clientSrc, /catch\s*\(_\)\s*\{\s*__webjsDirectEvents/, 'a worker failure falls back');
8797
// The debounce (#1397) is part of the relay, so it ships in BOTH scripts.
8898
assert.match(clientSrc, /const RELOAD_QUIET_MS/, 'the reload debounce ships in the client fallback');

website/app/docs/architecture/page.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,10 @@ import { listPosts } from '#modules/posts/queries/list-posts.server.ts';</code-b
103103
<h2>Request Lifecycle</h2>
104104
<ol>
105105
<li><strong>HTTP request arrives</strong> at the Node HTTP server (or HTTP/2 if TLS configured).</li>
106-
<li><strong>Framework-internal assets and probes</strong> (<code>/__webjs/*</code>: the core runtime, the dev reload client, downloaded vendor bundles, <code>/__webjs/health</code> and <code>/__webjs/ready</code>) are served here, ahead of everything below. They depend on neither the app analysis nor the vendor importmap, so a cold instance must not gate them. In <strong>development only</strong>, <code>/public/*</code> plus the <code>/sw.js</code> and <code>/offline.html</code> root remaps and <code>/favicon.ico</code> are served here too, so a stylesheet is never queued behind the startup analysis.</li>
106+
<li><strong>Some framework-internal assets and probes are answered here</strong>, ahead of everything below, because they depend on neither the app analysis nor the vendor importmap and a cold instance must not gate them: the health and readiness probes (<code>/__webjs/health</code>, <code>/__webjs/ready</code>), the build-info probe (<code>/__webjs/version</code>), the core runtime (<code>/__webjs/core/*</code>), the dev reload client and its SharedWorker (<code>/__webjs/reload.js</code>, <code>/__webjs/reload-worker.js</code>), and downloaded vendor bundles (<code>/__webjs/vendor/*</code>). This is a named set, not all of <code>/__webjs/*</code>: the server-action RPC endpoint is NOT in it (see step 5). In <strong>development only</strong>, <code>/public/*</code> plus the <code>/sw.js</code> and <code>/offline.html</code> root remaps and <code>/favicon.ico</code> are served here too, so a stylesheet is never queued behind the startup analysis.</li>
107107
<li><strong>Root middleware</strong> (<code>middleware.ts</code>) runs next if present, for every request that was not already answered above.</li>
108108
<li><strong>103 Early Hints</strong> sent (prod only) with modulepreload URLs for the matched page.</li>
109-
<li><strong>Route matching</strong>: the router tries (in order) static files, user source modules, API routes (<code>route.ts</code>), then page routes. In production this is where <code>/public/*</code> is served, so a middleware that guards an asset still guards it.</li>
109+
<li><strong>Route matching</strong>: the router tries (in order) the server-action RPC endpoint (<code>/__webjs/action/&lt;hash&gt;/&lt;fn&gt;</code>), static files, user source modules, API routes (<code>route.ts</code>), then page routes. The action endpoint sits here rather than in step 2, so root middleware DOES run for a server action, which is what lets you gate actions with auth or rate limiting. In production this is also where <code>/public/*</code> is served, so a middleware that guards an asset still guards it.</li>
110110
<li><strong>Segment middleware</strong> chain runs (outermost → innermost) for the matched route.</li>
111111
<li>For <strong>pages</strong>: SSR pipeline runs (load page + layouts, render to HTML, inject DSD, collect metadata, stream response with Suspense).</li>
112112
<li>For <strong>API routes</strong>: the matched handler function runs, returns a Response.</li>

0 commit comments

Comments
 (0)