This repository was archived by the owner on Oct 25, 2026. It is now read-only.
Repository navigation
fix(a11y): make the survey runtime speak the respondent's language, and gate what the docs claim - #62
Merged
Merged
Conversation
…glish to everyone - Every image a respondent meets rendered `alt="Image"`, which is worse than no alt: a screen reader announces a useless word instead of skipping a decorative image. The alt now comes from the stored file name, and falls back to empty — decorative — rather than to a placeholder. Two code sites covered all sixteen render paths. - Ten screen-reader-only strings were hardcoded English regardless of the survey's language, so an NPS card inside a `lang="ja-JP"` container announced eleven English labels. They route through the runtime's own translations now. - Ranking's keyboard reorder dropped focus at the ends of the list and announced nothing, so a keyboard user could not tell whether their keypress had done anything. Focus is restored to the pressed control and each move is announced through a live region. - The axe gate scanned eleven of eighteen element types and never the dropdown branch. It covers them by extending the seeded survey rather than adding a second spec. The eight new runtime strings exist in en-US only. `packages/surveys` generates its other locales through its own Lingo script, which needs an API key; until it runs i18next falls back to en-US, which is byte-for-byte today's behaviour.
…hat the sweep costs - A compile-time gate already stops an operator existing on one evaluator and not the other, but nothing checked that the two arms *behave* alike — which is the drift that actually shipped. A shared fixture table now drives both engines over every operator. The earlier attempt stalled because `apps/web` cannot reach the browser evaluator; that is solved by exporting a test entry point from the package rather than duplicating the fixtures. - The six-hourly reconciliation sweeps the whole graph while sharing the request path's one-second budget, so it competes with live permission checks. It is instrumented before being restructured, because the restructure cannot be sized without knowing what the sweep costs today. - The public link page had no cross-request cache; it has one now, keyed through `createCacheKey` with a TTL chosen from how quickly a published survey's rendering can legitimately change.
…nAPI bundle - The node `unit` project loaded a setup file heavier than the tests it serves; it has its own now, without disturbing the two projects that shared it. - Nothing persisted a Turbo cache and no workflow ran on push to main to produce one. This differs from the two caching experiments the repo already rejected, and the reason is recorded beside the change. - Nothing checked that an implemented `/api/v3` route appears in the OpenAPI bundle, so an endpoint could ship undocumented and skip the contract tests entirely. The new gate names the missing operation, in the shape `check-catalog.mjs` established.
…he page already had - Multi-language generation ran for minutes after the last streamed token with the user told nothing. Translation is chunked and partial results are reported as they arrive, which also keeps a single call from carrying every field of every language into an output cap. - The analysis filter lived only in component state, so a filtered view could not be shared, bookmarked, or survive a reload. It is in the URL now, and a bare URL still renders the default view. - Per-question denominators — impressions and skips — were already computed and handed to the client, and simply not rendered. - Contacts search keeps its application-side half; the trigram index it really needs is a migration, so the exact DDL is recorded rather than written.
🚨 PR Size WarningThis PR has approximately 4484 lines of changes (4206 additions, 278 deletions across 70 files). Large PRs (>800 lines) are significantly harder to review and increase the chance of merge conflicts. Consider splitting this into smaller, self-contained PRs. 💡 Suggestions:
📊 What was counted:
📚 Guidelines:
If this large PR is unavoidable (e.g., migration, dependency update, major refactor), please explain in the PR description why it couldn't be split. |
…essibility strings
The eleven screen-reader keys added with the accessibility work existed in en-US only, so `scan-translations` failed on a lockfile out of sync and twenty-four incomplete locale files. Generated through the package's own Lingo script — `pnpm --filter @forma/surveys i18n:generate` — rather than written by hand, so the lockfile checksums stay meaningful and Lingo can retranslate these strings later.
One of the keys was also reported unused while being used. `ranking-element.tsx` selected between `common.ranking_added` and `common.ranking_moved` through a variable, and the scanner only matches literal `t("...")` arguments, so a computed key is invisible to it. Both are spelled out at the call site now.
`translation-check.yml` fails a pull request whose locales are out of sync, which is right but leaves the contributor with nowhere to go: the other locales are machine-generated by Lingo.dev, and that needs a key most contributors do not have. This is the other half — the same `pnpm i18n` the documentation already tells you to run, on the branch, so adding a string stays a one-file change. It is what the repository's own instructions describe as happening automatically and, until now, nothing implemented. It runs only when an `en-US.json` actually changes, and each package is gated on its own catalogue, so a pull request touching neither pays nothing. Three things it deliberately does not do. It skips forks, where secrets are unavailable by design, rather than failing a job that cannot succeed. It skips when no key is configured, so a checkout without one behaves exactly as before. And it refuses to commit if `.env` — which it writes the key into, because both generate scripts read through `dotenv -e ../../.env` and `dev:setup` would otherwise shadow the secret with the placeholder from `.env.example` — ever stops being ignored.
… losing a response event - Every finished response loaded every enabled workflow's full published definition and discarded most of it in memory. The denormalised trigger column narrows the candidate set in the query instead, served by the widened index. `matchWorkflowsForResponse` stays the sole authority on whether a workflow fires, so a column that has drifted from its definition can only cost a wasted fetch — never a run that should not have happened. - The workflow list counted every run ever recorded, once per row. The "Runs" column is rendered, so the count could not simply be dropped; it is one grouped query now. - A Redis blip dropped the response pipeline event silently: no webhook, no follow-up, no billing event, and nothing recording it. The job id is now derived from the event, the response and its updated timestamp, which makes a retry idempotent without any schema at all, and the outbox carries what genuinely needs durability.
…gers, pipeline durability and retention Every statement is additive and non-blocking. `Integration` gains three delivery-health columns; the `NOT NULL DEFAULT 0` is a constant default, which PostgreSQL stores as a missing value rather than rewriting the heap on every version this project supports — the lowest is 15, per `.squawk.toml`. `Workflow` gains a nullable `triggerSurveyId` with a partial, re-runnable backfill, and its index widens to a superset of the old one, so dropping the old loses no access path. `ResponsePipelineOutbox` is a new table. `Organization` gains two nullable retention windows. A null `triggerSurveyId` means "matches every survey", never "matches none". That is what makes the partial backfill safe: a row it did not reach — draft, disabled, a different trigger type, a malformed definition — still reaches the in-memory matcher exactly as before. Reading null as an empty match would silently stop those workflows firing. A null retention window likewise means keep forever: the sweep only visits organizations that have set one. Indexes are created without CONCURRENTLY for the reason the repository already records in an earlier migration: `prisma migrate deploy` sends the file as one multi-statement script, PostgreSQL runs it in an implicit transaction, and a concurrent build aborts with 25001. One unrelated line moved: `zod/organizations.ts` carried a type assertion on `isAISmartToolsEnabled` that `no-unnecessary-type-assertion` rejects. It is pre-existing, and only surfaces because the pre-commit hook runs type-aware rules over files a commit touches.
…nd classify the new table - Notion, Airtable, Google Sheets and Slack deliveries record their outcome against the new columns, and integrations get the retry the webhook path already had, reusing that mechanism rather than a second one. This only became honest work once Notion and Airtable started reading their HTTP responses: a health surface built before that would have reported a permanently broken integration as healthy. - Display and WorkflowRun grew without bound; only the AuthZed outbox was pruned. A batched sweep prunes both now, following that pruner's shape. The window is per-organization configuration rather than a constant, because how long a customer's data is kept is not a decision a job function gets to make, and null means keep forever — an organization that has configured nothing is never visited. - `ResponsePipelineOutbox` is classified in the authorization resource inventory. That guard exists so every new model has someone state what it is in the authorization model; this one is internal delivery infrastructure nobody is granted access to, like the AuthZed outbox beside it. Twelve fixtures across six files gained the new columns, which is the type system working rather than churn: `consecutiveFailures` is NOT NULL, so every stand-in for a persisted row has to carry it. In `organization/service.test.ts` the fixture also doubled as the expectation, and that no longer holds — `select` deliberately does not fetch the retention windows, so the mapper cannot return them. The expectation strips them rather than the query growing a column nothing reads.
feat(db): four additive migrations, and the delivery health, durability and retention they unlock
Both sides gave sendToPipeline a deterministic BullMQ job id. Keep this branch's version (outbox fallback, undefined id when the response version is unusable) but take main's `-` separator: BullMQ 5.61 throws `Custom Id cannot contain :` for any custom id with a colon that is not exactly three segments, so the four-segment `response-pipeline:event:id:ms` id made every enqueue fail. Main's idempotency tests are kept alongside the exact-id assertions, and the outbox drain fixture now uses the real id format. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What & why
Was: Every image a respondent met announced the word "Image", ten screen-reader labels were English whatever the survey's language, and ranking's keyboard reorder moved the row but told the user nothing. The two survey-logic evaluators had no behavioural parity check, and an
/api/v3route could ship undocumented and skip the contract tests.Now: The respondent surface speaks the survey's language and announces what it did; the twin evaluators are driven over every operator by one shared table; and an undocumented v3 operation fails the build.
Where to look
survey-ui/src/lib/storage.ts— alt text, and why empty beats a placeholdersurvey-ui/src/lib/ranking.ts— focus restoration and the live regionscripts/— the OpenAPI route gateBreaking changes
None. The accessibility work changes what assistive technology announces, not any API, and the new survey-ui props default to today's English literals so other consumers are unaffected. The OpenAPI gate fails only on a route that is already undocumented.
Migrations & env
How this was tested
Rerun:
pnpm --filter @forma/survey-ui test src/lib/storage.test.ts src/lib/ranking.test.ts && pnpm --filter @forma/surveys test src/lib/live-region.test.ts src/components/general/survey.test.tsx && pnpm --filter @forma/web test --project=unit lib/authzed/scheduled-reconciliation.test lib/surveyLogic && node scripts/check-openapi-routes.mjsCoverage
"Image": 2 → 0, covering 16 render pathscreateCacheKey; two workspaces never shareThree declines, one of which corrects the analysis
The templates catalogue (r111) was declined because the brief is wrong. It claims the 56 non-catalogue keys are reachable only from the editor and the six template-list routes. They are not:
modules/survey/lib/elements.tsxholds 44 of them — the element-type labels — and is imported by twelve non-test files, including three analysis surfaces and the Notion mapping row. Splitting as specified would have broken those. The item needs rescoping, not implementing.Dropping
@forma/surveys#buildfrom the web test graph (r145) needs abuild:typesscript in that package, outside the cluster's block. The agent checked the two turbo-only alternatives and both are wrong: deleting the edge outright breaks the unit job.Integration delivery health (r136) needs a schema change this wave excluded. Exact DDL, verified against the init migration (the model has no
@@map):Its prerequisite is already merged — Notion and Airtable now report their failures — so a health surface built on this would no longer be reading a lie.
Also fixed in passing: the new reconciliation spec cast a three-field object to a result type carrying twelve counters and ten lists. The
aswas the only reason it compiled. It now builds the full shape from a zeroed base, so a field added to the result type surfaces as a type error here rather than asundefinedreaching the code under test.Open gaps
packages/surveysgenerates its other locales through its own Lingo script, which needs an API key; until it runs, i18next falls back to en-US — byte-for-byte today's behaviour, so nothing regresses.e2e.ymlcannot run on this fork. The spec change is a claim about coverage that CI here cannot verify.Risks
IMG_4821.png). That is still better than "Image", but a realimageAltTextfield is the proper fix and is not in this PR.Note
AI model used —
claude-opus-5[1m](Claude Code), reasoning effortunknown.