Skip to content

refactor(shared): move the node:sqlite Effect SQL client into shared - #7272

Open
juliusmarminge wants to merge 1 commit into
browser-profilesfrom
shared-sqlite-client
Open

refactor(shared): move the node:sqlite Effect SQL client into shared#7272
juliusmarminge wants to merge 1 commit into
browser-profilesfrom
shared-sqlite-client

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Aug 16, 2026

Copy link
Copy Markdown
Member

Stacked on #7254 — review that first; this PR's base is browser-profiles.

The desktop app needs to read browser cookie databases and should reach for the same Effect SQL client the server already uses, rather than opening node:sqlite by hand. Nothing about the client is server-specific, so it moves to packages/shared and both apps import it from there.

Pure move plus import updates — the client itself is unchanged. Split out of #7255 so the cookie-import work reviews on its own.

Model: Claude Opus 5 (1M context), harness: Claude Code.

🤖 Generated with Claude Code


Note

Low Risk
Import-path refactor only; persistence and migration behavior are unchanged.

Overview
Moves the Effect-based node:sqlite SQL client from server persistence into packages/shared as nodeSqliteClient.ts, exposed via @t3tools/shared/nodeSqliteClient.

Server scripts (migrate-dev-db, t3-sqlite-state), migration tests, and the Node SQLite loader in Sqlite.ts now import that shared module instead of ../NodeSqliteClient. The shared package test import is updated to match the new filename.

No behavior changes to the client—relocation and import wiring only, so desktop and other apps can reuse the same layer.

Reviewed by Cursor Bugbot for commit aae2535. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Move NodeSqliteClient from server persistence layer into the shared package

Moves NodeSqliteClient (an Effect-based node:sqlite client) from apps/server/src/persistence/ to packages/shared/src/nodeSqliteClient.ts and exposes it via a new @t3tools/shared/nodeSqliteClient subpath export. All import sites across server scripts, migration tests, and the SQLite layer loader are updated to use the shared path. No logic changes.

Macroscope summarized 4ab2301.

@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0e6c9c99-7cda-46de-ac74-fbe9df62a93e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ changed lines (additions + deletions). labels Aug 16, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changed Effect service code (desktop BrowserSession/PreviewManager, server ProviderService, the NodeSqliteClient move) against the service conventions. Service definition order, inline Context.Service interfaces, make/layer exports, Foo["Service"] references, environment-based dependency acquisition, and the pure module move with no compatibility shims all look consistent. One import-convention deviation in the new contracts module is noted inline.

Posted via Macroscope — Effect Service Conventions

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the new leading-icon utility on MenuSubTrigger also matches the chevron on sub-triggers that have no leading icon, which regresses the two existing icon-less sub-triggers (DiffPanel "Turn", PreviewMoreMenu "Appearance"). Details inline.

Posted via Macroscope — UI Consistency

@macroscopeapp

macroscopeapp Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Correction to my previous review comment: the inline note referenced packages/contracts/src/browserProfile.ts, which belongs to the base branch (browser-profiles) and is not part of this pull request's changed files. Within this PR's actual scope — moving the node:sqlite Effect SQL client to @t3tools/shared/nodeSqliteClient and updating its importers — there are no Effect service convention findings: the move is pure, no compatibility re-export shim was left behind, every consumer was updated to the canonical path, and all imports use subpath namespace form.

Posted via Macroscope — Effect Service Conventions

@macroscopeapp

macroscopeapp Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

UI consistency finding (inline comment could not be attached — apps/web/src/components/ui/menu.tsx is part of the stacked browser-profiles base, not this PR's own file list, so posting it here instead).

apps/web/src/components/ui/menu.tsxMenuSubTrigger leading-icon utility also hits the chevron

The new class string starts with [&>svg:first-of-type]:-mx-0.5. That only excludes the chevron when the trigger has a leading icon. On the two existing icon-less sub-triggers — DiffPanel's "Turn" and PreviewMoreMenu's "Appearance" — the chevron is the first svg of its type, so .trigger > svg:first-of-type (specificity 0,2,1) outranks the chevron's own ms-auto / -me-0.5 classes (0,1,0) and applies margin-inline: -0.125rem. ms-auto is overridden, so the chevron stops being pinned to the right edge and collapses next to the label.

Smallest fix: scope the utilities away from the last child, which never matches the chevron and behaves identically for real leading icons:

-        "[&>svg:first-of-type]:-mx-0.5 flex min-h-8 cursor-pointer items-center gap-2 ... [&>svg:first-of-type:not([class*='opacity-'])]:opacity-80 [&_svg]:pointer-events-none [&>svg]:shrink-0",
+        "[&>svg:not(:last-child)]:-mx-0.5 flex min-h-8 cursor-pointer items-center gap-2 ... [&>svg:not(:last-child):not([class*='opacity-'])]:opacity-80 [&_svg]:pointer-events-none [&>svg]:shrink-0",

(the ... stands for the unchanged middle of the existing class string). Worth a quick check of the emitted CSS plus the "Appearance" and "Turn" submenus after the change.

Posted via Macroscope — UI Consistency

@macroscopeapp

macroscopeapp Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved ea5198f

Pure mechanical file reorganization - moving NodeSqliteClient from apps/server to shared package with import path updates. File content unchanged (100% similarity), no runtime behavior changes.

No code changes detected at 4ab2301. Prior analysis still applies.

You can customize Macroscope's approvability policy. Learn more.

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). 📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. and removed size:XXL 1,000+ changed lines (additions + deletions). labels Aug 16, 2026
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 12.6 KiB 15.1 KiB
Codex Thread snapshot wire 6.3 KiB 7.3 KiB
Codex Live turn WebSocket wire 6.3 KiB 7.8 KiB
Codex Live turn WebSocket decoded 51.8 KiB 66.4 KiB
Codex Live turn messages 16 21
Claude Total thread wire 12.6 KiB 15.1 KiB
Claude Thread snapshot wire 6.3 KiB 7.3 KiB
Claude Live turn WebSocket wire 6.3 KiB 7.8 KiB
Claude Live turn WebSocket decoded 52.7 KiB 66.4 KiB
Claude Live turn messages 16 21

Baseline: unavailable · PR result: 4ab2301 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 99.9 KiB
  • Claude decoded thread snapshot: 100.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@github-actions github-actions Bot removed the 📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. label Aug 17, 2026
@juliusmarminge
juliusmarminge force-pushed the shared-sqlite-client branch 5 times, most recently from ee304c0 to 9cd8c24 Compare August 17, 2026 02:41
The desktop app needs to read browser cookie databases and should reach for
the same Effect SQL client the server already uses rather than opening
`node:sqlite` by hand. Nothing about the client is server-specific, so it moves
to `packages/shared` and both apps import it from there.

Pure move plus import updates; the client itself is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant