Files

T

Dmitry VerkhoturovandGitHub 23be25d84a Fix seven widget defects, including the cookies the separate-domain setup needs (#2197 )

* Drop the frontend workspace root and re-resolve the lockfile

`frontend/` carried a `package.json`, a `pnpm-workspace.yaml` and the lockfile
for a workspace of exactly one package. Two manifests meant two places to
declare a version, and the app pin was the one that did not win: `preact` and
`@babel/core` were each written twice, and a bump to the app manifest alone
would have been a silent no-op, since `pnpm.overrides` decides and it lived at
the root.

Everything pnpm reads now lives in `frontend/apps/remark42`: dependencies,
`packageManager`, `engines` and the overrides. `frontend/` keeps `.nvmrc`,
`.husky` and `CLAUDE.md`, none of which pnpm reads. The directory nesting
stays: every path in the repository points at `frontend/apps/remark42`,
including the published contributing docs, so moving the package up would have
rewritten 14 files to no benefit.

Moving the manifest kept the old resolutions verbatim, which left optional peer
subtrees the tree no longer reaches: `ts-node` under jest, `@swc/core` under
webpack, `vitest` under `@testing-library/jest-dom`, `tslib` under
`webpack-dev-server`. None is referenced by any config or source file here.
Re-resolving drops 137 packages and moves 59 to versions already permitted by
the ranges in the manifest, 1446 to 1308, with no direct dependency changing
version: the five that look changed differ only in their peer suffix. Every
file `pnpm build` produces is identical in size before and after.

The frontend-deps stage of the Dockerfile sets `CI=true` so the `prepare`
script skips husky, which has no git repository to install hooks into there.

* Stop markdown-only changes triggering heavy workflows, and check the documented versions

`ci-backend.yml`, `ci-build.yml` and `ci-frontend.yml` all end their path
filters with `!**.md`. The e2e workflow did not, so a change to any markdown
file under `frontend/` or `backend/` matched its `frontend/**` and `backend/**`
entries and started a docker build and the whole browser suite. The release
filter had the same hole and two of its own: it names `README.md` and `LICENSE`
on purpose, since `.goreleaser.yml` packages both, so it now excludes markdown
under `backend/` and `frontend/` only. `CLAUDE.md` and the installation page
were listed as well, and neither is packaged.

`ci-site.yml` goes on matching markdown, which is right, since the site is
built from it. It excludes `CLAUDE.md`, so a future `site/CLAUDE.md` cannot
start a site build, and `site/README.md`, which documents how to build the site
rather than being part of it.

The installation page tells a reader that a source build needs Go 1.25, Node
24+ and PNPM 10. Nothing kept those in step with `backend/go.mod`,
`engines.node`, `packageManager` and `.nvmrc`, and the drift is silent: a wrong
version in the docs builds and tests exactly as well as a right one. `.nvmrc`
is the pin with form here, having sat at 16 through the whole node 20 migration
because nothing red ever pointed at it. The check compares each stated version
against its source and holds `.nvmrc` to `engines.node`, and it fails when the
page states no version at all, so removing the claims cannot turn it into a
check that passes by comparing nothing.

Its own workflow rather than a step in an existing one, since the inputs span
the backend module, the frontend manifest and the site.

* Fix the cookie fallback page, asset path, message senders, auth teardown and cookies

Two defects with the same origin: 5825a55b, the January 2021 frontend
rewrite, first released in v1.7.0.

It removed the build entry for comments.html while leaving both the
template and the link to it in place, so the page the auth panel offers
when third-party cookies are blocked has been a 404 ever since, for
exactly the reader who has no other way in. The template needed no
changes; it is built again, and an e2e case now opens it on a thread
carrying a comment and waits for that comment, so the page being served,
its inline script running and it asking for the thread named in its own
query string are all covered. Against an image built without the plugin
entry that case fails on the 404, which is the regression it exists for.

It also fixed the public path to the domain root, so an instance mounted
under a prefix, which manuals/separate-domain documents, asked for
/web/google.svg when its own icons live under that prefix. Fifteen
provider icons in remark.mjs and one in last-comments.mjs. The path is
now derived from the url the bundle was loaded from, which is correct for
both arrangements, and the file loader no longer overrides it.

The host page also accepted postMessage from any window: every frame on a
page can reach window.parent, and the handler resizes the widget, scrolls
the page and opens the profile overlay. It now ignores anything that did
not come from a frame this module created.

A fourth, in the same family: the OAuth flow never tore its polling down.
`subscribed` was declared, checked and cleared but never set, so the guard
against a second subscription was dead code and every provider click
attached another listener pair. The five minute deadline then rejected
without unsubscribing, leaving those listeners and a retry that
reschedules itself for as long as getUser returns null. Cross-domain is
where getUser never stops returning null, so a reader on the arrangement
manuals/separate-domain documents was left polling /auth/user once a
minute for the life of the page, against a route capped at 2 req/s. It
also rejected with no argument, and the caller stores that as the error
state, so the interface had undefined to render. The deadline now tears
the subscription down and rejects with an error.

The message check had a second half. Hardening the parent left the widget
document trusting any sender, and it acts on signout and theme, so
anything holding a reference to the frame could sign a reader out.
`auth.hooks` already checked `event.source !== window.parent`; that check
is now a shared `isFromParent` and the three listeners that lacked it use
it too. The origin cannot stand in for it, since the host page is
whatever site embeds the widget and `ALLOWED_HOSTS` is enforced server
side through `frame-ancestors`.

And createInstance stacked its listeners. It reuses the marked iframe
instead of building one, but installed three listeners plus a title
observer on every call, while destroy could only reach the newest
closure, so a second call without a destroy stranded a set for good. The
listeners of the current instance are now detached before the next set
goes on. Reuse and the ignored config are unchanged: that contract is
open in the backlog note and not settled here.

The auth cookies the embedded case needs were not being delivered, in
both halves of the client's own writer. The name was decorated:
setAuthCookie prefixed with __Host- whenever the page was https, so a
real deployment wrote __Host-JWT and __Host-XSRF-TOKEN while the backend
looks for JWT and the fetcher reads XSRF-TOKEN, and nothing anywhere
reads a prefixed name. Nothing caught it because the prefix is applied
from the page protocol and every test and the dev server run on http;
there is now a second suite pinned to an https page, which is the only
condition that shows it. And the attributes could not be delivered: both
were SameSite=Strict, judged against the top-level site and not the
request's own origin, so a Strict cookie is never sent from a
third-party frame, which is the entire configuration this code exists
for. They now follow the embedding, Strict while the widget shares its
page origin and None with Secure and Partitioned once it does not, since
that is the only third-party form browsers still accept. Over http in a
third-party frame no combination works, and the strict form is written
instead of one the browser would reject outright.

That leaves the client half of #1877 working, whose reporter wanted
AUTH_SEND_JWT_HEADER for exactly this arrangement, and whose first half
merged as #1929. The server's own cookies still carry no Partitioned;
that is upstream work in go-pkgz/auth.

Two plan changes. A review pass corrected its central Path B premise,
which said the first document render is anonymous permanently, in every
configuration: it is anonymous in the configuration remark42 ships,
go-pkgz/auth exposing XSRFIgnoreMethods and remark42 leaving it unset.
The door is not shut, it is closed by a setting, and opening it is
scoped security work and not a flag flip, because GET /deleteme
deletes every comment a user has written and is a GET so the emailed
link works. And the separate-domain arrangement is promoted from a
constraint bullet to a named requirement with acceptance criteria, since
a test that signs in and posts without reloading passes while
persistence is entirely broken.

Review found a seventh, and it was reachable only because of the first:
comments.ejs built its title with innerHTML from the url query
parameter, so restoring the build entry made a reflected XSS live on the
instance origin, where the page is a top-level document, frame-ancestors
does not apply and the /web CSP allows unsafe-inline. The anchor is now
built through the DOM with textContent, and only http and https reach
href, since escaping alone leaves a javascript: url working. Two e2e
subtests pin both halves, and mutation testing separates them: restoring
innerHTML fails four assertions, while keeping the escaping and dropping
only the scheme guard fails the href one alone.

Review also found the poll teardown test did not exercise the poll.
handleWindowVisibilityChange is reachable only from the two listeners
and from the retry it schedules itself, and the test dispatched neither,
so no request was ever made and the assertion compared zero to zero; it
passed with the teardown reverted. It now dispatches focus, asserts
requests are being made and keep coming, and only then that they stop.
And the teardown could not cancel an in-flight getUser: a null resolving
after the deadline ran the code past the await and scheduled a fresh
retry with nothing left to clear it. A closure-local flag checked after
the await stops that, chosen over a second guard at the top of the
handler because only one of the two is detectable by mutation and this
is the one that prevents the stray timer rather than neutering it.

The inline handler in the iframe template accepted messages from any
window while acting on them through location.replace and document.title.
It now takes only the parent, the same check the host page side makes.

2026-08-22 12:34:24 -05:00

12 KiB

Raw Permalink Blame History

Frontend gotchas

Non-obvious constraints in the frontend toolchain and widget. Read before bumping dependencies or the node/pnpm versions, and before changing how the widget renders or how translations are extracted.

The node/pnpm version is pinned in many places, not one

CI staying green does not mean every pin is consistent — .nvmrc in particular is never read by CI, so it can silently drift. After changing the node or pnpm version, grep the whole repo and update every one of these, not just the ones CI exercises:

Dockerfile (production image) — FROM node:X-alpine and npm i -g pnpm@X.Y.Z
frontend/.nvmrc — not read by CI at all; only matters to a human running nvm use locally. This is the one that drifted unnoticed: it sat at 16 through the whole node-20 migration because nothing red ever pointed at it.
frontend/apps/remark42/package.json, both its packageManager field and its engines block
pnpm/action-setup@vN blocks in .github/workflows/ci-frontend.yml (5) and release.yml (2) — pin version: to the exact patch (e.g. 10.10.0), matching packageManager, not just the major. A floating major here is silent in CI (it just resolves to whatever the latest patch is at run time) but breaks the "Dockerfile and CI use the same pnpm" guarantee.
node: matrices in .github/workflows/ci-frontend.yml (every entry, not just the first) and the node-version: values in release.yml

When bumping pnpm/node, also re-check frontend/apps/remark42/package.json's engines field — it's separate from packageManager and won't update itself.

engines.node states the major we support, currently >=24, which is the active LTS; 22 has dropped to maintenance. @babel/core 8 wants ^22.18 || >=24.11 and size-limit 13 wants ^22.18 || ^24 || >=26, so 22 was the floor rather than the target. Individual dev dependencies can be stricter within that major (undici wants >=20.18.1); do not chase those patch floors into engines or the docs, or every lockfile refresh becomes a documentation change.

One manifest, at `frontend/apps/remark42`

frontend/ holds no package.json, no lockfile and no pnpm-workspace.yaml. Everything pnpm reads lives in frontend/apps/remark42: the dependencies, packageManager, engines and the pnpm.overrides block. Install and run from there, not from frontend/.

The directory nesting is kept because every path in the repository points at it, from the Dockerfile and the workflows to the published contributing docs. frontend/ still carries .nvmrc, .husky and this file, none of which pnpm reads.

pnpm 10's stricter `node-linker` layout needs explicit pins

One dep is pinned specifically because of pnpm 10's hoisting changes, not because of the dep itself:

@types/minimatch 5.1.2 — 6.x is an empty stub that the hoisted layout picks up instead of the real types

If a dependency bump mysteriously breaks types or module resolution only after a pnpm major bump, suspect the layout change before suspecting the dependency.

node's native fetch requires absolute URLs in tests

It rejects relative request URLs, and the failure is silent: requests simply never match. Any test harness that mocks fetch needs absolute base URLs and a jsdom base URL set.

Babel is the only thing that compiles

.babelrc.js drives both the bundle, through babel-loader, and the tests, which get it as babel-jest's configFile in jest.config.mjs. That indirection is not decoration: .babelrc.js is a file-relative config and would not reach node_modules, so the ESM-only packages in transformIgnorePatterns would arrive at jest untransformed.

Babel merges an env block over the root rather than replacing it, so anything the test run must not see has to be decided before the object is built rather than put in an env. jest.config.mjs sets BABEL_ENV itself and .babelrc.js reads it for two things: the compile targets, and the data-testid stripper, which the suites query by and the bundle must not carry. Verify with grep -c data-testid public/*.mjs after a production build; it must be 0. The plugin matches a literal JSXAttribute, which is every use in the tree; a spread such as {...{'data-testid': x}} would reach production, and that grep is what would catch it.

tsconfig.json types but never emits, so its target has no effect on output; browserslist in .babelrc.js decides that. Type checking runs out of band through fork-ts-checker-webpack-plugin.

Type-only imports have to say so

Babel compiles one file at a time with no type information, so it cannot tell that an import is types-only. Left unmarked, the module and everything it imports stay in the bundle, and a single type import of a store-connected component is enough to pull the whole redux graph into an entry that never uses it.

verbatimModuleSyntax in tsconfig.json and @typescript-eslint/consistent-type-imports are what enforce this, and the fix has to be a separate import type { ... } statement. Inline import { type X } does not work here: verbatim semantics keep the statement, so the module is still loaded. That is also why no-duplicate-imports runs with allowSeparateTypeImports, since a module legitimately appears twice.

JSX runs on the automatic runtime, in two places that must agree

preact 10.29 types a component's return as ComponentChildren, which only satisfies a JSX check on TypeScript 5.1+ via JSX.ElementType, and it scopes the JSX namespace to preact/jsx-runtime rather than declaring it globally. So the type layer has to use the automatic runtime:

tsconfig.json: jsx: react-jsx with jsxImportSource: preact
.babelrc.js: @babel/preset-react with runtime: 'automatic', importSource: 'preact'

Keep both in step. If babel were left on the classic pragma: 'h' transform, a new .tsx without import { h } would type-check and lint clean, then throw at runtime, because eslint-config-preact sets react/react-in-jsx-scope to 0 and the local config turns no-undef off.

`@babel/core` is pinned to 8 for the whole dependency tree

@jest/transform and istanbul-lib-instrument depend on @babel/core 7 outright, and a babel 8 preset loaded into a babel 7 core fails on the first enum it meets. The pnpm.overrides entry in frontend/apps/remark42/package.json is what stops that. eslint-config-preact is the one consumer that cannot take it: its @babel/eslint-parser loads babel 7 syntax plugins that babel 8 rejects, so a second scoped override, eslint-config-preact>@babel/core, holds that subtree on 7.

Held-back majors

These were deliberately not bumped because each is a config-migration or bundle-changing major, not a drop-in update — don't bump them opportunistically inside an unrelated dependency PR:

eslint 10 (eslint-config-preact peers on ^9), typescript 7, redux 5 with redux-thunk 3, node-emoji 2 (renames the field search() returns), @formatjs/cli 6 (the translation extraction pipeline), and postcss-preset-env 11 with cssnano 8 and postcss-html 2, which change generated CSS
redux 4

`/web` has a second source

privacy.html, markdown-help.html and 400x400.jpeg live in backend/app/webassets/assets and are embedded in the Go binary. The frontend build wins for any name present in both, which is what lets an operator override one by dropping a file next to the frontend files in --web-root. That directory replaces the embedded frontend outright when it exists, so an override belongs in a populated one. These three sit outside this toolchain: prettier, stylelint and pnpm lint do not see them, and they are served unminified. devServer.static lists the build output first and that directory second, matching the backend's order, so links to them resolve on the dev port too.

eslint config resolves from the process cwd

eslint.config.mjs lives in apps/remark42, and eslint loads the config next to the directory it is run from rather than the one nearest the file being linted. Anything that invokes eslint has to have apps/remark42 as its working directory: it is where every script runs, .husky/pre-commit cd's into it, and an IDE integration needs "eslint.workingDirectories": ["frontend/apps/remark42"].

Rules that only exist under a plugin's flat-config export are spread in explicitly; the block that re-enables project rules is the last entry in the array, because eslint-config-prettier (pulled in by prettierRecommended) switches several of them off and later entries win.

Verifying a build didn't regress

There's no automated build-output diff in CI. Before merging a dependency PR that touches the bundler/build tooling, manually diff the build output against a clean master checkout:

apps/remark42: expect webpack module-id numbers and css-module class tokens (e.g. .F_A → .L_A) to differ — that's normal churn from a webpack/css-loader bump. HTML, CSS values, and translation content should be byte-identical.
site: asset URLs carry a content hash, so a stylesheet or script change moves the filename on every page referencing it. Anything else differing is a real regression.

Where the alerts actually were

When clearing Dependabot/audit alerts, check whether the flagged package is actually reachable from production code or only from the dev/test toolchain — pnpm audit does not distinguish. Several alerts here were in build-time-only tooling (webpack-dev-server, laravel-mix-equivalent dev deps) with no patched release available; those are lower-risk than a runtime dependency with the same severity label.

Don't import `preact/compat`

It installs hooks on preact's shared options that remap onFocus/onBlur to focusin/focusout on every element and make @testing-library/preact rewrite change to input, so one import changes event behaviour across the whole widget, and it adds about 3.8 kB gzipped.

forwardRef lives only there, so a component that needs a ref takes it as an ordinary prop; TextareaAutosize is the pattern. For a component type annotation use FunctionComponent from preact, not React.FC.

i18n is a hand-written binding whose export names are fixed by the extractor

app/common/intl.tsx provides IntlProvider, useIntl, createIntl, defineMessages, FormattedMessage and IntlShape. formatjs extract (translation:extract) finds messages by recognising the identifiers defineMessages, FormattedMessage and intl.formatMessage in the AST, not by import source, so those three names are fixed. Rename any of them and extraction returns nothing, with no error and a zero exit code.

The damage lands on the next step. translation:check compares locale keys against extracted keys and fails loudly, but tasks/generateDictionary.js calls removeAbandonedKeys, so translation:generate after a rename deletes the now-unextracted keys from all 24 files in app/locales/ and writes them back, after which the check passes. Extract-then-generate is the documented translator workflow, so the destructive order is the normal one.

After any rename or refactor touching those identifiers, verify the count rather than the exit code:

pnpm translation:extract
node -e "console.log(Object.keys(require('./extracted-messages/messages')).length)"

It must match the key count in app/locales/en.json, not drop to 0.

The binding implements {name} interpolation and paired <tag>text</tag> rich text, and nothing else of ICU: no plural, select or selectordinal, no typed arguments such as {n, number}, and no apostrophe quoting. A plural or select form has braces the placeholder rule rejects, so it falls back to English wherever values are passed and shows its raw text where they are not; '' simply stays as two apostrophes.

translation:check validates every translated value's markup and placeholders against the English string it translates, mirroring the binding's rule, and the catalogue sweep in app/common/intl.test.tsx additionally renders the two messages that carry a link. What neither catches is unsupported ICU syntax, since a plural form is well-formed text as far as both are concerned. A message the binding cannot resolve falls back to the English source rather than reaching the page, so without these checks a broken translation is invisible in the interface.

12 KiB Raw Permalink Blame History