feat(desktop): import browser cookies into a profile - #7255
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
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
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 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.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
There was a problem hiding this comment.
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
757f025 to
5853095
Compare
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a substantial desktop capability for reading encrypted browser cookie stores and importing session cookies through new IPC, native keyring, persistence, and packaging paths. The sensitive data flow, production artifact changes, and added static-analysis suppressions warrant focused human review. Not approved because:
No code changes detected at Review your spending limits in Billing settings, or comment |
5853095 to
23b22b9
Compare
There was a problem hiding this comment.
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 → make → layer), 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
57fccc1 to
ea2efcb
Compare
3264ff7 to
76fc9d2
Compare
Two review findings: the import menu only hid uninstalled sources, so a platform that can never import from a browser (a macOS-only fork on Linux) still listed it and clicking led to a dead-end blocked screen — those are now left out too. And the "Default profile" search entry lost the word "browser" when its settings row went away, which broke the "default browser profile" query and read ambiguously beside its siblings; the search-only title says "Default browser profile" again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…key retry Three review findings on the import flow: - Profile discovery accepted any entry named `Cookies`, so a directory listed as an importable profile and then failed the SQLite open; the fallback scan and the installed check now require a regular file. - A "new profile" target chosen before the profile cap was reached could still be imported once it was, creating a profile past the limit; the Import button now disables in that case. - `keychainItemMissing` told the user to sign in and retry but offered no retry — it is now retryable, since signing in is exactly what creates the key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cap guard on the Import button had no feedback: with the "New profile" tile unrendered, nothing in Into looked selected and Import was simply off. The cap case now flows through the same message slot as a vanished existing target, and the disabled condition reuses that one derivation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
At the profile cap the Blank profile item went grey with no explanation while the import rows stayed live, so the menu read as broken rather than full; a noninteractive item now names the cap, matching the menu's other explained states. Also deletes entryExists, which nothing reached once discovery moved to databaseFileExists and the lock probe inlined readLink. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ses anything The nodeBuiltinImport suppression was justified by the signal-0 liveness probe, but that uses the process global, which the diagnostic never inspects — and the module imports no node builtin at all since the hostname moved to HostProcessHostname. Left in place it would silently permit a future node:fs import here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…abort an import
Two review findings. Chromium 96 moved the cookie database to
`Network/Cookies`, but the reader only ever looked at `{profile}/Cookies` —
so an install carrying a leftover root jar imported a stale or empty
database while the real sessions lived under `Network/`, and a fresh install
with only the `Network/` jar read as not installed and was hidden. Discovery,
counts, the installed check and the importer now resolve the first candidate
that is a regular file, most-current first.
And a second Import click before React left the configure screen started a
second run; the parent refused it, and the wizard applied that refusal as a
read failure — dropping out of the importing step mid-write and showing a
false error over a successful import. A synchronous in-flight ref now makes
the second click a no-op.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…database When the cookies imported but the new profile's settings write failed, the wizard reported "The browser's cookie database could not be read" — the one thing that had just succeeded. The failure now carries its own reason, `profileNotSaved`, with copy that says the cookies came over and the profile couldn't be kept (its partition is cleared again so nothing is stranded), and it offers a retry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cookies.set resolves once the cookie is in memory; Chromium persists the store on its own schedule, so a crash right after the wizard said Done could drop the import. Flush once after the writes; a failed flush is logged, not surfaced, since the cookies are in the session regardless.
The cap was only on the rendered controls. createProfile now refuses past BROWSER_PROFILE_MAX_COUNT against the live settings, and the post-import updater does too — the wizard blocks a new target at the cap, but the count can change while the import runs. That case reports profileLimitReached, since a retry cannot lower the count.
The rebase over #9281 resolved in favour of this branch's rewrite of the file and lost its size="sm" select triggers and icon-xs row button.
Stacked on #7272 (
shared-sqlite-client).Imports cookies from Helium on macOS into a T3 Code browser profile. Saved passwords and browsing history remain out of scope.
The app requests the browser key through the in-process Keychain API, so consent belongs to T3 Code. Denial stops the import; there is no permission bypass. Source databases are opened read-only and snapshotted consistently with SQLite before extraction.
The wizard chooses a source and destination, prevents profile edits during an import, and reports imported/skipped counts and affected sites. Plaintext and encrypted cookie records are handled separately; domain-bound records are validated. Partitioned cookies are skipped when their partition semantics cannot be preserved by Electron.
Validation includes synthetic decryption records, plaintext and legacy records, domain-binding failures, partitioned-cookie rejection, WAL snapshot consistency, interrupted writes, and wizard state transitions, plus scoped typechecks/lint. Earlier Helium imports were verified in the desktop app; this audit did not request another live Keychain read.
Original implementation: Claude Code. Review fixes: GPT-5.6 Sol agents, coordinated through Codex.
Note
Add browser cookie import into profiles via
BrowserImportserviceBrowserImportservice, contracts, and IPC methods to list browser sources and import cookies into a target partition.BrowserImportWizardand redesigns browser profile settings to support profile management and cookie imports.useSettingsto prevent race conditions and ensure correct publish order.persistClientSettingsUpdatein useSettings.ts now rejects on persistence failure and recomputes concurrent writes, changing publish timing.Macroscope summarized c5dbc20.
Note
Medium Risk
Touches macOS Keychain reads and cross-browser cookie data with new IPC surface area, plus a behavior change to how client settings are persisted and published.
Overview
Adds cookie import from installed Chromium-family browsers (initially Helium on macOS) into T3 Code preview browser profiles, wired end-to-end from contracts through desktop main, preload, and Integrations settings.
The desktop app introduces a
BrowserImportEffect service that lists sources, blocks imports while the source browser holdsSingletonLock, validates profile directories against listed profiles (path traversal hardening), snapshots the SQLite cookie DB viaVACUUM INTO, reads the encryption key through@napi-rs/keyring(in-process Keychain, no CLI bypass), decrypts Chromium cookies, and writes them into the same Electron partition as the target profile via new IPC channelslistBrowserImportSources/importBrowserCookies.Integrations → Browser profiles is reworked: an Add profile menu offers blank profiles or Import from detected browsers, a
BrowserImportWizardhandles quit-browser / configure / success / blocked flows, per-profile actions include Set as default, Clear cookies and cache, and remove; the standalone default-profile row is folded into the list. New profiles are persisted only after a successful import (with partition cleanup on save failures or profile cap).persistClientSettingsUpdateplus a serialized persistence queue ensures awaited settings writes (e.g. registering an imported profile) don’t race optimistic patches. Desktop artifact builds stage keyring native binaries like other nested native deps.Reviewed by Cursor Bugbot for commit c5dbc20. Bugbot is set up for automated code reviews on this repo. Configure here.