Skip to content

test: cover client canonical metadata after hydration - #197

Merged
shigahi merged 3 commits into
classmethod:masterfrom
Vamsi-2581:codex/test-client-canonical-meta-194
Nov 10, 2026
Merged

shigahi merged 3 commits into
classmethod:masterfrom
Vamsi-2581:codex/test-client-canonical-meta-194

Conversation

@Vamsi-2581

@Vamsi-2581 Vamsi-2581 commented Nov 3, 2026 •

Copy link
Copy Markdown

Summary

  • Add regression test in test/generated-worker.test.mjs verifying that the generated client script correctly restores canonical and social URL tags when Notion client-side rewrites occur across root, slug, mapped title-id, and unmapped title-id routes.
  • Exercises the head observer directly by targeting document.head and validates that attributeFilter includes 'href'.

This separate regression-test follow-up was invited by the maintainer on #195 after the behavior fix merged in #194.

Validation

  • Not run locally. The repository's pull-request workflow runs npm run build and npm test.

AI assistance

Prepared with OpenAI Codex assistance.

@shigahi shigahi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up on #195 with this. Running the generated client script in node:vm works well, and the test passes locally on Node 24 and 26.

To see what it guards against, I reintroduced a few regressions in src/code.ts. The test catches the MY_DOMAIN reference from #195, updating only the first matching tag, dropping twitter:url, and the pre-#194 client. It misses three:

  1. Using location.pathname instead of the page ID for pages without a slug. The test opens the unmapped page at /<id>, where both give the same URL. Notion's own paths are /Title-<id>, so /Some-Page-${UNMAPPED} should be canonical at /${UNMAPPED}, as in the server-side test above.
  2. Dropping the PAGE_TO_SLUG[getPage()] lookup. A mapped page opened by page ID, such as /Some-Page-${OTHER}, should be canonical at /chapter.
  3. Removing 'href' from the head observer's attributeFilter. The test calls the callback directly, so the observe options are never checked.

Picking the head observer by its target, rather than by observers.length === 2 and observers[0], also makes the test independent of how many observers the script creates and in which order. With this, all three regressions fail the test:

class TestMutationObserver {
  constructor(callback) { this.callback = callback; observers.push(this); }
  observe(target, options) { this.target = target; this.options = options; }
}
// ...
const headObserver = observers.find((observer) => observer.target === document.head);
assert.ok(headObserver.options.attributeFilter.includes('href'));
// ...and call headObserver.callback([]) in simulateNotionHeadRewrite()

for (const [pathname, canonical] of [
  ['/', ''],
  ['/chapter', '/chapter'],
  [`/Some-Page-${OTHER}`, '/chapter'],
  [`/Some-Page-${UNMAPPED}`, `/${UNMAPPED}`],
]) {
  location.pathname = pathname;
  simulateNotionHeadRewrite();
  expectCanonical(canonical);
}

Two smaller points:

  • Most of the stubs are never reached: the pushState/replaceState bodies, body.classList, the createElement methods and the #notion-app lookup. This is enough:
    const history = {};
    const window = { history, XMLHttpRequest: { prototype: {} } };
    const document = {
      head: {},
      title: '',
      createElement: () => ({}),
      querySelector: () => null,
      querySelectorAll: (selector) => selectors.get(selector) || [],
    };
    The test sets location.pathname directly and doesn't go through the history API, so please also adjust the PR description, which says it exercises observer callbacks after navigation.
  • There are two blank lines before the new test; one is enough.

Please run npm test locally before pushing the update.

Vamsi-2581 and others added 2 commits November 10, 2026 18:23
…coverage

Removed unused tests and updated document properties.
The previous commit removed two server-side canonical tests from classmethod#192
and the title assertion for pages without a slug, which failed CI.
They cover the worker's HTML rewriting; the new test covers the client
script.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shigahi

shigahi commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

Thanks for applying the suggestions. The new test now catches all the regressions from the review.

The last commit also removed two existing tests from #192 ("a mapped page opened by page ID is canonical at its slug" and "the home page keeps its canonical URL and metadata") and title: null in "a page without a slug is canonical at its page ID, without home page metadata". That title: null is what failed CI. These tests aren't unused: they cover the worker's HTML rewriting, while your test covers the client script.

I pushed 4974002 to restore them. npm test passes locally (26 tests), and the PR now only adds the new test.

@Vamsi-2581

Copy link
Copy Markdown
Author

Thanks @shigahi! I've updated the test to target the head observer directly by its document.head target, check attributeFilter.includes('href'), verify canonical resolution across all 4 path patterns (root, slug, mapped title-id, unmapped title-id), simplified the stubs, removed the extra blank line, and updated the PR description.

@shigahi shigahi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this adds the first tests for the client script.

@shigahi
shigahi merged commit bb34c2d into classmethod:master Nov 10, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants