AB#131731 chat-components: DOM compatibility regression check#244
AB#131731 chat-components: DOM compatibility regression check#244
Conversation
…t release
Renders every supported <Message> variant (core layouts + all demo-page
tabs) against the branch's built dist/ and the latest published
@cognigy/chat-components, and asserts the normalized DOM matches. Guards
against silent regressions in the default webchat layout consumers
depend on.
Runs as its own GitHub Action ("DOM Compatibility") so a break shows as
a distinct PR check. The baseline is resolved at CI time via `npm view`
so we never drift against a stale pin. Excluded from the default `npm
test` because it requires a dist/ build and the dynamically-installed
alias.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ibility workflow - eslint.config.js: add a `**/*.mjs` block declaring `globals.node`. Flat config ignores legacy `/* eslint-env node */` directives, so without this `console`/`process` in our install script tripped `no-undef` under CodeQL's ESLint pass (local `npm run lint` only scans .ts/.tsx so it missed this). - scripts/install-dom-compat-baseline.mjs: drop the stale `/* eslint-env node */` comment that no longer does anything. - .github/workflows/dom-compat.yml: drop the single-value matrix. With a matrix GitHub appends "(22.x)" to the PR-check title, which reads as if "22.x" were the release being compared against. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`npx prettier --check .` on CI flagged three whitespace issues in test/layouts/dom-compat.spec.tsx (arrow-body / call-args that fit on a single line). Ran `prettier --write` on the file to fix. No semantic changes; 29/29 dom-compat tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Decouple the DOM-compatibility regression spec from the layout-variant work in PR #242 (closed) so it can land independently against main. - Move test/layouts/dom-compat.spec.tsx → test/dom-compat.spec.tsx and remove the now-empty test/layouts/ directory; update path references in vite.config.ts, vitest.dom-compat.config.ts and the spec docstring. - Bring along the small test/fixtures/layout-messages.ts helper (the spec's only layout-PR-derived dependency — pure fixture data, no runtime coupling). - Add tsconfig.json `exclude` for test/dom-compat.spec.tsx since the spec imports `../dist/chat-components.js` and `chat-components-baseline` which only exist after the dom-compat workflow's build + install steps. The previous test/layouts/ location was implicitly skipped by the `test/*.spec.tsx` include glob. - Drop the `<Message layout="webchat">` explicit-prop assertion. Without the layout prop on main, only the default render path remains. - Reword the spec preamble + dom-compat workflow comment so neither references PR #242 / "default webchat layout"; the check now stands on its own as a generic Message DOM backward-compatibility guard. Local verification: lint, tsc, `npm run test` (121/121), `npm run test:dom-compat:install-baseline`, `npm run build`, `npm run test:dom-compat` (28/28) all pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated DOM-compatibility regression check that renders <Message> variants from this branch and compares normalized DOM output against the latest published @cognigy/chat-components, with a standalone CI workflow to surface DOM contract breaks independently of unit tests.
Changes:
- Introduces
test/dom-compat.spec.tsxplus a dedicated Vitest config to run it separately fromnpm test. - Adds a script to install the latest published package at CI/runtime as
chat-components-baseline. - Updates repo config (Vite/Vitest, TS, ESLint) and adds a GitHub Actions workflow to run the check.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.dom-compat.config.ts | New Vitest config that runs only the DOM-compat spec. |
| vite.config.ts | Excludes the DOM-compat spec from the default npm test run. |
| tsconfig.json | Excludes the DOM-compat spec from tsc typechecking. |
| test/fixtures/layout-messages.ts | Adds fixture messages used by the DOM-compat spec. |
| test/dom-compat.spec.tsx | New DOM-compat regression suite comparing branch vs latest published baseline. |
| scripts/install-dom-compat-baseline.mjs | Installs latest published package as an alias for baseline comparison. |
| package.json | Adds npm scripts for baseline install and dom-compat test run. |
| eslint.config.js | Declares Node globals for .mjs scripts under flat ESLint config. |
| .github/workflows/dom-compat.yml | New workflow to run the DOM-compat check as its own PR status check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The 'engagement message' case was passing without exercising any DOM.
matcher.ts gates engagement-source messages behind
`settings.teaserMessage.showInChat`; without that config flag both the
current branch and the baseline render `null`, so the comparison
collapses to empty === empty and any DOM regression in the engagement
render path would go undetected.
Verified by perturbing src/messages/Message.tsx with a sentinel
attribute on `<article>` and confirming the case fires (previously 27/28
caught the perturbation, now 28/28).
Two-part fix:
- Pass `config={ settings: { teaserMessage: { showInChat: true } } }`
for the engagement case so the matcher actually selects a plugin.
`assertSameDom` now forwards an optional `config` prop to both
branches so they render symmetrically.
- Add `expect(currentHtml).not.toBe("")` so any future fixture that
silently fails to match a plugin trips a loud assertion instead of
passing on emptiness. Cheap belt-and-braces against the same trap.
No production code changes. Verification:
- `npm run test:dom-compat` — 28/28 pass clean.
- Perturbation experiment — 28/28 fail (engagement included).
- `npm run test` — 121/121, `tsc --noEmit`, `lint` all clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ssage types Pulls in every demo tab the spec previously skipped behind the "requires widgetSettings.config injection" excuse and renames the fixture helper to drop the layout-PR vestige. Coverage growth: 28 → 40 cases (12 new). Each new case was verified end-to-end by reintroducing the perturbation experiment from commit 0f2fed4 (sentinel attribute on `<article>`) — all 40/40 cases now fail under that perturbation, confirming none of the additions is a vacuous empty-vs-empty pass. New cases (each cited from test/demo.tsx): - Adaptive Cards [1] and [2] (was just [0]). - Default Preview x2 — exercises the `enableDefaultPreview` branch. Both fixtures encode "RENDER OK" in `_defaultPreview` and "RENDER WRONG" in `_webchat`, so a regression that flipped channel selection fails the comparison. - xApp Buttons x2 — quick-reply pill (`_default._quickReplies` + `_webchat.quick_replies` with `content_type: "openXApp"`) and template button (`attachment.template_type: "button"` with `type: "openXApp"`). - HTML Sanitization x3 — default tags, `customAllowedHtmlTags: ["p", "strong"]`, and `disableHtmlContentSanitization: true`. Default-config case is already covered by `bot text message`. - Markdown text + borderless text — exercises the `renderMarkdown` and `disableBotOutputBorder` branches inside Text.tsx. - Collated bot follow-up with `prevMessage` — header-suppression path through the matcher's collation rules. Mechanical changes: - `Case` type gains optional `prevMessage`; `assertSameDom` forwards it to both renders symmetrically. - Per-tab widget configs (`defaultPreviewConfig`, `customAllowedTagsConfig`, `sanitizationDisabledConfig`, `renderMarkdownConfig`, `disableBorderConfig`) extracted as named constants rather than inlined per case. - Spec docstring updated to drop the now-obsolete skip list — only "UI Components" (not Message-rendered) and "Streaming messages" (animationState changes DOM over time) remain genuinely out of reach. File rename: `test/fixtures/layout-messages.ts` → `test/fixtures/messages.ts`. The "layout-" prefix was a vestige of the closed PR #242 layout-variant work and read oddly here. New fixtures live alongside the existing source-variant + plugin-payload exports in the renamed file. Verification: tsc --noEmit clean, lint clean, `npm run test` 121/121, `npm run test:dom-compat` 40/40, perturbation experiment 40/40 fail. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // collapse whitespace between tags | ||
| .replace(/>\s+</g, "><") | ||
| // trim leading/trailing whitespace | ||
| .trim() |
There was a problem hiding this comment.
normalize() removes all whitespace between tags via />\s+</, which also strips intentional single-space text nodes (e.g. </strong> <em>). That can hide real DOM/text regressions; consider only stripping indentation/newline whitespace or otherwise preserving meaningful spaces while still normalizing formatting.
| // collapse any resulting double spaces inside attribute values | ||
| .replace(/ +/g, " ") |
There was a problem hiding this comment.
The final .replace(/ +/g, " ") runs on the entire HTML string, not just inside attributes, so it can collapse meaningful multiple spaces in rendered text content (e.g. inside <pre> or intentionally spaced strings). If the goal is to de-dupe class/attr spacing after canonicalization, constrain this normalization to attribute values (or at least to class="...").
| // collapse any resulting double spaces inside attribute values | |
| .replace(/ +/g, " ") | |
| // collapse any resulting double spaces only inside class attribute values | |
| .replace(/\bclass=(["'])([^"']*)\1/g, (_match, quote: string, classValue: string) => { | |
| const normalizedClassValue = classValue.replace(/ +/g, " ").trim(); | |
| return `class=${quote}${normalizedClassValue}${quote}`; | |
| }) |
| `[dom-compat] installing ${ALIAS}@npm:${PKG_NAME}@${latest}` + | ||
| (installed ? ` (replacing ${installed})` : "") + | ||
| "...", | ||
| ); | ||
| run(`npm install --no-save --no-package-lock --no-audit --no-fund ${ALIAS}@npm:${PKG_NAME}@${latest}`, { | ||
| stdio: "inherit", | ||
| }); |
There was a problem hiding this comment.
npm install --no-save typically still updates package-lock.json; so running this script locally can dirty the lockfile even though the header comment says it won't. If you want to avoid modifying the lockfile, add the appropriate npm flag (and/or adjust the comment to reflect actual behavior).
| import { IMessage } from "@cognigy/socket-client"; | ||
|
|
||
| // ----- Source-variant text messages ----- | ||
|
|
||
| export const botTextMessage: IMessage = { |
There was a problem hiding this comment.
PR description mentions test/fixtures/layout-messages.ts, but this PR adds test/fixtures/messages.ts (and there is no layout-messages.ts under test/fixtures). Update the PR description to match the actual file(s) introduced so reviewers know what moved/was added.
…back Five small follow-ups after the four web-UI suggestion accepts. All local checks clean (40/40 dom-compat including post-fix perturbation sweep, 121/121 unit tests, tsc, lint, prettier). scripts/install-dom-compat-baseline.mjs - Re-wrap the long `npm install` command across multiple lines so prettier-check on CI passes (the `Format` workflow was failing on this single line after the `--no-package-lock` accept). vitest.dom-compat.config.ts - Reword the header comment that claimed Vitest "picks up plugins from vite.config.ts via the resolve config." That's not how `vitest --config` works — this file must restate every field Vitest needs (plugins, environment, setup files, css modules, resolve aliases). Comment now reflects that. test/dom-compat.spec.tsx - normalize(): tighten the inter-tag whitespace strip from `>\s+<` to `>\s*[\r\n]\s*<` so it only collapses INDENTATION (whitespace runs containing a newline). Single intentional spaces between inline elements like `</strong> <em>` are preserved, so a regression that drops or adds them is caught instead of normalized away. - normalize(): scope the trailing double-space collapse from a global ` +/g` to attribute-value contents only, via `="([^"]*)"` → callback. The intent had always been to clean up duplicate spaces left in `class="foo bar"` after CSS-module canonicalization; running the rule on the whole HTML also collapsed meaningful spaces in text content (e.g. inside `<pre>`). - Reword the stale "demo-page fixtures" header comment that listed collation as skipped — collation became a covered case in 15bdbf7. Also add a parallel one-line heading on the per-source fixture imports so the two import blocks read as parallel sections. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s-main drift Two related fixes after the first real-world failure on PR #244 (caught a published-but-not-merged sibling release): a published 0.71.0 (from the c26 theme PR) shipped a gallery aria-controls accessibility fix that main's source doesn't have yet, so the dom-compat check naturally reported the drift. The check is doing its job, but the divergence isn't something this PR's branch can fix — and we shouldn't block landing on it. scripts/install-dom-compat-baseline.mjs - Change baseline selection from "always npm latest" to `min(npm latest, working tree version)` via numeric MAJOR.MINOR.PATCH compare. When the working tree advances past npm latest (the normal case during dev), npm latest is still the baseline — drift detection is preserved. When the working tree is behind npm latest (anomaly), the script pins to the working tree's own version, degrading to rebuild-vs-itself with a clear notice rather than reporting a divergence the branch can't fix. - Reword the file preamble to document this policy explicitly. .github/workflows/dom-compat.yml - Reorder steps: `npm ci` → `npm run build` → install-baseline → test. Previously the build ran AFTER install-baseline, but `npm install --no-save chat-components-baseline@npm:...` still resolves and writes to node_modules (only the lockfile is left alone) — locally we saw it touch ~204 packages. Building first means our `dist/` is produced with the exact lockfile-pinned dependency tree, not the alias-shifted tree that exists post-install-baseline. Without this fix the comparison wasn't honest: branch source was bundled with potentially-different transitive versions than the baseline npm package was bundled with at publish time. Verification (npm ci → build → install-baseline → test): - 40/40 dom-compat pass. - Negative test (perturb DOM in Message.tsx) — 40/40 fail. Safety net intact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 3. CSS-module hashed class names. The published baseline build emits | ||
| // classes in the default hashed form (`_header_21mid_1`, `_incoming_21mid_8`, | ||
| // `_title2-regular_1ltiv_41`), while the branch-under-test source is | ||
| // loaded by Vitest with `classNameStrategy: "non-scoped"` (see | ||
| // vite.config.ts test.css.modules), which yields plain class names | ||
| // (`header`, `incoming`, `title2-regular`). This is a build-time | ||
| // packaging difference, not a DOM-structural one, so we canonicalize | ||
| // both shapes to the plain name before comparing. |
There was a problem hiding this comment.
The comment explaining CSS-module class normalization is inconsistent with how this spec actually runs: both CurrentMessage and BaselineMessage are imported from built dist bundles, so neither side is using Vitest's classNameStrategy: "non-scoped" output. Consider updating this comment to explain that normalization is needed because CSS-module hash suffixes can differ between builds/releases even when the DOM structure is identical, not because one side is un-hashed.
| // 3. CSS-module hashed class names. The published baseline build emits | |
| // classes in the default hashed form (`_header_21mid_1`, `_incoming_21mid_8`, | |
| // `_title2-regular_1ltiv_41`), while the branch-under-test source is | |
| // loaded by Vitest with `classNameStrategy: "non-scoped"` (see | |
| // vite.config.ts test.css.modules), which yields plain class names | |
| // (`header`, `incoming`, `title2-regular`). This is a build-time | |
| // packaging difference, not a DOM-structural one, so we canonicalize | |
| // both shapes to the plain name before comparing. | |
| // 3. CSS-module hashed class names. Both CurrentMessage and | |
| // BaselineMessage are imported from built dist bundles, so both sides | |
| // use hashed CSS-module output. Those hash suffixes can still differ | |
| // between builds/releases (`_header_21mid_1`, `_incoming_21mid_8`, | |
| // `_title2-regular_1ltiv_41`) even when the underlying DOM structure | |
| // and logical class identity are unchanged. That is a build-artifact | |
| // difference, not a DOM-structural one, so we canonicalize hashed | |
| // class tokens to their plain local names before comparing. |
The "non-scoped vs hashed" framing in normalize()'s comment was left over from when only one side imported from src/ via Vitest. Now that both `CurrentMessage` and `BaselineMessage` come from built dist bundles, the actual reason for canonicalization is different: the CSS-module hash suffix is content-derived per build, so the same logical class can carry a different hash between releases (or even between two rebuilds of the same source after a node_modules shuffle) without any DOM-structural change. Reword the comment to describe what the regex actually does. The plain-name fallback note for `classNameStrategy: "non-scoped"` is preserved as a "convenient if anyone ever runs the spec against non-dist source" aside. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Extracts the DOM-compatibility regression test from PR #242 (closed) into its own PR so the safety net can land independently of the layout-variant work.
The check renders every supported
<Message>variant on this branch and asserts the normalized DOM matches the same fixture rendered by the latest published@cognigy/chat-componentsrelease. A failure means the branch would silently break consumers of the existing DOM contract.What's in the box
test/dom-compat.spec.tsx— 40 cases covering everyMessageSendervariant, both plugin payload shapes (generic gallery, quick replies), every demo-page tab that renders via<Message>, plus the gated branches (Default Preview, HTML Sanitization custom-tags / disabled, xApp buttons, markdown / borderless text variants, message collation).test/fixtures/messages.ts— fixture helpers consumed by the spec: per-source bot/user/agent/engagement, gallery + quick-replies plugin payloads, Default Preview / xApp / sanitization / markdown / collation contrast fixtures.scripts/install-dom-compat-baseline.mjs— resolves the latest published version at CI time (npm view) and installs it as thechat-components-baselinealias with--no-save --no-package-lockso it never dirties the lockfile.vitest.dom-compat.config.ts— narrowsincludeto just this spec; the mainvite.config.tsexcludes it fromnpm testbecause it has preconditions (dist/build + the dynamic alias).tsconfig.json— excludes the spec fromtscfor the same preconditions..github/workflows/dom-compat.yml— dedicated GitHub Action so a DOM-compat break shows as its own PR check, separate from the unit-test workflow.eslint.config.js— adds a**/*.mjsblock withglobals.nodeso the install script doesn't tripno-undefunder CodeQL's ESLint pass (localnpm run lintonly scans.ts/.tsx).How the comparison stays honest
dist/on both sides goes through the same Vite production CSS-module pipeline. Avoids false positives from Vitest'sclassNameStrategy: "non-scoped"(phantom classes from missing keys, collapsed scopes from key collisions).useIdtokens (both:r0:and«r0»shapes), react-tooltip ids, UUIDs, swiper wrapper hashes; canonicalizes hashed CSS-module class names (_header_21mid_1→header); and collapses double spaces that arise inside attribute values from that canonicalization (scoped to="..."so meaningful spaces in text content are preserved).nullon both sides and trivially pass — that exact gap was caught and fixed for engagement messages in0f2fed4.Test plan
npx tsc --noEmit— cleannpm run lint— cleannpm run test— 121/121npm run test:dom-compat:install-baseline— installs currentlatest(0.70.0) as the aliasnpm run build— producesdist/chat-components.jsnpm run test:dom-compat— 40/40DOM compatibilityworkflow runs green on this PRNotes for reviewers
b901373,99df823,fb2183f) plus follow-up commits that decouple them from the layout-variant work, plug coverage gaps, and address review feedback.ActionButtons/Typography/ChatEventdirectly, not via<Message>) and "Streaming messages with markdown" (animationState transitions change the DOM over time, so a static comparison would be flaky).🤖 Generated with Claude Code