Skip to content

Commit a4f96b7

Browse files
trialclaude
andcommitted
fix(browser): review of 1.40.0 -- declaredCanonical comes only from a canonical verdict, posts with a stored sitemap render too, http(s) and keyable only, reset on every verdict; v1.40.0
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 8739270 commit a4f96b7

4 files changed

Lines changed: 57 additions & 24 deletions

File tree

‎packages/browser/src/RenderJob.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -221,10 +221,11 @@ export default class RenderJob {
221221
reason: string | undefined;
222222
/**
223223
* The URL the page's canonical link names, when a non-indexable render's canonical names ANOTHER URL
224-
* (resolved against the rendered URL, fragment dropped). Unset otherwise, so the key is absent from
225-
* every result that has nothing to say and an older plugin never sees it. The plugin's entity registry
226-
* records it: on a site where every spelling of a product is one document, this is the origin naming
227-
* the product's canonical.
224+
* (resolved against the rendered URL, fragment dropped) — posted whether or not the render produced
225+
* content, so a sitemap target that is serialized anyway carries it too. Unset otherwise, so the key is
226+
* absent from every result that has nothing to say and an older plugin never sees it. The plugin's
227+
* entity registry records it: on a site where every spelling of a product is one document, this is the
228+
* origin naming the product's canonical.
228229
*/
229230
declaredCanonical: string | undefined;
230231

@@ -374,8 +375,10 @@ export default class RenderJob {
374375
reason: this.content
375376
? undefined
376377
: (this.reason ?? (attemptError ? 'error' : this.redirectedTo ? 'redirect' : undefined)),
377-
// Under the same rule as `reason`: a render that produced content has no verdict to explain.
378-
declaredCanonical: this.content ? undefined : this.declaredCanonical,
378+
// With content too: a sitemap target is serialized even when its canonical names another URL, and
379+
// that is exactly the re-slugged URL whose new canonical the plugin wants. Not the rule `reason`
380+
// follows, because this is not why there is no content; it is what the page said.
381+
declaredCanonical: this.declaredCanonical,
379382
// The failed attempt's detail — without it the plugin can only log "unknown
380383
// prerender error". `phase` separates a navigation that never completed (slow
381384
// origin) from a failure in the settle/serialize work. Anything can be thrown, so
@@ -545,7 +548,7 @@ export type VariantMetadata = {
545548
documentReused: true | undefined;
546549
documentPrefetched: true | undefined;
547550
reason: string | undefined;
548-
/** See `RenderJob.declaredCanonical`: present only on a non-indexable result whose canonical names another URL. */
551+
/** See `RenderJob.declaredCanonical`: present only on a non-indexable result whose canonical names another URL, content or not. */
549552
declaredCanonical: string | undefined;
550553
error: { name: string; message: string; phase: string | undefined } | undefined;
551554
};

‎packages/browser/src/renderer.ts‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,8 +1143,9 @@ const renderer: Renderer = async (page, job) => {
11431143
job.isIndexable = verdict.isIndexable;
11441144
if (!verdict.isIndexable) {
11451145
job.reason = verdict.reason;
1146-
job.declaredCanonical = verdict.declaredCanonical;
11471146
}
1147+
// Assigned on every verdict, so an earlier attempt's never outlives a later one's.
1148+
job.declaredCanonical = verdict.declaredCanonical;
11481149

11491150
if (job.isIndexable || job.isFromSitemap) {
11501151
// Before postProcess: it may strip nodes, and this must describe the page as
@@ -1310,12 +1311,11 @@ export function indexVerdict(
13101311
const verdict = canonicalVerdict(signals.canonicalHref, pageUrl);
13111312
const disowned = verdict === 'elsewhere' || (verdict === 'variant' && strict);
13121313
if (!signals.noindex && !disowned) return { isIndexable: true };
1313-
const declaredCanonical = verdict === 'self' ? undefined : declaredCanonicalOf(signals.canonicalHref, pageUrl);
1314-
return {
1315-
isIndexable: false,
1316-
reason: signals.noindex ? 'noindex' : verdict === 'variant' ? 'canonical-variant' : 'canonical-mismatch',
1317-
...(declaredCanonical ? { declaredCanonical } : {}),
1318-
};
1314+
const reason = signals.noindex ? 'noindex' : verdict === 'variant' ? 'canonical-variant' : 'canonical-mismatch';
1315+
// Only a CANONICAL verdict reports where the page points: a noindex page's canonical is a statement made by
1316+
// a page that asked not to be indexed, and is not what the plugin adopts targets from.
1317+
const declaredCanonical = reason === 'noindex' ? undefined : declaredCanonicalOf(signals.canonicalHref, pageUrl);
1318+
return { isIndexable: false, reason, ...(declaredCanonical ? { declaredCanonical } : {}) };
13191319
}
13201320

13211321
/**
@@ -1329,12 +1329,17 @@ function declaredCanonicalOf(canonicalHref: string | null, pageUrl: string): str
13291329
try {
13301330
const url = new URL(canonicalHref, pageUrl);
13311331
url.hash = '';
1332-
return url.href;
1332+
// A web page's URL, of a length that could ever be a cache key; anything else is not worth posting.
1333+
if (url.protocol !== 'https:' && url.protocol !== 'http:') return undefined;
1334+
return url.href.length <= DECLARED_CANONICAL_MAX ? url.href : undefined;
13331335
} catch {
13341336
return undefined;
13351337
}
13361338
}
13371339

1340+
/** Longest declared canonical posted: past this no Harper primary key could hold it (the limit is ~1,978 bytes). */
1341+
const DECLARED_CANONICAL_MAX = 2048;
1342+
13381343
function extractIndexSignals(): { canonicalHref: string | null; noindex: boolean } {
13391344
let canonicalHref: string | null = null;
13401345
let noindex = false;

‎packages/browser/test/earlySettleSkip.test.ts‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,14 @@ test('the post-settle verdict reports the declared canonical too, and an indexab
109109
assert.equal(fine.declaredCanonical, undefined);
110110
});
111111

112+
test('a SITEMAP target whose canonical names another URL is serialized, and still reports where it points', async () => {
113+
const listed = await render('/canonical-elsewhere', {}, true);
114+
await listed.close();
115+
assert.equal(listed.isIndexable, false);
116+
assert.match(listed.html ?? '', /<link rel="canonical"/, 'a listed URL is captured anyway');
117+
assert.equal(listed.declaredCanonical, `${base}/other`);
118+
});
119+
112120
// A 404's settle is skipped BY THE BAIL, not by the `status >= 400` abort: that abort only stops
113121
// subresource interception, so `goto` still resolves and the page would otherwise settle in full.
114122
// An earlier review argued the opposite (that `goto` rejects, making this arm dead code); this test
@@ -197,7 +205,6 @@ test('indexVerdict: the shared verdict both paths use', () => {
197205
assert.deepEqual(indexVerdict({ canonicalHref: 'https://x.test/other', noindex: true }, page, false), {
198206
isIndexable: false,
199207
reason: 'noindex',
200-
declaredCanonical: 'https://x.test/other',
201208
});
202209
});
203210

@@ -237,10 +244,20 @@ test('indexVerdict: a canonical naming another URL is reported, resolved against
237244
indexVerdict({ canonicalHref: '/product/prd-1/new-slug.jsp#top', noindex: false }, page, false).declaredCanonical,
238245
'https://x.test/product/prd-1/new-slug.jsp'
239246
);
240-
// A noindex page that also names another canonical still says where it points.
247+
// Only a canonical verdict reports it: a noindex page's canonical is not something to adopt a target from.
248+
assert.equal(
249+
'declaredCanonical' in indexVerdict({ canonicalHref: 'https://x.test/other', noindex: true }, page, false),
250+
false
251+
);
252+
// A web URL of a keyable length, or nothing.
253+
assert.equal(
254+
'declaredCanonical' in indexVerdict({ canonicalHref: 'ftp://x.test/other', noindex: false }, page, false),
255+
false
256+
);
241257
assert.equal(
242-
indexVerdict({ canonicalHref: 'https://x.test/other', noindex: true }, page, false).declaredCanonical,
243-
'https://x.test/other'
258+
'declaredCanonical' in
259+
indexVerdict({ canonicalHref: `https://x.test/${'a'.repeat(3000)}`, noindex: false }, page, false),
260+
false
244261
);
245262
// A strict variant names this document re-spelled: reported too, it is still the origin's spelling.
246263
assert.equal(

‎packages/browser/test/jobResult.test.ts‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ test('a noindex page posts outcome=non-indexable with the reason', async () => {
111111
assert.equal(meta.reason, 'noindex');
112112
});
113113

114-
test('a canonical verdict posts the canonical the page declared; a render with content never does', async () => {
114+
test('a canonical verdict posts the canonical the page declared — with or without content; an indexable render never does', async () => {
115115
const job = makeJob();
116116
job.attemptStarted();
117117
job.httpResponse = { statusCode: 200, headers: {} };
@@ -123,16 +123,24 @@ test('a canonical verdict posts the canonical the page declared; a render with c
123123
assert.equal(meta.outcome, 'non-indexable');
124124
assert.equal(meta.declaredCanonical, 'https://site.example.com/product/x-new-slug');
125125

126+
// A sitemap target is serialized even when non-indexable: its content and its declared canonical both post.
127+
const listed = makeJob();
128+
listed.attemptStarted();
129+
listed.httpResponse = { statusCode: 200, headers: { 'content-type': 'text/html' } };
130+
listed.isIndexable = false;
131+
listed.declaredCanonical = 'https://site.example.com/product/x-new-slug';
132+
listed.attemptEnded(undefined, '<html>listed</html>');
133+
const stored = await send(listed);
134+
assert.equal(stored.outcome, 'rendered');
135+
assert.equal(stored.declaredCanonical, 'https://site.example.com/product/x-new-slug');
136+
126137
// Absent, not null, when there is nothing to say: an older plugin never sees the key.
127138
const plain = makeJob();
128139
plain.attemptStarted();
129140
plain.httpResponse = { statusCode: 200, headers: { 'content-type': 'text/html' } };
130141
plain.isIndexable = true;
131-
plain.declaredCanonical = 'https://site.example.com/product/x-new-slug'; // left over from an earlier attempt
132142
plain.attemptEnded(undefined, '<html>ok</html>');
133-
const stored = await send(plain);
134-
assert.equal(stored.outcome, 'rendered');
135-
assert.equal('declaredCanonical' in stored, false);
143+
assert.equal('declaredCanonical' in (await send(plain)), false);
136144
});
137145

138146
test('a failed render posts outcome=error with the attempt error and derived reason', async () => {

0 commit comments

Comments
 (0)