Modernize dependencies, add tests, and align consent handling - #238
Open
samkim wants to merge 43 commits into
Open
Modernize dependencies, add tests, and align consent handling#238samkim wants to merge 43 commits into
samkim wants to merge 43 commits into
Conversation
Keeps the subagent-driven-development workspace and spec/plan docs out of version control per project preference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The vitest.config.ts file is built as CommonJS (no "type": "module" in package.json), so import.meta is not allowed. Use the native __dirname global instead of reconstructing it from import.meta.url. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nking) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… YAML loader Task 2 (components/annotation.test.tsx) should only have added a test file, but also modified vitest.config.ts and vitest.setup.ts — Task 1's already- reviewed deliverables — introducing a broken virtual-module SVG mechanism, an unscoped CSS-module plugin, dead vi.mock() calls with wrong relative paths and fabricated data, and an undeclared transitive `yaml` dependency. Revert vitest.config.ts and vitest.setup.ts to Task 1's approved state, and add back only the yamlPlugin needed to load the real .yaml content files annotation.tsx imports. Add `yaml` as an explicit devDependency instead of relying on incidental pnpm hoisting. While re-verifying, found that Task 1's original resolve.alias entries (`find: /\.svg$/` and `find: /\.module\.css$/`) never actually worked: Vite's alias resolution does `importee.replace(find, replacement)`, and a suffix-only pattern only replaces the matched suffix, mangling ids like "../content/HNIcon.svg" into "../content/HNIcon<replacement>" instead of fully substituting them. This was never caught because no test exercised a real (unmocked) .svg/.module.css import until now. Fixed by anchoring the regexes to the full specifier (`/^.*\.svg$/`, `/^.*\.module\.css$/`), which makes the whole-string replacement Vite performs actually resolve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…form and stub ResizeObserver for @headlessui/react Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ongs to a later task)
Upgrades next (15.5.9 -> 16.2.12), react/react-dom (19.1.1 -> 19.2.8), @next/mdx, @types/react(-dom), eslint-config-next, and eslint per the modernization plan. Handles the Next.js 16 breaking changes this repo actually hit: - publicRuntimeConfig/getConfig() were removed; migrated the two static values (GA measurement ID, canonical URL base) to next.config.mjs's `env` key and read them via process.env in GTag.tsx, pages/index.tsx, and pages/[range].tsx. - Turbopack is now the default bundler for `next dev`/`next build`, but this repo's custom webpack rules (next-plugin-yaml, @svgr/webpack) have no Turbopack equivalent configured, so dev/build now opt out via the officially supported `--webpack` flag. - `next lint` was removed; migrated via the official next-lint-to-eslint-cli codemod to a flat eslint.config.mjs and a plain `eslint .` script, and pinned eslint to 9.39.5 (still within eslint-config-next's >=9.0.0 peer range) since eslint-config-next's bundled eslint-plugin-react@7.37.5 crashes under eslint 10 (removed `context.getFilename()`). - next dev auto-updated tsconfig.json's `jsx` to "react-jsx", which Next.js 16 requires. All Phase 0 vitest/Cypress tests and the production build pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ors on pre-existing patterns eslint-config-next@16.2.12 bundles eslint-plugin-react-hooks@7.1.1 (up from 5.2.0), which enforces react-hooks/set-state-in-effect as an error by default. This flagged 8 pre-existing setState-in-useEffect call sites that are working, test-covered patterns (URL fragment sync, scroll-into-view, annotation set toggling, portal creation). Refactoring the effect logic is out of scope for a dependency-upgrade task and risks behavior changes to code Phase 0's tests specifically protect, so each site is suppressed with a scoped eslint-disable comment and rationale instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Refreshes @headlessui/react, react-click-away-listener, slugify, @types/mdx, autoprefixer, cypress, and postcss to their latest compatible minor/patch releases. Packages requiring a major bump (typescript, vitest, tailwindcss, jsdom, eslint, @types/node, @types/jsdom, puppeteer-core, start-server-and-test) are left untouched per plan scope.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…der instead of client mount The unwrapped iframe fired the ns.html fetch for every visitor, JS-enabled or not, bypassing Consent Mode entirely. Revert GTMScript to the standard nested <noscript><iframe/></noscript> GTM snippet (matches authzed/web's production layout.tsx) and fix the test instead: assert the correctly nested markup via renderToStaticMarkup, since @testing-library/react's render() is a from-scratch client mount that can never observe noscript children. Also corrects the HubSpotLoader comment to attribute the async-vs-defer cross-test leakage to this test file's simplified next/script mock, not next/script's real production behavior (which returns null and never renders a host <script> element for the afterInteractive strategy used here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… using a non-production test key posthog-js doesn't expose window.posthog when initialized via the ESM entrypoint this app uses, so check the SDK's own opt-in/opt-out persistence (__ph_opt_in_out_<key> in localStorage) instead of waiting on a live capture request against a real project.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- CI: add the four env vars ExternalScripts/instrumentation-client need to activate (NEXT_PUBLIC_VERCEL_ENV/GTM_ID/POSTHOG_KEY/POSTHOG_HOST) to the cypress job so consent.cy.ts's dataLayer/localStorage assertions can actually run instead of timing out on every PR. - ExternalScripts: only push a gtag consent update when a real az-consent cookie decision exists; when absent, leave Google's own region-scoped, geo-based static default (set by ConsentModeDefaults) un-overridden instead of synthesizing one from the isEUVisitor() timezone heuristic. - instrumentation-client: restore the cookieless_mode explanatory comment that was dropped when porting from docs/instrumentation-client.ts. - README: document the Vercel env vars required for analytics/consent to activate in production, since they silently no-op otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- util/consent/debug.ts: getDebugCookie() no longer truncates cookie values at the second `=`, matching getCookieValue()'s split-and-rejoin in storage.ts. - util/consent/eu-detection.ts: remove isEUVisitor()'s stale module-level cache; the timezone lookup is cheap enough that caching wasn't worth the staleness risk (e.g. for the __consent_debug_eu override cookie). - components/ExternalScripts.tsx: warn once via console.warn when NEXT_PUBLIC_GTM_ID is missing in production so GTM's silent no-op is diagnosable; document why HubSpot intentionally gates on the `statistics` consent field (matches authzed/web and authzed/docs' established convention, verified against both); cross-reference EU_REGION_CODES against eu-detection.ts's EU_TIMEZONES list. - instrumentation-client.ts: warn once when NEXT_PUBLIC_POSTHOG_KEY is missing in production. - util/isProd.ts (new): shared `isProd` check, replacing the verbatim duplicate definitions in ExternalScripts.tsx and instrumentation-client.ts. - eslint.config.mjs: add `ignores` for coverage/.next/out so flat config stops linting generated/build output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t mocks
- components/annotation.tsx: surface eslint-disable comments that were
previously masked by react-hooks/set-state-in-effect only reporting
the first setState call per effect (setContent after setTitle in
AnnotationPopper; setAnnotationActive and setAnnotationSetActive('intro')
in the URL-fragment effect). Verified by temporarily removing the
existing disables and confirming eslint reports at most one violation
per effect regardless. Deduplicated the repeated 2-line rationale
comment down to one full copy plus short back-references at every
other site (components/AnnotationSetSelect.tsx included).
- util/consent/storage.ts: extract validateParsedCookie() so
readConsentCookie/parseConsentCookie share one JSON-parse-and-validate
path instead of duplicating it; remove unused, untested
consentedIdentify (ported from docs for parity but never called here).
- util/consent/index.ts: drop the consentedIdentify re-export.
- test/mockPosthog.ts (new): shared posthog-js mock factory for
PaperInfoMenu/annotation/AnnotationSetSelect/SelectionShare tests,
each still calling vi.mock() itself for Vitest's hoisting.
- components/SelectionShare.test.tsx: isolate the navigator.clipboard
stub to its own test via save/restore in try/finally, since
Object.assign-based mutation isn't undone by vi.restoreAllMocks();
verified jsdom has no clipboard property by default and that running
the file twice produces identical results.
- components/AnnotationSetSelect.test.tsx: drop unused `import React`
(tsconfig uses the react-jsx automatic runtime).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The eslint-disable-next-line comments added for the masked set-state-in-effect violations were themselves flagged as unused -- ESLint's rule only ever reports the first setState call per effect, so lines after the first are never independently flagged and a disable directive there is a no-op the linter complains about. Replaced with plain explanatory comments (no directive) noting the masking behavior and that a real disable will be needed if the effect is ever reordered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rod rename - Add a regression test for isEUVisitor()'s cache removal that calls it twice within a single module instance (no vi.resetModules() in between) so a reintroduced cache would fail it. - Add util/consent/debug.test.ts covering getDebugCookie(), including the literal "=" truncation regression it was previously fixed for. - Move the missing-NEXT_PUBLIC_GTM_ID console.warn out of GTMScript()'s render body and into ExternalScripts()'s existing mount-only useEffect, so it fires once per mount instead of on every render. - Rename util/isProd.ts's export to isVercelProduction to avoid confusion with the differently-scoped, NODE_ENV-based isProd locals in next.config.mjs and pages/api/preview/[ranges].ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… are both unset
Visiting any selection-range URL during local `pnpm dev` without
PREVIEW_ENDPOINT set crashed with `getaddrinfo ENOTFOUND undefined`,
since the endpoint fell back to `https://${process.env.VERCEL_URL}`
which is `https://undefined` outside of a Vercel deployment. Adds a
final local-dev fallback to http://localhost:3000, matching the value
already documented in the README and used by cypress.config.ts.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…scroll flag) chrome-aws-lambda@10.1.0 is unmaintained and pins puppeteer-core@^10.1.0 as a peer; the installed puppeteer-core@24.20.0 removed the internal module path its vendored Browser/BrowserContext shims required, crashing with MODULE_NOT_FOUND. Replace it with @sparticuz/chromium@140.0.0 (maintained, version-matched to puppeteer-core's expected Chrome revision), and call puppeteer-core directly instead of through chrome-aws-lambda's re-export. Also fixes an unrelated latent bug this unmasked: HighlightProvidedSelection checked the URL-derived selectionId before renderState.ranges, so any /_render/<range> OG-image request took the selectionId branch and returned before setting window._scrolled, timing out the API's waitForFunction. Reordered so the ranges-driven (query-param) branch, which is what the _render page actually relies on, is checked first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…env.d.ts CI's TSC Typecheck step ran before `next build`, and the old `next lint` script (which implicitly bootstrapped Next.js tooling and generated the gitignored next-env.d.ts, the only source of an ambient *.svg declaration) was replaced by plain `eslint .` as part of the Next 16 migration. Without next-env.d.ts present, tsc had no module declaration for the *.svg imports in SelectionShare.tsx and failed with TS2307 on a fresh checkout -- passing locally only because next-env.d.ts happened to already exist on disk from earlier `next dev`/`next build` runs. Add an explicit declaration to the already-tracked types.d.ts, typed to match how these imports are actually used (as SVGR-transformed React components), removing the dependency on next-env.d.ts's generation order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@sparticuz/chromium ships a chrome-headless-shell binary, which only
supports the legacy "headless shell" mode. Passing `headless: true` makes
puppeteer-core add `--headless=new` (the full-Chrome headless mode
introduced in Chrome 112+), which this binary doesn't implement -- it
launches, navigates, and runs JS successfully (so the API still returns
200), but silently fails to composite/paint, producing a blank screenshot.
Confirmed via the deployed Vercel preview: the exact same remote /_render
page screenshots correctly when driven by local Chrome, isolating the bug
to the sparticuz launch options rather than the page/selection logic.
Fixed by following @sparticuz/chromium's documented launch pattern:
headless: 'shell' plus puppeteer.defaultArgs({ args: chromium.args,
headless: 'shell' }), which correctly emits the `--headless` flag chrome-
headless-shell expects instead of the conflicting `--headless=new`.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…it fixes rendering)
…ing as the true fix
Root-caused via a live debugging session against the actual Vercel preview deployment (the local devOptions path can't exercise this code, since @sparticuz/chromium ships a Linux-only binary): the API returned 200 and produced a valid PNG, but the screenshot was blank except for a stray border line -- no rendered text, and the highlight/scroll effect never visibly took effect, even though its `_scrolled` flag was set correctly. Isolated via a debugJson diagnostic branch and direct A/B testing on redeployed previews: neither `headless: 'shell'` nor removing `--single-process` (both tried and reverted) had any effect. The actual cause was call order -- chromium.font() (which registers a custom font) ran before chromium.executablePath() (which extracts the bundled Chromium binary and its font assets from the brotli archives). Calling font() first left the whole font subsystem uninitialized, so no text painted at all, and consequently nothing needed scrolling into view resolved correctly either. Swapping the order fixes both symptoms. Confirmed on a redeployed preview: page text renders, scrollY reflects the actual highlighted passage's position, and the screenshot shows the correct highlighted content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
samkim
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Modernizes this repo's toolchain a new component + e2e test harness that validates existing behavior and guards the upgrade, and updated consent handling.
Also includes two bug fixes found and fixed after the modernization work landed: a
pages/[range].tsx/api/preview/[ranges].tslocal-dev crash from an incomplete endpoint fallback, and a chrome-aws-lambda/puppeteer-core incompatibility (plus a related latent OG-image bug) that broke social-card image generation.Dependency upgrades
--webpack; migratedpublicRuntimeConfig→env;next lint→ flat ESLint config)posthog-js) pinned to 1.407.5, kept on the shared "Web+Cloud" projectchrome-aws-lambda(unmaintained, incompatible with modernpuppeteer-core) replaced with@sparticuz/chromium@140.0.0eslint-config-next15.5.3 → 16.2.12@headlessui/react,@next/mdx,@types/*,autoprefixer,cypress,postcss,react-click-away-listener,slugify@types/gtag.js(no longer needed once GTM/gtag.js was replaced)Testing
vitest.config.ts,vitest.setup.ts)AnnotationSetSelect,SelectionShare,PaperInfoMenu,ExternalScripts(SSR-verified consent gating)annotation.cy.ts), consent-gated GTM/PostHog behavior (consent.cy.ts)test.yaml) updated with the env vars these specs needConsent and analytics
az-consentcookie module fromdocs(util/consent/*)gtag.js/GTagcomponent with a consent-gatedExternalScriptscomponentBug fixes
pages/[range].tsx/pages/api/preview/[ranges].ts: fixed an endpoint fallback that resolved to the literal stringhttps://undefinedin local dev when neitherPREVIEW_ENDPOINTnorVERCEL_URLwas set, crashing withENOTFOUND undefined/api/preview/[ranges]):chrome-aws-lambdareached intopuppeteer-coreinternals removed in modern versions, crashing withMODULE_NOT_FOUND— replaced with@sparticuz/chromium. This unmasked a second, unrelated pre-existing bug inHighlightProvidedSelectionwhere the OG-image render path never set the scroll-complete flag the screenshot API waits on; fixed by checking the render-specific selection state first