Repository navigation
feat: a light theme through a shared token layer, and four silent query-execution defects - #384
Merged
Merged
Conversation
…bort test to its own signal
… feat/light-theme-tokens # Conflicts: # src/components/DatabaseDocs.tsx # src/components/agent/AgentRail.tsx # src/components/rich-text.tsx
Member
Review notesOverall this is careful, well-argued work. The token layer is the right design, the dark values reproduce the old literals so tokenising is a no-op in dark, Two blockers below. Both fail silently — no CI check covers either path. Blockers1. The published package will not contain
2. Running a query in a second tab cancels the first tab's query and leaves it spinning forever —
Should be fixed before merge3. The embedded surface renders a mixed theme in a light host —
4. ERD export ground is still hardcoded dark —
5. The literal sweep needs a second pass after the rebase
6. No docs changed
Minor
|
…uery Scoping the in-flight run to its tab gave `cancelQuery` a first parameter, `tabId?: string`. Both call sites in the standalone studio still passed the function itself to a button's `onClick`, so React filled that slot with its MouseEvent. An event object is truthy, `runsRef` holds no entry under it, and the handler returned at the first guard: the fetch was never aborted, the background EXPLAIN kept running, and `/api/db/cancel` was never sent. The button looked live and did nothing. Measured against a production build, cancelling a `SELECT pg_sleep(20)` two seconds in: before, both requests ran the full 20027 ms and returned 200 with no abort and no cancel request; after, both report AbortError and `/api/db/cancel` answers 200. TypeScript could not see it — an optional parameter still satisfies the `() => void` the prop declares — and neither could the tests, which called the captured prop with no arguments and so exercised the path the browser never takes. The new test supplies the argument React supplies, and asserts on arity rather than on delegation. The embedded surface is unaffected: `StudioWorkspace` cancels through `useQueryAdapter`, whose `cancelQuery` takes no parameters.
This was referenced Aug 16, 2026
Member
|
Reviewed this against a production build, driven in Chrome, with two local Postgres tabs and computed styles measured rather than eyeballed. The theme work holds up well; I pushed one fix directly to the branch so the feature does not have to wait another round. What I pushed (8291724)
Measured on a
Nothing here is a criticism of the design — it is the ordinary cost of changing a callback's arity late in a branch. Worth noting for next time: TypeScript cannot catch it, because an optional parameter still satisfies the The embedded surface was never affected — Verified working
Follow-ups, filed separately, not blocking
For calibration on that second one: I ran the same contrast sweep in both themes and light came out with 33 text nodes below AA against dark's 57. Low contrast is this app's existing character, not something the light theme introduced — and One small thing, purely cosmetic: the description still describes Nice work on this — the comments carry the reasoning rather than restating the code, and the claims in the description were checkable and checked out. |
cevheri
added a commit
that referenced
this pull request
Aug 16, 2026
Both were found while reviewing the light-theme PR and verified against the merged code, and neither belongs in the tracker: nothing is broken for a user today and no outside reporter is waiting on either. U1 is a residual race the PR's own guard leaves open — an EXPLAIN that resolves after both its own run and a later one finds no map entry, reads as not superseded, and writes a stale plan over the newer results. U2 records that the ESLint rule which catches the defect fixed in #384's last commit is already configured, just not aimed at src/components, together with the measurement that stops it being a one-line change: extending the scope to Studio.tsx flags both real call sites and 21 benign ones. The third finding from that review, a ninth chart series repainting as the first, went to the tracker instead as #403 — a reader can hit it, and choosing between capping, folding and secondary encoding is a product decision.
cevheri
added a commit
that referenced
this pull request
Aug 16, 2026
One conflicting file, `src/components/agent/AgentRail.tsx`. Both sides touched it for unrelated reasons: this branch moved workflow selection off a pre-typing tab row and onto a server-side reading of the objective, while #384 rewrote the rail's hardcoded dark Tailwind colours onto the shared semantic token layer. Four hunks, each keeping both intents: - the mode buttons: this branch's `disabled` freeze while a start is held, over main's `text-fg-muted hover:bg-fill` - the workflow tab row: stays deleted, which is the point of the branch; the tokens main applied to the objective block below it are kept - the pre-start auto-execute checkbox: stays deleted, and consent now exists only in the consent step; the Advanced disclosure takes its place - the budget block: this branch's run-gated gauges and the withheld ceilings under Automatic, over main's tokens Tokenising the merge is the part a textual resolution would have missed. Every control this branch added was written against the literals #384 removed, so a resolution that only reconciled the overlapping hunks would compile, pass and ship a light theme in which the Advanced disclosure, the consent step and the provenance banner stayed dark while the rail around them adapted. Each of those classes is now the token main chose for the same element elsewhere in the file — the consent checkbox takes `border-edge bg-panel` from the checkbox it replaces, its label and terms the `text-fg-secondary` / `text-fg-muted` of the ones it inherits, the workflow buttons the `text-fg-muted hover:bg-fill` of the mode buttons beside them, and Cancel the `text-fg-tertiary hover:bg-fill` of the rail's other quiet buttons. The rail now carries no `zinc-`, `white/` or `black/` utility at all, which is what main left it as. The status accents are left as literals, because main left them so on purpose: amber for the SQLite warning and the unresolvable connection, rose for the failure reason, blue for the selected state, the progress bar and the consent card's own tint.
This was referenced Aug 17, 2026
cevheri
added a commit
that referenced
this pull request
Aug 17, 2026
…n schema (#418) Plan mode opened on every engine and was grounded on two. On the other nine it could name nothing, so it correctly refused - while the sidebar, on the same screen and the same connection, listed the tables. That was never a limit of the engines. It was a limit of where grounding was read FROM: the sidebar calls the provider's getSchema(), which every provider implements, and the agent composed a catalog STATEMENT whose CATALOG_PLANS carried postgres and sqlite and nothing else. This adds the provider as a second grounding source under its own audited operation, and leaves the composed path exactly where it was. db.schema.read is a sixth operation descriptor - R0/metadata-read, no required capabilities, an empty strict input because getSchema() takes no selector, and resourceCost heavy because it is N+1 round trips and, on MongoDB and Couchbase, samples documents to infer field names. It is modelled on db.operations.read, which is the precedent that makes it legitimate rather than a side door: context-snapshot.ts rejects asking the provider directly because that bypasses M1, and the objection is correct about a RAW call and not about one made under an operation descriptor. The read goes through runAuditedAgentCall, so it meets the same mode check, repair ledger, deadline, budget, policy and audit stream as every other call. It acquires agent-operations rather than agent-read-only, and that is the line the whole change rests on: the read-only profile requires queryReadOnly, which none of the nine engines has, so the factory would refuse it before the provider was ever asked. PostgreSQL and SQLite keep the composed path deliberately - its reads are audited statement by statement, it carries foreign keys, and SQLite's inventory is parsed out of stored DDL the provider does not expose the same way. Both modes take the shared capture. A mode gate would not have held: the process snapshot hold is keyed by connection and is filled and read by both, so gating would only have made agent grounding depend on whether a plan run went first - the #384 failure this codebase has already fixed once. Agent mode gains no reach: a schema workflow there still fails at its first read, pinned by a test that asserts the capture succeeded first. The three questions the issue left open are answered. validatePlanStatement now takes the engine's language and declines to judge rather than judging wrongly - every correct MongoDB or Redis draft was already classified NON_READ_STATEMENT, invisible only because no schema workflow was grounded there. The ER block stops hedging over foreign keys on six engines that cannot declare one, driven by a capability rather than an engine check. And Redis is grounded, with the answer that the inventory has to say what it is an inventory OF. Three defects came from driving eleven engines in a browser, not from a gate. The inventory called everything a table. A grounded Redis plan was handed a block headed "17 table(s)" listing user:*, order:* and fifteen more, and then drafted ZCARD user:* - treating a synthesized prefix group as an addressable key. The product already had the vocabulary and the agent ignored it: ProviderLabels.entityName says Table, Collection, Datasource, Key Pattern, Key Prefix. It now reaches the packing, the relations block, every plan rule, the timeline and the rail, and nothing changes for the six SQL engines. A second capability carries the part a model cannot know because it is a fact about this codebase: those rows are groupings this server derived from a bounded scan, they are not keys, and the list is what one reading reached. A grounded plan run was told, in its own rules, that its inventory came through the same read-only catalog path agent mode uses. The preface was made a 2x2 over readBy x readVia and this second provenance sentence was left one-dimensional, so on nine engines the model got that claim and its contradiction in one conversation - on engines where agent mode cannot take a read-only path at all. An environment failure lost a plan run on nine engines that previously could not lose one. A capture that threw before the reading left ended the run internal, or showed "the agent cannot run on this database engine" where plan mode demonstrably works. It now converts to an unavailable capture and the run continues and says why. MongoDB's getSchema() called estimatedDocumentCount() and indexes() unguarded and MongoDB refuses both on a view, so one view aborted the whole schema read for the sidebar as well as the agent. Fixed by reading the type listCollections() already reports rather than hiding views, and verified live: six objects listed, the view with its fields inferred, no indexes, and no row count - absent rather than zero, because a view is not empty, its size is unknown. Driven live on 2026-08-17 against real local servers, one engine at a time. Ten of eleven grounded and drafting in their own language: PostgreSQL 22 tables, SQLite 8, MySQL 3, Oracle 3, SQL Server 3 with correct TOP 10, MongoDB 5 collections then 6 with a view, ClickHouse 5, Couchbase 3 with SQL++, Druid 2 datasources, Redis 17 real key prefixes. LibreDB is the exception and the cause is measured: lib.open() takes an exclusive file lock and the grounding read builds a second provider on a path the connection's own provider already holds. Left open, filed rather than hidden: LibreDB's lock, a Redis plan that can still name KEYS, and MongoDB's missing authSource. Closes #414.
16 of 18 tasks
3 tasks done
cevheri
added a commit
that referenced
this pull request
Sep 6, 2026
…colour (#402) (#607) * test(theme): measure accent contrast instead of recording it in prose Every contrast figure in the repository is prose: measured once by hand and written into a comment. Prose does not fail when the value it describes moves, which is how a token tuned against a near-black ground survived into a light theme at 1.46:1 (#402). tests/helpers/contrast.ts does the arithmetic the browser does — oklch through Oklab to clipped sRGB, alpha compositing in the gamma-encoded space, WCAG luminance in the linear one — and reads the Tailwind palette out of node_modules rather than transcribing it, so a Tailwind upgrade that restyles blue-400 reports itself as the dark-mode change it is. theme.css gains the accent layer #384 left out of scope: four state roles with two text steps, a wash and a filled ground each, plus a fifteen-hue identity palette selected per mode the way charts/palette.ts selects. Light floor 4.79:1, dark floor 4.97:1, and the light identity set is no tighter anywhere than the dark set it is modelled on. * fix(theme): tokenise the accents, so light stops painting dark-tuned colour (#402) 697 colour literals across 68 files move onto the token layer #384 built. Two families, because colour here answers two different questions: brand/warning/ success/danger where it tracks a condition, and a fifteen-hue identity palette where it labels a thing — which engine, which panel, added vs removed, primary key vs foreign key. The state family is brand, not accent. shadcn owns --accent (its neutral hover ground, #f5f5f5) and globals.css maps --color-accent to it AFTER importing the studio layer, so a token of that name loses the cascade in silence and paints near-white text on a white page. The compiled stylesheet is the only place that shows; no unit test can see it. Dark is a byte-for-byte no-op except where a -500 text literal was drift — the codebase used three steps of one hue for one meaning, and those collapse onto the base token. Every such site is listed in the PR. Also: the tests that asserted a hue name now assert the token, and are renamed where the name said 'returns green'. * fix(theme): repair the review's findings — a wrong Oklab matrix, an inverted hover, and nine repainted dark grounds Three defects an independent review found, none of which any gate could see. The Oklab a-row in the contrast helper was transcribed wrong: -2.4285922050 became -0.2429228246 and the sign on the s term flipped. Contrast never touches Oklab so every ratio stayed correct, but every separation figure was measured in a space that is not Oklab — including the ones that chose the palette. Corrected against Ottosson's published values, with a round-trip check so the next slip fails instead of measuring quietly. In the right space the natural assignment wins: hue-amber returns to amber-800 (the step warning already uses) and yellow's alt to -900; orange moves to -900 instead, because Tailwind's amber ramp rotates into orange as it darkens. Three error pages hovered DOWN the ramp where the rest of the app hovers up. Flattening them onto -solid-hover inverted the direction and took white-on-hover from 6.8:1 to 3.8:1. They get -solid-active, and a test now measures what a filled control's label survives — the AA sweep had excluded solids as grounds, so nothing did. Nine monitoring grounds and borders were routed through success/warning, which are emerald and amber, while the literals were green and yellow: a hue change in dark, outside the stated exception. They take the identity hues that reproduce them exactly. A pair-by-pair audit of the whole diff now reports 93 differences, all of them the sanctioned -500/-200 text drift, and zero others. Also: two assertions that compared an object to itself, a GROUNDS.dark that was never read, a tint ceiling asserted rather than derived (four live sites use /25, one of them the pill this issue is about), and a word boundary that let bg-brand-tint satisfy --studio-brand. * fix(theme): measure faded accent text, and stop a token resolving light on a black ground Two external reviews on #607. One found a real regression, one found a real hole in the gate; the smaller items are fixed here too. CreateTableModal's SQL preview is a terminal: a hard `bg-black` in both themes. Routing its text through a mode-aware token made light resolve blue-700 on black — 5.27:1 on main, 2.30:1 here. It is now scoped to the dark palette the way the login hero scopes its own pinned-dark column, and measures 5.27:1 again. The contrast gate only ever measured an OPAQUE foreground, so it could not see `text-warning/80` and its 45 siblings: an opacity modifier composites before anyone reads it, and a token that clears AA at full strength can fall under it faded. The suite now measures those, and holds them to the rule the rest of the issue works to — light no worse than dark — with the sites that still fail pinned by name rather than described in a sentence. Measured against main, this migration improved every one of those 38 sites in light (worst 1.35:1 → 6.80:1) and regressed none, which is why they are a second failure class rather than this one. Also: the Explain button's hover had collapsed onto its own base, because the sanctioned -500 text drift moved the base to what the hover used to be; getThresholdColor drew two of three rings from the identity family and one from the state family, at identical values, so the mix was invisible and is now asserted against; the mode-independence check enumerated suffixes by hand and so skipped brand-solid-active entirely; and both the token comment and the theming doc said nineteen engines where DatabaseType has seventeen. * fix(test): sort the source walk, so the faded-accent lists are not filesystem order CI failed on a list with identical membership and a different order. The scans that read `src` walked it with `readdirSync`, which returns filesystem order, so every list built on them was machine-ordered: the pinned set happened to match the order of the machine it was written on and matched nothing else. Locally it was green, which is the worst version of this bug — the suite was reporting the developer's inode order as a fact about the codebase. Sorted at the walk rather than only at the assertion, in both files that do it, so nothing else built on those scans inherits it. Verified by reversing the walk and re-running: still green, which is the control that a sort matching one machine would not have passed.
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
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.
Description
Studio was pinned to one theme:
darkwas hardcoded on<body>and ~1400 colourliterals were written straight into components, so the shadcn token layer beneath
them never had any effect. This PR moves colour to a single source, publishes that
source with the package, and adds a working dark ↔ light toggle.
Reviewing that surface turned up four unrelated defects in the query-execution
hook — all of which fail silently — so they are fixed here too, in their own
commit.
Type of Change
Related Issue
No tracking issue — found during a frontend review.
Changes Made
fix(query)— four silent defects (205c3d4)unmounting the studio) left it running and its plan could land on a tab whose
results it no longer described.
whichever answered last won, so the user could read A's rows under B's statement.
finallycleared the abort refs unconditionally — a run finishing after itsreplacement started wiped the newer run's controller, leaving Cancel aborting
nothing and the server-side cancel never sent.
useStorageSyncdropped a collection whose push failed; the write survived inlocalStorage and nowhere else until the user happened to touch that same
collection again. Failed pushes now re-queue with backoff.
feat(theme)— the token layer (911e873)src/styles/theme.css: five surface levels, a six-step text ramp, hairlinesand control edges. Both palettes are selected, not derived — the dark values
are the literals they replaced, so tokenising a component is a no-op in dark.
dist/styles.css(staged byscripts/copy-theme.mjs; tsupresolves
.cssimports to empty modules on purpose).ThemeProvideris mounted only in the standalone layout, so an app embeddingthe library surface follows its own
darkclass with no provider and nocompeting toggle.
ThemeTogglerendersnullwhen it finds no provider.darkclass (it is a designeddark surface with a literal white heading). Four other white-on-white bugs fixed.
fix(charts)— token-blind canvases (4e2ba83)@xyflowdiagram and every recharts tooltip resolve no CSS token, sothey are handed a palette via
useEffectiveTheme, which observes the document'sdarkclass rather than next-themes — one source answers both deployments.the data-viz validator on its own surface — green vs amber at CVD ΔE 5.7 against
an 8.0 target, i.e. two adjacent series a red-blind reader could not separate.
Replaced with a palette validated in both modes.
slots sit near 2:1 against white — legible as a wedge, unreadable as a word.
docs(theme)— post-merge correctionsmainsettled that studio is no longer embedded in libredb-platform and deletedthe rules file carrying that contract. The theme code still argued from it, so
the comments now say what is true: an app embedding the library surface must
import the stylesheet. The design is unchanged; only the justification was stale.
theme.cssnow spells out the consequence of skipping that import.Testing
Verified green after the merge:
format·lint·typecheck·knip·test:components(30/30 groups) ·build·build:lib·attw.Beyond unit tests, both themes were captured against a production build with
Playwright and checked by measuring computed styles rather than eyeballing
screenshots — dark is pixel-identical where it was meant to be, and the reload-into-
a-stored-theme path reports 0 hydration errors. The token migration was
re-verified after merging
main: one intentional literal remains, a spinner insidea solid blue button.
The chart palette was checked with the data-viz palette validator against this app's
own chart surfaces, not assumed.
New tests:
theme-toggle,theme-provider,use-effective-theme,charts/palette,plus tooltip-theme assertions on the admin overview and audit charts. Two new
isolation groups were added to
tests/run-components.sh(both files mocknext-themes, which is process-wide).Also fixes an unrelated pre-existing flake:
VisualExplain > clicking Re-analyze…asserted on a spy over
AbortController.prototype.abort. That spy is global — itcounts aborts from every component sharing the process — so a neighbour (AgentRail
creates its own controllers) could fail it: measured at 2 failures in 4 runs on
main, and the merge made it worse. It now asserts on the first request's ownAbortSignal, which belongs to exactly one request and nothing else can touch.Five consecutive full-suite runs clean.
Test Environment
Screenshots (if applicable)
Checklist
Additional Notes
Consumers embedding the library surface must import the stylesheet. Studio's
globals.cssis not packaged, so a tokenised surface resolvesvar(--studio-*)tonothing — invalid at computed-value time, which drops grounds to
transparentandhairlines to
currentColor. The token layer is therefore published on its own:Standalone studio is unaffected;
globals.cssimports it directly.Three deliberate changes to the dark theme, each flagged because the working rule
elsewhere in this PR was "dark must not change":
(see above). This is the only visible dark change of any size, and it is an
accessibility fix, not a preference.
MetricChart's tooltip ground#111→#18181b, so the three charts share onestyle instead of three near-identical copies.
The toggle is two-state (dark ↔ light), deliberately. No "follow the system"
step: one click on, one click back. As a consequence
enableSystemis off, andbecause next-themes then writes a stored
"system"to the document as a literalclass, the storage key moved to
libredb-theme— a clean slate rather than amigration.
Not fixed, worth a follow-up:
CHART_COLORS[index % length]means a 9th seriesrepaints as the 1st, indistinguishable. The fix is series capping / folding into
"Other", which is a product decision rather than part of theming.
Local test caveat: 87 tests fail on this machine for want of a
helmbinary(chart-template tests) and JWT env; identical on
main, and CI has both.