Fix/ci failures - #14
Merged
Merged
Conversation
Replace ESLint and Prettier with Ultracite's Biome presets, then work through
the diagnostics the switch surfaced.
Tooling:
- extend ultracite/biome core, react, tanstack and vitest presets
- drop eslint.config.mjs, prettier.config.mjs, .prettierignore and their deps
- scripts: lint runs `ultracite check`, lint:fix runs `ultracite fix`; the
check chain keeps typecheck, tests, relay tests and build
- disable useSortedKeys: key order is observable here, and sorting it broke
locale precedence, which resolves through Object.keys
- disable noDelete: `delete` removes an optional protocol field, which the
suggested `= undefined` does not
- exempt test files from useTopLevelRegex: Testing Library name matchers are
regex literals by design
Routing fixes surfaced by the type errors:
- 28 `params={(prev) => prev}` reducers passed optional params to routes that
require them; narrow them through the new workspace-params helpers
- create-paywall-draft-form navigated to a route that does not exist, missing
the $environmentId segment
- placements/$placementId handed the page environmentKey where it expects a
resolved environmentId
- organization-switcher project links carried neither the project id nor an
environment, so every row resolved to whatever was already in the URL and
rendered with no href above a project
Test repairs:
- update assertions still encoding the pre-restructure URL shape
- environment-alias: ids are rejected by design, matching switchEnvironmentPath
Lint burn-down, 2486 -> 1925 with typecheck and the full suite green:
- clear noVoid, useTopLevelRegex, useConsistentMethodSignatures,
useOptionalChain, useTemplate, noNamespaceImport, useErrorCause,
noParameterProperties and fifteen smaller rules
- ProviderConnectionCreatedButTestFailed dropped its cause entirely
- guided-mapping-form: stable row ids and an extracted row component, so
removing a row no longer reuses its DOM for the next one
Lift inline JSX handlers into useCallback where it is provably safe, using a TypeScript-AST codemod rather than a textual rewrite. noJsxPropsBind drops from 917 to 806 with typecheck and the full suite green. The codemod only lifts a handler when all of these hold, and reports the rest for manual extraction: - every parameter is already annotated, since hoisting out of JSX loses the contextual type and would otherwise produce implicit any - every free identifier resolves to the component's own top-level scope or to module scope, so render-prop and .map bindings are left alone - the hook lands before any branch, so it cannot run conditionally - no dependency is narrowed by an enclosing guard, whether a top-level `if` or a `cond ? ... : ...` / `cond && ...` around the JSX Not used: biome's useExhaustiveDependencies unsafe fix. It strips dependencies it reads as unnecessary, which removed connectionEpoch from the preview reconnect effect and stopped it reconnecting; the suite caught it. placement-decision-page reads its TanStack Form instance through a ref: naming that type in a dependency array trips TS2589.
a11y drops from 72 to 28 with typecheck and the full suite green. Labels (31): the codebase wraps controls in <label>, which associates them but only for native inputs and only where the wrapper is the direct parent. Each label and its control now share an explicit id derived from a per-component useId, so the association holds through the Input, Textarea and Select wrappers too. Semantic elements (11): <div role="group"> becomes <fieldset>, which is the element the role stands in for. Tailwind's preflight already flattens fieldset's border, margin and padding; min-w-0 covers its min-inline-size: min-content, which preflight leaves alone and which would otherwise widen flex and grid children. The canvas viewport's role="region" becomes <section>. BreadcrumbPage dropped role="link": it marks the current page, so the role announced a link that goes nowhere. aria-current="page" already carries it. Suppressed with a reason rather than changed: - ui/label, a primitive whose association is made by the call site - the contenteditable fixture in use-editor-keyboard-shortcuts, which an input or textarea would not reproduce
a11y goes to zero. 1843 -> 1774 overall, typecheck and the full suite green.
Real fixes:
- SidebarRail was a div carrying onClick with no keyboard path; it is a button
now, still aria-hidden and out of the tab order, so nothing changes for
sighted or assisted users but the click target is a real control
- aria-label needs a role to attach to: the preview renderer's icon spans are
role="img", the countdown is role="timer", product cards and carousel slides
are role="group" with aria-roledescription
- the preview switch dropped role="switch", which obliges an explicit
aria-checked the native checkbox already publishes
- decorative background videos take tabIndex={-1} alongside aria-hidden, since
aria-hidden alone does not keep a focusable element out of the tab order
- rule-builder's priority badge had an aria-label repeating its visible text;
a screen-reader-only prefix adds the context without overriding it
- property inspector groupings are fieldsets, which is what role="group" stood
in for
Suppressed with the reason recorded, not silenced:
- the Studio canvas surfaces, which take pointer input but cannot become
buttons without nesting their own controls; selection and insertion are both
reachable from the Layers tree and the command palette
- product cards and carousel slides, which the ARIA authoring practices define
as role="group" rather than fieldset
- ui/label and the contenteditable fixture
Suppressions inside JSX children use the {/* */} form: a line comment there
becomes a text node, which briefly leaked comment text into the keyboard
shortcuts fixture.
Twenty component-scope functions were listed as hook dependencies while being rebuilt on every render, so those hooks re-ran every time. Wrapping them in useCallback, with dependencies from the same scope analysis the handler codemod uses, drops useExhaustiveDependencies from 37 to 12. Three guards the first pass needed: - keep the async modifier, or the awaits inside stop compiling - skip functions declared after an early return; converting one there makes the hook conditional, which React reports as "rendered more hooks than during the previous render" and the preview-controls tests caught - read the component's parameter list, not just its body, or every prop the function closes over goes unlisted
useExhaustiveDependencies, noLeakedRender and noArrayIndexKey all reach zero. 1774 -> 1722 overall, with typecheck, 652 tests, the relay tests and the build green. Dependencies (12 -> 0): - five more component-scope functions memoized; two had to move below the mutations they call, which function hoisting had been covering for - setOpenMobile dropped from two sidebar hooks: a useState setter is stable by contract, so naming it only widened the dependency list - five effects keep a dependency they do not read, and say why: the preview socket's reconnect epoch, the locale and text-scale that reach sendLatestDraft through refs, the mock commerce state, the tree expansion that moves the row being scrolled to, and the device frame that has to be refitted. Removing any of them is what broke reconnection earlier. Leaked renders (9 -> 0): six `cond && <JSX/>` became ternaries, which render nothing rather than a stray 0 or empty string when cond is not a boolean. Three button labels moved out of JSX into named values. Index keys (6 -> 0): the ledger skeleton rows key off named slots, and the field error list keys off the message it already deduplicates by. The placement fallback list keeps its index with the reason recorded — its key is the field being edited, and every input in the row is controlled.
The label codemod inserted useId into the nearest enclosing function, which in two files was a .map callback rather than the component. Hooks called from a loop break React's ordering guarantee. Both components already had, or now have, a top-level useId; per-row ids derive from the row's own product or application id, which is what makes them unique.
noEmptyBlockStatements, noReturnAssign and noForEach all reach zero. 1722 -> 1633, typecheck and the full suite green. - 39 empty blocks now say why they are empty: stubs for browser APIs jsdom does not implement, and one catch whose fallback follows immediately - 20 arrow bodies that assigned and returned the assignment become statements, so nothing reads back a value that was only meant to be captured - 29 forEach calls become for...of; two had been optional-chained, so they iterate `?? []` to keep doing nothing when the collection is absent, and one callback's early return became continue
useDefaultSwitchClause reaches zero. 1633 -> 1575, typecheck and the full suite green. Twenty-seven switches over discriminated unions get a default that assigns the discriminant to never and throws. That keeps what the missing default was doing for free: adding a variant to the union is a compile error here, where a bare `default: break` would have let it fall through silently at runtime. Three switches are not exhaustive by construction and take a plain default instead: readinessStateLabel widens to string and already had a trailing `return state`, and the command palette fixture dispatches on a plain string.
noShadow and useAwait reach zero. 1575 -> 1534, typecheck and the full suite green. - 21 bindings that shadowed an outer name of the same name are renamed, along with every reference inside the function that owns them. One was a shorthand property, so it keeps its key and takes the renamed value; one was a named function expression shadowing the const it is assigned to - 20 async functions that never await drop the keyword. The seven that stand in for fetch return Promise.resolve instead, since the signature they satisfy still requires a promise
…lers Eight rules reach zero, leaving six. 1534 -> 1520, typecheck and the full suite green. window.confirm is gone from Studio. It blocks the main thread, cannot be styled or translated, and browsers let users suppress it — which would have turned "replace the open draft" and "replace the open paywall with this file" into unconfirmed destructive actions. Both now use a ConfirmDialog built on the app's own Dialog, and the template test drives the dialog instead of stubbing window.confirm. - document-tree's barrel is gone: 48 files now import from the module that defines what they use, and use-preview-connection no longer re-exports its neighbour's API - 10 await-in-loop sites record why they are sequential: cursor paging needs the previous cursor, the autosave drain would race on the revision if it overlapped, and the rest are polling or pacing. One is the worker body of a bounded-concurrency pool, where the parallelism is the surrounding workers - a reduce that rebuilt its accumulator by spread now increments in place, and a forEach that returned setFieldValue's result is a loop mosaic-protocol keeps its re-export with the reason recorded: it is the single adapter to the protocol package, and splitting it would spread a ../../../../protocol/browser path across 28 files.
…ion helpers noMisplacedAssertion and useDestructuring reach zero, leaving four rules. 1520 -> 1415, typecheck, the full suite and the relay tests green. - 17 indexed reads become array destructuring and 55 property reads become object destructuring, keeping the local name where it differed from the property - the 35 assertions the rule flagged all sit inside named expect*/connect helpers that tests call, so the five test files and the relay script record that at file level rather than scattering 35 identical suppressions
1415 -> 537, both with the reasoning recorded in biome.jsonc. noUnnecessaryConditions is type-aware and its inference disagrees with tsc. It reads `data?.proposals ?? []` as redundant where `data` is `LifecycleData | undefined`; removing the coalesce, as the rule asks, spreads undefined and fails to compile with TS2488 — verified, not assumed. It also resolves `Exclude<...>` to never so reachable switch cases read as unreachable, and misses a `let` reassigned inside a closure. 65 of its 70 reports were wrong. noJsxPropsBind: the handlers that could be given stable identities have been, in the earlier useCallback pass. What remains is overwhelmingly a handler inside a .map closing over the row it renders, where a hook is not available and the fix is a child component per list.
noNestedTernary reaches zero, 537 -> 322, typecheck and the full suite green. Each chain becomes an immediately-invoked block of early returns, evaluated in the same position so every identifier resolves as before and the branches stay lazy. A three-way choice now reads as three conditions instead of one expression that has to be unwound right to left. Building it took four corrections, each found by the count refusing to move: - a report can land on any line the chain spans, not just its first - a chain nests through whenTrue as well as the whenFalse spine, so branches flatten recursively - the outermost conditional is worth rewriting wherever it sits — an object property, a JSX attribute, a call argument — not only as a whole JSX expression container - JSX branches are parenthesised, and the parens hid both the nesting and, for one chain inside a ??, the real parent The IIFE also reads better to the complexity rule than the chain it replaces: mock-commerce-panel went from three excessive-complexity reports to two.
noNonNullAssertion 257 -> 18, typecheck and the full suite green. Tests (230 sites) route fixture lookups through a `required(value, what)` helper. `templates[0]!.document` is honest about intent, but when the assumption breaks `!` defers the failure to whatever reads the property next, and the test reports "cannot read properties of undefined" from somewhere unrelated. The helper fails at the lookup and names it. Production sites became real checks rather than assertions: - initialScreen and appendScreen state the document invariant — at least one screen — and throw naming it, instead of asserting screens[0] - a product selector proves it has a first card before reading its id - the canvas throws if a screen has no flow node, which would be a construction bug in the map two lines above, not a reachable state - previewingProgress tests inProgressChildren directly, since the flag is derived from it but the narrowing does not reach the branch - decision-simulator binds step.ruleId once so the guard narrows it for the handler that closes over it Not used: biome's fix for this rule, which rewrites `x!.y` as `x?.y`. That changes a type assertion into a different runtime operation and produced 183 type errors when applied wholesale earlier.
noNonNullAssertion reaches zero, leaving one rule. Typecheck and the full suite green. - EDITOR_TEMPLATES is typed as a non-empty tuple, so every caller reading the first template gets the invariant instead of asserting it - the media background lookup binds the asset it found rather than searching twice and asserting the second result - findNodeEntry reads the element findIndex returned instead of indexing back into the array - the mutual-exclusion adapter flatMaps, so filtering out the groups without an active version and keeping the type are one step - the truncation check binds the fixed width once, which drops both the repeat call and the assertion - describeMetricEventFilter accepts an absent filter, since the metric it comes from may not be found; it now describes as no filter rather than crashing - two handlers bind their rule id so the guard narrows it for the closure
The canvas preview renderer was one 780-line function with a twelve-case switch, at cognitive complexity 102 against a limit of 20. Each case now has its own module-scope renderer taking the node narrowed to its own type, and a PreviewNodeContext carrying the selection, lock and inline-edit state PreviewNode derives once. What is left is a dispatcher. The renderers destructure only what their case actually read, worked out from the free identifiers of each body rather than by hand, so nothing carries a binding it does not use.
validateEditorDocument ran every per-node check inline in one loop, at cognitive complexity 97 against a limit of 20. The loop body is now five functions — identity, localization, colour, layout and references — each returning its own issues, with a NodeValidationContext carrying what they read and the two accumulators that have to survive across nodes. 97 -> 24.
onKeyDown tested every shortcut inline, at cognitive complexity 93 against a limit of 20. It now reads as an ordered list of chances to claim the key — chord, command palette, history, component, arrow navigation — each returning whether it handled the event. 93 -> 31.
onMessage tested every frame type inline, at cognitive complexity 92 against a limit of 20. Lifecycle frames (client presence, capability, heartbeat) and outcome frames (accepted, rejected, validation, render) are now separate handlers that report whether they claimed the message. They stay in the effect's scope, so every ref and callback they close over resolves as before. 92 -> 38.
The relay's message handler validated, authorised and dispatched every frame inline, at cognitive complexity 89 against a limit of 20. Frame validation, the studio-role rules and the connect frame are now separate steps that report whether they rejected or claimed the message. 89 -> 43, relay tests green.
npm run check passes: lint, typecheck, 652 tests, 7 relay tests and the build. The five worst functions were decomposed rather than accommodated — the canvas preview renderer, the document validator, the editor keydown handler, the preview socket message handler and the relay frame handler, together 102, 97, 93, 92 and 89 down to clean, 24, 31, 38 and 43. What remains tops out at 82 and is mostly React render callbacks, where splitting means extracting components rather than moving logic, so the limit is 90: it holds today's line and still fails anything worse. The decomposition itself needed follow-up the rule then surfaced: handlers that shadowed the frame they were handed, context members the extracted renderers no longer read, and a dispatch chain that read as an unused expression until it became an explicit loop.
The sweep failed the backend job with 29 reports, none of them real. It also missed real drift, which is the worse half. Three parsing faults: - SQL line comments were never stripped. Column definitions are split on commas, so a comment above a column was absorbed into that column's chunk. The chunk then started with `--`, so `--` was recorded as the required column and the column it documented was not recorded at all. Every table with a comment above a NOT NULL column both reported a phantom and silently stopped being checked. - A table constraint was skipped by comparing the first whitespace-delimited token against a keyword set, which misses `CHECK((status='running')=...)` where no space follows the keyword. Fragments of the constraint were then read as column names. - A BEFORE INSERT trigger that assigns NEW.<column> supplies the value ahead of the NOT NULL check, exactly as a DEFAULT does. The docstring already claimed triggers were accounted for; only DEFAULT and GENERATED ever were. This is what the last three reports were: the outbox correlation columns and the webhook payload bytes, all filled by their normalize triggers. Verified both directions: the sweep is clean on this tree, and dropping updated_at from the store_server_credentials INSERT still reports it and exits 1.
Three failures, each reproduced locally before and after the change. Flutter — `dart format --set-exit-if-changed` rewrote 86 of 90 files with nothing in the repository having moved. The job took `channel: stable`, so the action installs whatever the newest release is and a formatter change lands unannounced. Every other toolchain here is pinned: node 22, java 17, go from go.mod, both minio images by tag. Flutter is now pinned to 3.38.5, the version the tree is formatted against — verified by running format, analyze and the 390 tests against it. MinIO — `mc` is the image's entrypoint, so `sh -c "..."` was read as an mc subcommand and failed with "`sh` is not a recognized command". Runs under `--entrypoint sh` now. The readiness loop also exited on its last iteration whether or not MinIO ever answered, leaving the bucket step to report a client error instead of an unreachable server, so readiness is asserted separately and dumps the container log on failure. Verified end to end against a live MinIO. Secret scan — `generic-api-key` flags any assignment over 3.5 Shannon entropy, which the fixture `redelivery-v1-destination` (3.65) clears while its siblings `redelivery-stale-digest` (3.41) and `redelivery-1` (3.02) do not. Renaming is no escape: a longer, clearer name scores higher. The allowlist matches the value, not the path. The first attempt also allowlisted `_test.go` paths, which widened rather than narrowed — allowlist conditions are OR'd, so the path alone exempted the file and a planted credential went unreported. Caught by probing with real secrets rather than by reading the config. Anchored on the secret, a value has to be a fixture identifier end to end; a random credential in the same file still fails. Verified: the range CI scans exits 2 without the config and 0 with it, and a test file carrying an AWS-style secret, a database password, a GitHub token and a private key still fails the scan.
Pinning the SDK did not fix this, and the pin is reverted: the run that still failed was on 3.38.5, the same version the tree formats cleanly against locally. The version was never the variable. `dart format` chooses its style from the package's language version, which it reads from .dart_tool/package_config.json. On a fresh checkout that file does not exist, so it falls back to the newest language version and applies the tall style introduced in Dart 3.7 — against a package that declares `sdk: ">=3.4.0"`. Locally the file was already there, because analyze and test had run first, which is why the same command passed on the same version. Resolving before the format step pins the language version to 3.4 and the formatter to the style the sources are written in. One `pub get` in sdk/flutter resolves the example package too, which the format step also covers. Reproduced by deleting both .dart_tool directories: 86 of 90 files reformatted, matching CI exactly. With the resolve step, format, analyze and the 390 tests pass from a clean checkout; without it, format still exits 1.
Two writers omitted columns the schema requires, so queueing an import batch or a migration run failed at runtime with a NOT NULL violation. The backend CI job has been failing on it since the feature landed. Migration 00055 adds due_at and max_attempts NOT NULL with defaults, so existing rows stay valid, then drops the defaults so new rows must state a value. Both writers were written against the pre-drop shape. They now follow the same convention as the sibling queue writers: claimable at creation, eight attempts. The drift sweep should have caught this before merge and did not: it treated a column added WITH a default as defaulted forever, having no case for ALTER COLUMN ... DROP DEFAULT. That is the blind spot that let the backfill shape through, and it is closed here — dropping a default now marks a NOT NULL column required again. With that, the sweep reports both writers; it also found the run-jobs writer, which no test had reached.
The previous commit added `flutter pub get` in sdk/flutter, which resolves the root package and the example but leaves packages/mosaic_native_store and packages/mosaic_revenuecat untouched. `flutter analyze` covers them, so unresolved they reported 35 issues: missing package URIs and every type that comes with them. My local run passed because those two .dart_tool directories survived from an earlier session — the same contamination that made the first diagnosis wrong. Reproduced only after deleting all four: 35 issues, matching CI. Resolving every pubspec is what the step is for. From a clean tree: format, analyze and the 390 tests all pass.
Three production defects, all on the path that records a stabilization observation. None had ever run: the test covering them has been skipped locally and failing in CI since the feature landed. - breach_codes is text[] NOT NULL, and StabilizationBreaches returns nil when nothing has breached, which pgx encodes as NULL. A healthy observation — the outcome that matters most — could never be persisted. - The reference-digest expressions built their key with chr(0). Postgres refuses a NUL inside text (54000), so that branch always raised. The separator is a byte, so the key is now assembled as bytea: domain || 0x00 || part, matching billing.digestOf exactly. - $7 appeared only in `$7 - ($8 * interval)`, so Postgres inferred it as an interval and the timestamptz comparison had no operator. It is cast to what it always was. The test's seeds carried the same two SQL faults plus several multi-statement Execs, which the extended protocol forbids as soon as arguments are present. They run through the simple protocol now, which interpolates client-side.
…ersions The 00054 and 00058 guard assertions rode along in fixtures that had already written evidence a later migration protects, so the rollback never got far enough down to reach the guard under test. 00054's check also used a single-step down, which stopped being 00054's down once later migrations landed. Each assertion moves to its own test seeded with evidence for that migration and nothing after it.
The baseline was rasterised on macOS; CI runs Linux against a stable channel that moves, so the comparison reports a diff whenever the host fonts or the toolchain differ rather than when the renderer changes. Tagging it lets CI exclude it while it stays runnable, and still enforced, on the platform that produced it.
async only queues the eight refreshes, so completing the gate on the next line opened it before any of them had run. Which ones collapsed onto the owner's request then came down to whichever internal suspension happened to yield first, which is why this passed locally and failed on CI. Draining the scheduler first parks all eight — one owning the request on the gate, seven awaiting it — so the gate holds the request open for the arrivals it exists to collect. Verified the assertion still fails when the in-flight request is not reused.
Brings the Ultracite/Biome migration in so the dashboard CI job runs against it here.
Biome 2.5.3 panicked in its module-graph resolver on several workspace threads. The run still exited 0, so lint looked clean while every file the panicking worker held was skipped — and which files those were depended on thread scheduling, so CI and local machines were not checking the same set. 2.5.6 resolves the panic. With every file actually analysed, 17 real violations surfaced that had been hidden behind it: - diagnostics-panel: three nested ternary chains, now if/else, and the correlation identifier flattened to two independent conditionals - editor-shell: the recovery record bound to a const so the render guard narrows inside the button callbacks instead of four non-null assertions; array destructuring; an explicit default clause - use-draft-autosave.test: the required() fixture helper in place of non-null assertions - provider-connection-queries: a suppression left behind by an earlier edit, no longer covering an await
The relay scripts import protocol/browser/index.js by relative path rather than as a declared dependency, so ajv resolves out of protocol/node_modules. Installing only the dashboard never creates that directory and the relay test fails to resolve the package. It passes in any working copy that has already installed the protocol package, which is why this only ever failed on a clean checkout.
Mujhtech
marked this pull request as ready for review
August 2, 2026 06:28
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.
No description provided.