Skip to content

feat(desktop): import browser cookies into a profile - #7255

Open
juliusmarminge wants to merge 18 commits into
shared-sqlite-clientfrom
browser-profile-import
Open

feat(desktop): import browser cookies into a profile#7255
juliusmarminge wants to merge 18 commits into
shared-sqlite-clientfrom
browser-profile-import

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Aug 16, 2026

Copy link
Copy Markdown
Member

Stacked on #7272 — review that first; this PR's base is shared-sqlite-client.

Imports cookies from a browser already installed on the machine into a T3 Code browser profile. Helium on macOS is the first source.

Cookies carry the logged-in sessions, which is what makes an imported profile useful. Saved passwords are deliberately out of scope — Electron exposes no password store to put them in, so importing them would mean building a credential vault and autofill injection.

The consent path is the design

The key is read through the in-process Keychain API, not by shelling out to /usr/bin/security. macOS attributes both the prompt and the resulting ACL grant to the binary that asks:

prompt says grant belongs to
security CLI "security" a tool any process can invoke
in-process T3 Code this app

There is no fallback when consent is denied. The techniques that work around it exist to defeat exactly this, and this feature isn't worth shipping them. unsupportedPlatform covers the case that isn't a permission at all — Chrome on Windows, whose App-Bound Encryption is designed to stop this.

The read is untimed: macOS answers it with a modal, and a timeout racing the user means the prompt can be approved after nothing is left listening — which reads as "approving did nothing".

Notes for review

Sources pin their own coordinates. Chromium forks disagree: Helium uses keychain service Helium Storage Key / account Helium, where Chrome and closer relatives use <Name> Safe Storage / <Name>.

expires_utc overflows JS safe integers — microseconds since 1601, which node:sqlite refuses to narrow. The division happens in SQL so only seconds cross the boundary. This affected nearly every cookie.

The cookie DB is snapshotted before reading, since Chromium keeps it open with WAL and reading in place can observe a torn write.

Packaging: the platform .node is staged beside the loader, mirroring the existing Clerk passkey handling — pnpm nests the arch package, electron-builder only retains top-level deps, and the napi loader checks for a sibling binary first. Without this it works in dev and fails when packaged.

Failures are differentiated rather than collapsed into "approve the prompt", which is useless advice for a missing keychain item.

Testing

Typecheck and lint clean; suite passing. Verified end to end in the desktop app against a real Helium profile: 5,002 cookies imported, 26 skipped, with the prompt correctly naming T3 Code.

The decrypt path can't be unit-tested — it requires a Keychain prompt to be answered — so it is exercised in the app rather than in CI.

Note

Add browser cookie import wizard to desktop profile settings

  • Adds a multi-step import wizard (BrowserImportWizard.tsx) guiding users through importing cookies from Chromium-family browsers into a new or existing profile, with steps for quit prompt, source/target selection, progress, success summary, and error handling.
  • Implements a new BrowserImport desktop service (BrowserImport.ts) that lists available browser sources and imports cookies via readChromiumCookies, writing them into the target Electron session partition.
  • Reads Chromium cookies on macOS by snapshotting the SQLite DB (including WAL sidecars), decrypting values via OSCrypt/AES-128-CBC using the macOS keychain password (ChromiumCookies.ts).
  • Exposes two new IPC endpoints (PREVIEW_IMPORT_SOURCES_CHANNEL and PREVIEW_IMPORT_COOKIES_CHANNEL) wired through preload, main process handlers, and the DesktopPreviewBridge contract.
  • Replaces the standalone default-profile selector in Integrations settings with per-row actions (set default, clear cookies/cache, remove, import), and removes the browser-default-profile search entry.
  • Stages @napi-rs/keyring native binaries during desktop artifact builds to satisfy sibling-first resolution.
  • Risk: cookie import is macOS-only; non-darwin platforms receive a unsupportedPlatform error. Profile directory inputs are hardened against path traversal via isSafeProfileDirectory.

Macroscope summarized 2d762e5.


Note

High Risk
Reads and decrypts another app’s cookie database via Keychain and writes into Electron partitions; security-sensitive despite path/profile validation and explicit non-bypass of consent.

Overview
Adds cookie import from installed Chromium-family browsers (first source: Helium on macOS) into T3 Code preview profile partitions, wired through new contracts, desktop BrowserImport / ChromiumCookies services, and IPC (listBrowserImportSources, importBrowserCookies) exposed on the preload bridge.

On macOS the flow reads the browser’s SQLite cookie store (with a WAL snapshot), decrypts v10 values using the OS keychain via in-process @napi-rs/keyring (no security CLI fallback), validates source profiles to block path traversal, refuses import while SingletonLock indicates the browser is running, then writes cookies into the target Electron session with host-only vs domain-cookie handling. Packaging stages keyring .node binaries like existing Clerk passkey natives.

Integrations → Browser profiles gains an Add profile menu (blank profile or import from a detected browser), a multi-step BrowserImportWizard, per-row actions (set default, clear data, remove), and Default badges; the separate default-profile selector and its settings-search entry are removed. New profiles from import are persisted only when imported > 0.

Unit tests cover source detection/hardening, wizard step logic, and cookieScope; keychain decrypt is exercised manually per PR notes. BrowserImportWizard still has a TEMP layout comment to clean up before merge.

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

@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: 206eb18f-5030-4953-a326-6ae45cca16a6

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 new BrowserImport service, its Chromium cookie helper, and the IPC/layer wiring against the Effect service conventions.

Service shape, module layout (Context.Service tag + inline interface, make, layer), namespace imports, and layer composition in main.ts all look correct. The findings below are about the error model: the new failure type is unstructured (reason: Schema.String) and every construction discards the underlying cause, including one that erases a structured BrowserSession error.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/desktop/src/preview/BrowserImport/BrowserImport.ts
Comment thread apps/desktop/src/preview/BrowserImport/ChromiumCookies.ts Outdated
Comment thread apps/desktop/src/preview/BrowserImport/BrowserImport.ts Outdated
Comment thread apps/desktop/src/preview/BrowserImport/BrowserImport.ts Outdated
Comment thread apps/desktop/src/preview/BrowserImport/BrowserImport.ts Outdated
Comment thread apps/desktop/src/preview/BrowserImport/Sources.ts Outdated
@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: 40385ef · 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.

@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.

UI Consistency

One finding in the changed browser-profiles UI (apps/web/src/components/settings/IntegrationsSettings.tsx).

The menu/primitive usage itself looks consistent with the shared system: Menu/MenuPopup/MenuItem/MenuSub from components/ui/menu, MenuTrigger render={<Button …/>} matching the existing pattern in PreviewMoreMenu/ProjectScriptsControl, existing size="icon-sm" / variant="ghost-muted" Button variants, min-w-* on popups honouring MenuPopup's width-defaulting contract, and the bordered list container matching the convention already used in ConnectionsSettings. aria-labels are preserved on both the rename input and the new row trigger.

The issue is a state-display regression introduced by removing the default-profile Select while filtering Incognito out of the new list: a stored default of incognito (which the removed Select allowed a user to pick) now leaves the section with no "Default" badge on any row.

Posted via Macroscope — UI Consistency

Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx Outdated
@juliusmarminge
juliusmarminge force-pushed the browser-profile-import branch from 757f025 to 5853095 Compare August 16, 2026 22:08
@github-actions github-actions Bot added the 📱 Native Change Changes the native fingerprint; merging blocks production OTAs until a new store build ships. label Aug 16, 2026
Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx
@juliusmarminge
juliusmarminge marked this pull request as ready for review August 16, 2026 22:18
Comment thread apps/desktop/src/preview/BrowserImport/BrowserImport.ts
Comment thread apps/desktop/src/preview/BrowserImport/ChromiumCookies.ts
@macroscopeapp

macroscopeapp Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

4 blocking correctness issues found. New feature introducing browser cookie import capability, touching keychain APIs and external browser data. Contains an unresolved HIGH severity finding about symlinked profile directories potentially allowing reads outside intended paths, plus multiple race condition concerns in the UI layer.

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

@juliusmarminge
juliusmarminge force-pushed the browser-profile-import branch from 5853095 to 23b22b9 Compare August 16, 2026 22:53
@juliusmarminge
juliusmarminge changed the base branch from browser-profiles to shared-sqlite-client August 16, 2026 22:54
Comment thread apps/desktop/src/preview/BrowserImport/ChromiumCookies.ts
Comment on lines +786 to +809
disabled={disabled}
aria-label={`${profile.name} options`}
/>
}
>
<MoreVertical />
</MenuTrigger>
<MenuPopup align="end" className="min-w-44">
<MenuItem
disabled={isDefault}
onClick={() => updateSettings({ browserDefaultProfileId: profile.id })}
>
Set as default
</MenuItem>
<MenuItem onClick={() => clearProfileData(profile.id, profile.name)}>
Clear cookies and cache
</MenuItem>
{builtIn ? null : (
<MenuItem
variant="destructive"
onClick={() => setProfilePendingRemoval(profile)}
>
Remove profile and data
</MenuItem>

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.

🟡 Medium settings/IntegrationsSettings.tsx:786

Per-profile actions remain usable while busy is true, so a user can remove a profile during an in-flight import; removeProfile clears its partition, but the import can then write cookies back into that orphaned partition. Disable the profile action trigger and menu items while the import is running.

-                      disabled={disabled}
+                      disabled={disabled || busy}
@@
-                    disabled={isDefault}
+                    disabled={busy || isDefault}
@@
-                  <MenuItem onClick={() => clearProfileData(profile.id, profile.name)}>
+                  <MenuItem disabled={busy} onClick={() => clearProfileData(profile.id, profile.name)}>
@@
-                      onClick={() => setProfilePendingRemoval(profile)}
+                      disabled={busy}
+                      onClick={() => setProfilePendingRemoval(profile)}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/settings/IntegrationsSettings.tsx around lines 786-809:

Per-profile actions remain usable while `busy` is `true`, so a user can remove a profile during an in-flight import; `removeProfile` clears its partition, but the import can then write cookies back into that orphaned partition. Disable the profile action trigger and menu items while the import is running.

Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx

@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 new BrowserImport service, its Sources/ChromiumCookies helpers, the IPC method, and the layer wiring against the Effect service conventions. The service module follows the canonical shape (errors → Context.Service with inline interface → makelayer), dependencies are acquired from the environment, and the failure translations now keep a real cause. One remaining gap on error context.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/desktop/src/preview/BrowserImport/ChromiumCookies.ts

@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.

UI Consistency — 1 finding

The two issues flagged on earlier commits are addressed: loadSources now clears sources before each refresh, and resolvedDefaultId resolves against the rendered rows.

One consistency gap remains: with BrowserDefaultProfileSetting removed and Incognito no longer rendered as a row, the section can badge Default on a profile that is not the effective default (see inline comment).

Minor (not blocking): lines 507–529 now carry three consecutive doc comments for a single component — the "Create, rename, and remove browser profiles" and "Per-profile cookie import" blocks are leftovers from the removed/renamed pieces and could be folded into one.

Posted via Macroscope — UI Consistency

Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx

@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.

Two findings in the reworked browser-profiles section. Both are about the settings UI now disagreeing with the runtime behaviour it configures, rather than styling.

Posted via Macroscope — UI Consistency

Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx
Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx Outdated
Comment thread apps/desktop/src/preview/BrowserImport/ChromiumCookies.ts
Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx
@juliusmarminge
juliusmarminge force-pushed the browser-profile-import branch from 57fccc1 to ea2efcb Compare August 16, 2026 23:21
Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx Outdated
@juliusmarminge
juliusmarminge force-pushed the browser-profile-import branch from 3264ff7 to 76fc9d2 Compare August 16, 2026 23:59
juliusmarminge and others added 14 commits August 17, 2026 10:09
The list was a loose stack of rows with two competing controls and a separate
"Default browser profile" setting duplicating what the list already showed.

Profiles now render as a table. Which one is default is a badge on its row and
is changed from that row's menu, so the standalone setting is gone. Each row
also gains "Clear cookies and cache", scoped to that profile's partition.

Creating and importing collapse into one "Add profile" menu, because from the
user's side they are one decision: "I want a profile with my Helium logins in
it". Each source offers its targets directly — New profile, or any existing
one — so importing into a fresh profile no longer means creating it first and
then hunting for a second control. Previously import offered no choice of
target at all.

Incognito is not listed. It keeps nothing between launches, so it has no data
to clear, no name to edit, and no state to manage; it belongs in the menu that
opens a tab. It is also excluded as an import target, since importing into it
would be discarded on quit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sers

`importCookies` forwarded the IPC-supplied `sourceProfileDirectory` straight
into the cookie database path, so `..` segments walked out of the browser's
user-data directory and imported any cookie database reachable on disk. It is
now only honoured when the source itself reported it.

The running-browser check used `access` on Chromium's `SingletonLock`, which is
a symlink to a `<host>-<pid>` target that never exists. Following it reported
every running browser as closed, so an import could read a live, mid-write
database — Helium open on this machine was detected as closed. It now stats the
link itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The import module reached for `node:fs/promises`, `node:os`, `node:path` and
`node:sqlite` directly and threaded results through hand-rolled promises, which
meant a blanket `nodeBuiltinImport:off` on two files and failures that were
plain thrown `Error`s.

It now uses FileSystem, Path and the shared Effect SQL client, and the cookie
snapshot is a scoped resource rather than a try/finally with a cleanup
callback. `node:crypto` stays — it implements the OSCrypt primitives Chromium
uses and has no Effect equivalent — with the suppression narrowed to it and a
reason attached.

Failure reasons are a typed union in contracts rather than free strings, so the
renderer maps a reason to copy instead of substring-matching an error message.

Porting `isSourceRunning` to `FileSystem.stat` reintroduced the dangling
SingletonLock bug, because `stat` and `exists` both follow symlinks; `readLink`
is the probe that answers for the entry itself. The regression test caught it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing a profile deleted it and wiped its cookies and cache from a single
icon-menu click, while every comparable destructive action in Settings confirms
first. It now routes through the same AlertDialog.

The Default badge resolved against the unfiltered profile list while the table
renders only non-incognito rows, so a stored default of "incognito" left the
section with no default marked at all. It now resolves against the rows that
render.

Reopening the import menu kept the previous source list on screen while the
refresh was in flight, leaving a source that had since become unavailable
selectable; the list is cleared first so the menu shows its loading state.

A cookie sidecar that exists but cannot be copied is no longer ignored
alongside the missing-file case. SQLite would open the snapshot without the
write-ahead log and return a cookie set silently missing its newest
transactions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ChromiumCookieReadError` carried only a reason and a cause, so every
`readFailed` and keychain refusal logged identically. A user with several
Chromium browsers installed had no way to tell which one refused. The database
path is now a structural attribute and the message derives from it, matching
how `BrowserSession`'s errors carry their partition.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Renaming the component left three consecutive doc blocks above it, two of them
describing controls that no longer exist separately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Detection keyed off the browser's user-data directory, which is not evidence
the browser exists. Installers for native messaging hosts create an empty one
for every Chromium fork they know about, so a machine with only Chrome and
Helium listed Edge, Brave, Vivaldi, Opera and Arc as importable sources — each
holding nothing but a `NativeMessagingHosts` folder.

It now keys off the cookie database, which is the thing an import actually
needs. Existence is checked without opening the file, which matters for Safari:
TCC permits `stat` on the jar inside its container but refuses a read, so
Safari is still found and the user gets the Full Disk Access prompt instead of
Safari vanishing from the list.

A source that is not on the machine is now left out of the menu rather than
shown as a dead row. Every other unavailable reason stays visible, because each
names something the user can do — quit the browser, grant access.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`importInto` created the target profile before `runImport` checked for an
environment and a bridge, so choosing an import target before the environment
resolved left a new empty profile named after the source browser and produced
no toast at all. The check now happens first and says what went wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`clearProfileData` returned silently when no environment was connected, while
the menu item stayed enabled — a dead control with no explanation. It now
reports the same way `importInto` does for the same precondition.

Also switches `browserImport` to the subpath namespace import the rest of
`packages/contracts` uses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A `Badge` carries no disabled treatment of its own, so the solid `bg-primary`
pill stayed at full strength in the desktop-only block while the name, rename
field and row menu button around it all sat at 0.64. The badge it replaced was
inside the dimmed span, so this was a regression.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`info_cache` keys are directory names from the browser's own metadata file,
which anything running as the user can write. A key like `../../../../secrets`
was returned as a profile and handed to `cookieDatabasePath`, so the import
would read a database outside the browser's user-data directory — the same
escape the IPC-side guard closes, reached through the other door.

Only a single plain path segment is accepted now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every imported cookie passed `domain` to `session.cookies.set`. Electron reads
any `domain` as marking a domain cookie and normalizes it with a leading dot,
so every host-only row — Chromium stores those without one — came out scoped to
all subdomains of the host it had been confined to. It also rejects `__Host-`
cookies, which require `domain` to be absent.

`domain` is now carried only for rows Chromium marked as domain cookies, and
omitted from the write otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fallback assumed a single `Default` profile, so a browser whose cookies
live in `Profile 1` reported nothing to import — and since sources without a
cookie database are now left out of the menu, it disappeared entirely rather
than degrading.

The user-data directory is scanned for directories that hold a cookie database
instead, which is the same signal the install check uses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rows `decryptChromiumValue` could not read were dropped without a trace, so an
import that recovered a fraction of the database still reported a clean
success. They now reach the user as part of the skipped total.

This matters most on Linux, where records written under a keyring-derived
`v11` key are unreadable unless that secret is reachable — see the note in
`ChromiumKeys`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread apps/desktop/src/preview/BrowserImport/ChromiumCookies.ts
catch: () => undefined,
}).pipe(
Effect.as(true),
Effect.catchCause(() => Effect.succeed(false)),

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.

🟡 Medium BrowserImport/BrowserImport.ts:200

Cancelling the import fiber is converted to false here, so a shutdown or other interruption does not stop the loop and it continues attempting every remaining session.cookies.set write. Effect.catchCause handles interruption as well as promise failures; catch only the typed failure channel so interruption propagates.

Suggested change
Effect.catchCause(() => Effect.succeed(false)),
Effect.catch(() => Effect.succeed(false)),
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/desktop/src/preview/BrowserImport/BrowserImport.ts around line 200:

Cancelling the import fiber is converted to `false` here, so a shutdown or other interruption does not stop the loop and it continues attempting every remaining `session.cookies.set` write. `Effect.catchCause` handles interruption as well as promise failures; catch only the typed failure channel so interruption propagates.

Comment thread apps/desktop/src/preview/BrowserImport/BrowserImport.ts
Comment thread apps/desktop/src/preview/BrowserImport/Sources.ts Outdated
Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx
@juliusmarminge
juliusmarminge force-pushed the browser-profile-import branch from 40385ef to 7b8e294 Compare August 17, 2026 08:35
// segment is dropped: `..` or a path separator would otherwise be handed
// to `cookieDatabasePath` and read a database outside the user-data
// directory.
Effect.map((entries) => entries.filter(([directory]) => isSafeProfileDirectory(directory))),

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.

🟠 High BrowserImport/Sources.ts:105

A symlinked profile such as <userData>/Default lets cookieDatabasePath resolve and import <external>/Cookies outside the browser user-data directory. isSafeProfileDirectory only validates the key lexically, so it does not enforce the path-containment guarantee described here; resolve the profile path and verify it remains under root (or reject symlinked profile components) before returning it.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/desktop/src/preview/BrowserImport/Sources.ts around line 105:

A symlinked profile such as `<userData>/Default` lets `cookieDatabasePath` resolve and import `<external>/Cookies` outside the browser user-data directory. `isSafeProfileDirectory` only validates the key lexically, so it does not enforce the path-containment guarantee described here; resolve the profile path and verify it remains under `root` (or reject symlinked profile components) before returning it.

@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 profile badge in the preview chrome row uses a native title tooltip where every other hover hint in that row goes through the shared Tooltip primitive.

Posted via Macroscope — UI Consistency

The Add-profile menu buried imports in nested submenus and disabled rows —
a running browser was a dead "quit it" line you couldn't act on, and picking a
source profile meant hunting through submenus. The menu now lists each browser
as a plain row; clicking one opens a wizard that carries the whole import.

The wizard has a screen for every state instead of a disabled row: quit the
browser and retry, choose which source profile and where it lands, then import.
A new target profile is registered only once cookies actually arrive, so a
blocked or empty import leaves nothing behind.

Sources are cached and refreshed without blanking, so the menu no longer
reflows on open. The step transitions are pure and tested.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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.

UI consistency review of the new browser-import surfaces. Three findings in apps/web/src/components/settings/BrowserImportWizard.tsx; the earlier findings in IntegrationsSettings.tsx (default-badge dimming, Clear cookies and cache silent no-op, default resolution against the rendered list) all look addressed at this head.

Posted via Macroscope — UI Consistency

Comment thread apps/web/src/components/settings/BrowserImportWizard.tsx Outdated
Comment on lines +257 to +263
function ImportingStep() {
return (
<DialogPanel className="flex items-center gap-3 py-6">
<Spinner className="size-4 text-muted-foreground" />
<span className="text-sm text-muted-foreground">Importing cookies…</span>
</DialogPanel>
);

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.

ImportingStep renders a bare DialogPanel, so while it is on screen the popup has no DialogTitle. Base UI derives the dialog's accessible name from that title, so the dialog goes unnamed for assistive tech mid-flow, and visually the popup collapses to a spinner-only box between the configure and done screens. Every other step here supplies a header.

Suggested change
function ImportingStep() {
return (
<DialogPanel className="flex items-center gap-3 py-6">
<Spinner className="size-4 text-muted-foreground" />
<span className="text-sm text-muted-foreground">Importing cookies…</span>
</DialogPanel>
);
function ImportingStep() {
return (
<>
<DialogHeader>
<DialogTitle>Importing cookies…</DialogTitle>
</DialogHeader>
<DialogPanel className="flex items-center gap-3 py-6">
<Spinner className="size-4 text-muted-foreground" />
<span className="text-sm text-muted-foreground">This can take a moment.</span>
</DialogPanel>
</>
);
}

Posted via Macroscope — UI Consistency

};

const recheckAfterQuit = () => {
setStep({ step: "importing" });

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.

Re-checking after the user quits the browser reuses the importing step, so the dialog shows the spinner labelled Importing cookies… while nothing is being imported — the import only starts later, from the configure screen. The same screen then means two different things depending on how it was reached.

Consider tracking which action is pending (re-check vs. import) and passing the label into ImportingStep, or adding a distinct checking step in browserImportWizard.logic.ts, so the busy copy matches the work in flight.

Posted via Macroscope — UI Consistency

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 05fe2c1. Configure here.

Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx
Comment thread apps/web/src/components/settings/IntegrationsSettings.tsx
juliusmarminge and others added 2 commits August 17, 2026 11:51
…Into

The configure step was two flat radio lists. It's now a From → Into layout —
the source profiles on one side, the target on the other, side by side when the
dialog has room and stacked when it doesn't — so the copy direction reads at a
glance.

Each source profile shows how many cookies it holds, counted with a bare
`COUNT(*)` that needs no decryption, so picking "You — 5,065 cookies" over
"test — 6 cookies" is an informed choice. The count is absent where the store
can't be read yet (Safari before Full Disk Access).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An import that skipped some cookies only said how many. It now says which
sites they belonged to — the reader collects the hosts of rows it couldn't
decrypt, the writer adds the hosts it couldn't set, and the import result
carries the distinct list (capped, since a broken key can skip thousands). The
wizard's done screen reads "example.com, google.com and 3 more".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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.

Two findings in the import wizard's changed lines. Earlier findings on this PR (the importing step doing double duty for the post-quit re-check, and ImportingStep rendering without a DialogTitle) are still open but not re-posted here.

Posted via Macroscope — UI Consistency

Comment on lines +199 to +202
// TEMP: an in-dialog layout switcher for comparing directions live — the ui.sh
// picker can't load under the app's CSP. Collapse to the chosen variant and
// delete this switcher before merge.
function ConfigureStep({

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.

This TEMP note describes an in-dialog layout switcher that no longer exists and asks for its removal before merge, so it now misdescribes ConfigureStep for anyone reading the file. Worth dropping it (and folding the single-consumer ConfigureFooter extraction back into ConfigureStep, since it was only shared between the switcher's variants).

Suggested change
// TEMP: an in-dialog layout switcher for comparing directions live — the ui.sh
// picker can't load under the app's CSP. Collapse to the chosen variant and
// delete this switcher before merge.
function ConfigureStep({
function ConfigureStep({

Posted via Macroscope — UI Consistency

Comment on lines +300 to +309
<button
type="button"
onClick={onSelect}
className={cn(
"flex w-full items-center justify-between gap-3 rounded-lg border px-3 py-2 text-left transition-colors",
selected
? "border-primary bg-primary/8"
: "border-border/60 hover:border-border hover:bg-muted/40",
)}
>

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.

SelectableTile is the wizard's only selection control, but the raw <button> exposes its state visually only — no aria-pressed/aria-checked, so assistive tech reads the two mutually exclusive lists as plain buttons and never announces which profile is picked. Every comparable custom tile in the repo carries that state (ConnectionsSettings connection-mode cards use aria-pressed, its endpoint tiles use role="radio"/aria-checked inside a labelled role="radiogroup"), and those also add a focus-visible ring since the tile opts out of the Button primitive's ring.

Smallest fix is to expose the pressed state and restore the ring here; if you want full radio semantics instead, wrap each <section> in role="radiogroup" with an aria-label and switch to role="radio"/aria-checked.

Suggested change
<button
type="button"
onClick={onSelect}
className={cn(
"flex w-full items-center justify-between gap-3 rounded-lg border px-3 py-2 text-left transition-colors",
selected
? "border-primary bg-primary/8"
: "border-border/60 hover:border-border hover:bg-muted/40",
)}
>
<button
type="button"
aria-pressed={selected}
onClick={onSelect}
className={cn(
"flex w-full items-center justify-between gap-3 rounded-lg border px-3 py-2 text-left outline-none transition-colors focus-visible:ring-2 focus-visible:ring-ring",
selected
? "border-primary bg-primary/8"
: "border-border/60 hover:border-border hover:bg-muted/40",
)}
>

Posted via Macroscope — UI Consistency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ 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