feat(browser-profiles): device-bound browser auth + per-folder feeds + Mac UI overhaul#706
Merged
Conversation
…+ Mac UI overhaul
Schema (auth_profiles)
* device_worker_id + browser_kind + user_data_dir + cdp_url columns.
* A browser_session profile is now either:
- cloud (auth_data jsonb, today's path), or
- device-bound: cookies on the Mac in user_data_dir, or attached
to a running Chrome via cdp_url.
Exactly one of {user_data_dir, cdp_url} when device_worker_id is set.
* Migrations 20260513120000 + 20260513150000.
Server endpoints (worker-JWT scoped)
* /api/workers/me/auth-profiles GET/POST/DELETE — Mac creates browser
profiles for its device. Server enforces device ownership.
* /api/workers/me/feeds GET/POST/DELETE — one feed per local folder, with
folder_id + display_name in the config. Replaces the single auto-wired
files feed.
* /api/workers/me/browser-connectors — dynamic list of connectors with
a browser auth method (no more hardcoded Mac picker).
* manage_connections auto-pins device when a device-bound profile is
picked; rejects mismatches.
* execution-context puts user_data_dir / cdp_url into session_state so
openStealthBrowser launches via launchPersistentContext or attaches
via CDP. Connector code is unchanged (capterra/g2/glassdoor/trustpilot
+ browserNetworkSync threading for linkedin/x/revolut).
Connector SDK
* openStealthBrowser + acquireBrowser + browserNetworkSync gain
userDataDir; new persistent-context launch path.
* FeedDefinition.userManaged: auto-wire skips feeds whose config has
required fields it can't fill. local.directory.files is marked
userManaged so the Mac app creates one feed per folder explicitly.
Mac app
* BrowserProfileManager: discovers Chrome/Brave/Arc/Edge + their
profiles, manages ~/Library/Application Support/Lobu/browser-profiles/,
autoDetectCdpUrl probes 9222-9225.
* BrowserProfilesView (inline under Integrations): one row per installed
browser, inline list of its profiles + Add form with Copy / CDP mode.
* AppState.localFolders ([LocalFolder]) replaces [Data] bookmarks.
LocalFolder.folderId is SHA256(bookmark).prefix(6).hex — deterministic
so a re-added folder maps to the same server feed (no duplicate
history). One-shot migration of legacy [Data] bookmarks on load.
* reconcileFolderFeeds after each poll: creates feeds for new folders,
deletes orphan auto-wired-with-NULL-config feeds, drops server feeds
whose folder_id no longer exists locally.
* Menubar: device label next to "Lobu", per-integration disable toggles
(Screen Time / WhatsApp / Health), unified "+ Add" pattern with
inline expansion, source-path sub-rows for FDA-backed integrations,
inbox click-fallback opens /<slug>/notifications when no resource_url.
Web (submodule)
* AuthProfileItem carries device_worker_id/browser_kind/user_data_dir.
* Picker labels device-bound entries; selecting one locks the device
field.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46e31c5a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
1246
to
1251
| const needsBrowserAuth = | ||
| !!authSelection.browserMethod && | ||
| !!authSelection.authProfile && | ||
| authSelection.authProfile.profile_kind === 'browser_session' && | ||
| !isDeviceBoundBrowserSessionConnect && | ||
| !browserProfileUsable; |
There was a problem hiding this comment.
When a Mac-created browser profile is selected explicitly, hasReadySelection is still false because getBrowserSessionReadiness() only inspects empty server-side auth_data; this new !isDeviceBoundBrowserSessionConnect branch also makes needsBrowserAuth false, so the guard immediately below rejects the request with “Select or create a browser auth profile” instead of creating the device-pinned connection. This breaks the recommended manage_connections(action='connect', auth_profile_slug=...) flow for every device-bound browser profile.
Useful? React with 👍 / 👎.
Comment on lines
1018
to
1021
| (authSelection?.authProfile?.profile_kind === 'browser_session' && | ||
| !isDeviceBoundBrowserSession && | ||
| !browserProfileUsable) || | ||
| authSelection?.authProfile?.status === 'pending_auth' |
There was a problem hiding this comment.
For action='create', a device-bound browser profile created by the Mac app remains pending_auth, and there is no server callback that later marks it active; despite the new device-bound readiness exemption, this status check still creates the connection as pending_auth. Due-feed materialization only considers connections.status = 'active' (packages/server/src/scheduled/check-due-feeds.ts), so feeds on these connections never produce runs even though the cookies are available on the device.
Useful? React with 👍 / 👎.
Comment on lines
+103
to
+104
| const userDataDir = getBrowserUserDataDir(ctx.sessionState); | ||
| const session = await openStealthBrowser({ cdpUrl: 'auto', userDataDir }); |
There was a problem hiding this comment.
For “Attach via CDP” profiles, the server now sends the selected endpoint as session_state.cdp_url, but this connector path only reads user_data_dir and otherwise passes cdpUrl: 'auto'. On Macs where the user picked a non-default port or has multiple debuggable Chromium instances, the sync can attach to the wrong browser or fail auto-discovery instead of using the profile the user just created; the other modified browser connectors have the same pattern.
Useful? React with 👍 / 👎.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds device-bound browser authentication on macOS with managed Chromium profiles, introduces local folder persistence and reconciliation, extends the connector SDK to support persistent browser sessions, updates connectors to use session state for browser configuration, and adds corresponding server APIs for device-scoped profile and feed management. ChangesmacOS app: local folders and browser profiles
Server: device-bound browser auth and feeds
Connector SDK: persistent browser profile support
Connector implementations: persistent browser profiles
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
6 tasks
Three findings: * manage_connections action='create': a device-bound browser_session profile is `pending_auth` until the user logs in via the managed Chrome, which used to propagate to the connection. Result: connection stuck in pending_auth → materializeDueFeeds filters to status='active' → feeds never produce runs. Cookies live on disk on the device, so a run is perfectly capable of executing — mark the connection active and let the first sync surface any "logged out" error. * manage_connections action='connect': hasReadySelection inspected only the server-side auth_data via getBrowserSessionReadiness, which is empty for device-bound profiles. Combined with the existing !isDeviceBoundBrowserSessionConnect exemption in needsBrowserAuth, the guard below rejected with "Select or create a browser auth profile" for exactly the profile the Mac app just created. Exempt device-bound profiles from the readiness probe and treat them as ready outright. * Connectors with a stored CDP url (capterra, g2, glassdoor, trustpilot + linkedin, x, revolut) ignored it and passed `cdpUrl: 'auto'`. On Macs with several debuggable Chromiums or a non-default port the sync could attach to the wrong browser. New helper getBrowserCdpUrl reads it from session_state; every browser connector now prefers the stored endpoint and falls back to 'auto' only when none is set.
Member
Author
|
Addressed all three codex findings in 4f0cad3:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/tools/admin/manage_connections.ts (1)
1692-1699:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the device reassignment atomic with the rest of the update.
This second
UPDATEcan fail onidx_connections_org_connector_device_liveor the FK after the firstUPDATEhas already committed status/auth/config changes. The request then errors out after partially applying the mutation.Safer shape
- updated = await sql` - UPDATE connections - SET display_name = COALESCE(${args.display_name ?? null}, display_name), - slug = COALESCE(${nextSlug}, slug), - status = COALESCE(${effectiveStatus}, status), - auth_profile_id = ${nextAuthProfileId}, - app_auth_profile_id = ${nextAppAuthProfileId}, - config = ${...}, - updated_at = NOW() - WHERE id = ${args.connection_id} AND organization_id = ${organizationId} AND deleted_at IS NULL - RETURNING * - `; + updated = await sql.begin(async (tx) => { + const rows = await tx` + UPDATE connections + SET display_name = COALESCE(${args.display_name ?? null}, display_name), + slug = COALESCE(${nextSlug}, slug), + status = COALESCE(${effectiveStatus}, status), + auth_profile_id = ${nextAuthProfileId}, + app_auth_profile_id = ${nextAppAuthProfileId}, + device_worker_id = ${hasDeviceWorkerArg || updateProfileDeviceWorkerId ? nextDeviceWorkerId : sql`device_worker_id`}, + config = ${...}, + updated_at = NOW() + WHERE id = ${args.connection_id} AND organization_id = ${organizationId} AND deleted_at IS NULL + RETURNING * + `; + return rows; + }); @@ - if (hasDeviceWorkerArg || (updateProfileDeviceWorkerId && !hasDeviceWorkerArg)) { - await sql` - UPDATE connections - SET device_worker_id = ${nextDeviceWorkerId}, updated_at = NOW() - WHERE id = ${args.connection_id} AND organization_id = ${organizationId} - `; - (updated[0] as Record<string, unknown>).device_worker_id = nextDeviceWorkerId; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/tools/admin/manage_connections.ts` around lines 1692 - 1699, The device_worker_id reassignment is done in a separate UPDATE which can commit while earlier updates roll back on constraint/index failure; make the device reassignment atomic with the rest of the update by including device_worker_id in the same SQL UPDATE that modifies status/auth/config (or by wrapping both updates in the same transaction) so they succeed or fail together; adjust the UPDATE that currently writes other fields (the one that produces updated[0]) to also set device_worker_id = ${nextDeviceWorkerId} when hasDeviceWorkerArg || (updateProfileDeviceWorkerId && !hasDeviceWorkerArg) and remove the separate UPDATE and the standalone assignment to (updated[0] as Record<string, unknown>).device_worker_id.
🧹 Nitpick comments (2)
packages/connectors/src/local_directory.ts (1)
46-57: ⚡ Quick winTighten
folder_idvalidation to the deterministic ID format.Current constraints allow arbitrary strings and the description mentions UUID. If this value is deterministic SHA-256-based, enforce that format so bad IDs fail at config validation time rather than during sync/reconcile.
✅ Suggested schema adjustment
folder_id: { type: 'string', - minLength: 8, - maxLength: 64, - description: 'Opaque stable id (UUID) minted on the Mac. Maps to a security-scoped bookmark stored locally on the device.', + minLength: 64, + maxLength: 64, + pattern: '^[a-f0-9]{64}$', + description: 'Opaque stable SHA-256 id minted on the Mac. Maps to a security-scoped bookmark stored locally on the device.', },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/connectors/src/local_directory.ts` around lines 46 - 57, The schema for folder_id currently allows arbitrary strings; tighten validation in the local_directory schema by replacing the loose minLength/maxLength for folder_id with a strict pattern that matches a deterministic SHA-256 hex ID (64 hex chars) — e.g., add a "pattern" like "^[a-f0-9]{64}$" (or "^[A-Fa-f0-9]{64}$" if case-insensitive) and update the description to say "Deterministic SHA-256 hex id (64 hex chars) minted on the Mac" so invalid IDs fail schema validation; modify the folder_id property in the same object where folder_id is defined to implement this change.apps/mac/Lobu/MenuBarContent.swift (1)
553-553: ⚡ Quick winGate browser-profile loading on disclosure expansion.
.taskruns when the menu renders, so this fetch happens even if Integrations stays collapsed. Tie the load tointegrationsExpandedso the popover doesn't pay that network cost up front.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mac/Lobu/MenuBarContent.swift` at line 553, The current `.task { await browserHub.loadIfNeeded(state: state) }` runs on render and triggers loading even when Integrations is collapsed; gate this call behind the `integrationsExpanded` state so loading only happens when the disclosure is opened. Replace the unconditional `.task` with a conditional that runs the load only when `integrationsExpanded` becomes true (for example, use `.task { if integrationsExpanded { await browserHub.loadIfNeeded(state: state) } }` or use `.onChange(of: integrationsExpanded)`/a `Task` started when `integrationsExpanded` transitions to true) so `browserHub.loadIfNeeded(state:)` is invoked only on expansion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mac/Lobu/AppState.swift`:
- Around line 452-459: The guard that skips reconcileFolderFeeds() when
localFolders is empty can leave orphan server feeds if folders were removed
before feedId was learned; remove the conditional check and always call await
reconcileFolderFeeds() (or explicitly call it when localFolders.isEmpty) so the
folder/feed reconciliation runs even with an empty localFolders collection,
ensuring any stale server-side feeds are cleaned up; update the code around the
localFolders check to unconditionally invoke reconcileFolderFeeds() (referencing
localFolders and reconcileFolderFeeds()) and keep the await to preserve async
behavior.
In `@apps/mac/Lobu/BrowserProfileManager.swift`:
- Around line 143-150: The launchManaged function currently ignores
NSWorkspace.openApplication(at:configuration:completionHandler:) errors, so
modify InstalledBrowser.launchManaged to actually throw launch failures by
wrapping the openApplication call in withCheckedThrowingContinuation (or
withUnsafeThrowingContinuation) and resume either returning on success or
throwing the completion handler's Error; specifically, replace the
fire-and-forget call in launchManaged (the
NSWorkspace.shared.openApplication(at: target, configuration: config) { _, _ in
}) with a continuation that inspects the completion handler's error parameter
and calls continuation.resume(throwing: error) when non-nil or
continuation.resume() on success so callers receive the failure instead of it
being swallowed.
In `@apps/mac/Lobu/BrowserProfilesView.swift`:
- Around line 14-17: loadIfNeeded currently sets the loaded flag regardless of
reload outcome; change it so loaded is only set to true on a successful reload:
call await reload(state:) inside a do/catch (or inspect its returned success
value) and set loaded = true only when reload completes without
throwing/indicating failure, leaving loaded false on errors so future calls will
retry; reference the loadIfNeeded(state:), reload(state:), and loaded symbols
when making the change.
- Around line 316-324: The materialized copy created by
BrowserProfileManager.materializeManagedProfile (assigned to target) is left
behind if client.createMyBrowserAuthProfile throws; wrap the API call in a
do/catch (or try/await with defer) so that on failure you remove the
materialized directory (e.g., FileManager.default.removeItem at target.path) and
rethrow the error, ensuring you only delete on error and not after successful
creation; apply the same pattern for the other occurrence around the second
createMyBrowserAuthProfile call (lines 341-343) so both paths clean up the
temporary profile directory on failure.
In `@db/migrations/20260513150000_auth_profiles_cdp_url.sql`:
- Around line 3-13: Add a DB CHECK constraint on table auth_profiles to enforce
the device-bound invariant: when a row represents a device-bound browser session
(profile_type = 'browser_session' or whatever column/flag you use to mark
device-bound profiles) exactly one of the columns user_data_dir and cdp_url must
be non-null. Implement this by ALTER TABLE auth_profiles ADD CONSTRAINT (e.g.,
auth_profiles_device_bound_xor_chk) that conditionally checks (profile_type <>
'browser_session' OR ((user_data_dir IS NULL) <> (cdp_url IS NULL))) so
non-device-bound rows are unaffected and device-bound rows require an exclusive
OR between user_data_dir and cdp_url.
In `@db/schema.sql`:
- Around line 250-256: Add a CHECK constraint (e.g.,
auth_profiles_browser_session_xor_check) on the same table that enforces the
device-bound browser-session XOR invariant: when profile_kind =
'browser_session' and device_worker_id IS NOT NULL, exactly one of user_data_dir
and cdp_url must be non-NULL (i.e., forbid both NULL and both non-NULL). Update
the table definition to include this CHECK referencing the existing columns
profile_kind, device_worker_id, user_data_dir and cdp_url so the DB, not
application code, enforces copy-vs-attach semantics.
In `@packages/connector-sdk/src/browser-network.ts`:
- Around line 76-103: The persistent-browser path can leak an open
BrowserContext if operations after launchPersistentContext (e.g.,
context.addCookies, context.pages()/context.newPage) throw; wrap the post-launch
operations in a try/catch/finally and ensure the context is closed on any
failure: call launchPersistentContext(...) to obtain context, then perform
addCookies and page creation inside a try block, and in the catch/finally call
context.close() (or context.browser()?.close()) when an exception occurs before
returning; update the return path to only return after successful post-launch
steps and ensure ownsBrowser remains correct when closing on error.
In `@packages/connector-sdk/src/browser/acquire.ts`:
- Around line 157-185: The acquireViaPersistent function can leak the persistent
Playwright context if addCookies or newPage throws after
launchPersistentContext; wrap the post-launch operations (calls to
context.addCookies and context.newPage and any other work after obtaining
context) in a try/catch/finally or try/catch that on error calls await
context.close() (or context?.close()) before rethrowing so the persistent
browser is cleaned up; specifically update acquireViaPersistent to close the
launched context when any exception occurs after launchPersistentContext to
avoid leaving processes running.
In `@packages/connectors/src/linkedin.ts`:
- Around line 324-331: Summary: Validation currently enforces the server-side
li_at cookie when userDataDir is absent, which breaks explicit CDP attach mode;
change the condition to skip validation when an explicit CDP URL is provided.
Fix: in the block that computes cdpUrl, userDataDir, and cookies (using
getBrowserCdpUrl, getBrowserUserDataDir, getBrowserCookies), only call
validateCookieNotExpired('li_at', 'linkedin') when userDataDir is falsy AND
cdpUrl is 'auto' (i.e., not an explicit attach). Update the conditional from "if
(!userDataDir) { validateCookieNotExpired(...)}" to "if (!userDataDir && cdpUrl
=== 'auto') { validateCookieNotExpired(...)}" so device-bound CDP profiles that
attach a browser are not incorrectly validated.
In `@packages/server/src/db/embedded-schema-patches.ts`:
- Around line 345-354: The embedded patch with id 'auth-profiles-cdp-url'
currently only adds the cdp_url column but must also enforce the device-bound
XOR invariant; update the apply function in that patch to run an additional SQL
ALTER TABLE that adds a CHECK constraint (use IF NOT EXISTS with a unique name
like auth_profiles_device_bound_xor_check) ensuring that either device_worker_id
IS NULL OR exactly one of user_data_dir and cdp_url is non-null (e.g. CHECK
(device_worker_id IS NULL OR ((user_data_dir IS NOT NULL)::int + (cdp_url IS NOT
NULL)::int = 1))); include this ALTER TABLE in the same apply async (sql) block
so embedded/PGlite will reject invalid auth_profile states.
In `@packages/server/src/tools/admin/manage_connections.ts`:
- Around line 1219-1233: The pending-row lookup and duplicate-check logic are
still using deviceBinding.deviceWorkerId instead of the computed
effectiveDeviceWorkerIdConnect (which accounts for
authSelection.authProfile?.device_worker_id via profileDeviceWorkerIdConnect),
causing missed matches and unique-index insert errors; update any
queries/conditions that reference deviceBinding.deviceWorkerId in the connect
flow (the pending-row lookup and duplicate check code paths) to use
effectiveDeviceWorkerIdConnect (and keep the existing early-return that compares
effectiveDeviceWorkerIdConnect vs profileDeviceWorkerIdConnect) so idempotency
works when the device id is inherited from the profile.
- Around line 996-1008: After inheriting the profile's device into
effectiveDeviceWorkerId (using profileDeviceWorkerId), re-run the per-device
duplicate guard that earlier checks for an existing (org, connector, device) row
so we surface the friendly validation error instead of a DB unique-index
failure; specifically, after the block that sets effectiveDeviceWorkerId (and
the else-if that returns on mismatch), invoke the same duplicate-check logic
used earlier (the query/guard that tests for an existing connection with the
org_id, connector_id and effectiveDeviceWorkerId) and return the same validation
error path when a duplicate is found.
In `@packages/server/src/worker-api.ts`:
- Around line 1981-1990: The INSERT currently always creates a new feed
(variable inserted) allowing duplicate active feeds for the same deterministic
folder; make the operation idempotent by turning the INSERT into an UPSERT keyed
on the deterministic unique column (the feed key / folder identifier) so retries
or concurrent runs return the existing row instead of inserting another. Replace
the current INSERT INTO ... VALUES ... RETURNING block inside the code that sets
inserted with an INSERT ... ON CONFLICT (feed_key) DO UPDATE (or DO NOTHING)
strategy that returns the existing/updated row (e.g. ON CONFLICT (feed_key) DO
UPDATE SET status='active', config=COALESCE(EXCLUDED.config, feeds.config),
next_run_at=NOW() RETURNING ...), and if the INSERT returns no row then SELECT
the row by feed_key; update references to the inserted variable accordingly so
downstream code uses the single canonical feed row.
- Around line 1742-1745: The auth-profile list SQL query that populates the
variable rows (const rows = (await sql`SELECT id, slug, display_name,
connector_key, profile_kind, status, browser_kind, user_data_dir, created_at,
updated_at FROM auth_profiles`)) omits cdp_url, causing attach-mode profiles to
lose their CDP-backed flag; update that SELECT to also include cdp_url and
ensure any response mapping that reads rows (where the auth profile objects are
constructed) passes the returned cdp_url through so the API response contains
cdp_url for each profile.
In `@packages/web`:
- Line 1: The submodule commit pointer for the packages/web submodule references
a commit id that doesn't exist on the remote; update the submodule reference by
either pushing the missing commit to the submodule's remote or changing the
submodule pointer to a reachable commit and committing that change.
Specifically, open the packages/web submodule (or .gitmodules and the submodule
folder), fetch the latest refs from its remote, and then either (a) push the
local commit ca173b3a4b65f97f4a126f564d88df3d80839110 to the submodule remote,
or (b) run git checkout <existing-commit-or-branch> inside the submodule and
update the parent repo’s pointer (git add packages/web; git commit) so the
parent no longer references the inaccessible commit. Ensure the new commit is
reachable from the submodule remote before merging.
---
Outside diff comments:
In `@packages/server/src/tools/admin/manage_connections.ts`:
- Around line 1692-1699: The device_worker_id reassignment is done in a separate
UPDATE which can commit while earlier updates roll back on constraint/index
failure; make the device reassignment atomic with the rest of the update by
including device_worker_id in the same SQL UPDATE that modifies
status/auth/config (or by wrapping both updates in the same transaction) so they
succeed or fail together; adjust the UPDATE that currently writes other fields
(the one that produces updated[0]) to also set device_worker_id =
${nextDeviceWorkerId} when hasDeviceWorkerArg || (updateProfileDeviceWorkerId &&
!hasDeviceWorkerArg) and remove the separate UPDATE and the standalone
assignment to (updated[0] as Record<string, unknown>).device_worker_id.
---
Nitpick comments:
In `@apps/mac/Lobu/MenuBarContent.swift`:
- Line 553: The current `.task { await browserHub.loadIfNeeded(state: state) }`
runs on render and triggers loading even when Integrations is collapsed; gate
this call behind the `integrationsExpanded` state so loading only happens when
the disclosure is opened. Replace the unconditional `.task` with a conditional
that runs the load only when `integrationsExpanded` becomes true (for example,
use `.task { if integrationsExpanded { await browserHub.loadIfNeeded(state:
state) } }` or use `.onChange(of: integrationsExpanded)`/a `Task` started when
`integrationsExpanded` transitions to true) so `browserHub.loadIfNeeded(state:)`
is invoked only on expansion.
In `@packages/connectors/src/local_directory.ts`:
- Around line 46-57: The schema for folder_id currently allows arbitrary
strings; tighten validation in the local_directory schema by replacing the loose
minLength/maxLength for folder_id with a strict pattern that matches a
deterministic SHA-256 hex ID (64 hex chars) — e.g., add a "pattern" like
"^[a-f0-9]{64}$" (or "^[A-Fa-f0-9]{64}$" if case-insensitive) and update the
description to say "Deterministic SHA-256 hex id (64 hex chars) minted on the
Mac" so invalid IDs fail schema validation; modify the folder_id property in the
same object where folder_id is defined to implement this change.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c745564-ca00-46ef-95d9-bc5a47af098f
📒 Files selected for processing (34)
apps/mac/Lobu.xcodeproj/project.pbxprojapps/mac/Lobu/AppState.swiftapps/mac/Lobu/BrowserProfileManager.swiftapps/mac/Lobu/BrowserProfilesView.swiftapps/mac/Lobu/LobuClient.swiftapps/mac/Lobu/LocalDirectorySyncService.swiftapps/mac/Lobu/MenuBarContent.swiftdb/migrations/20260513120000_auth_profiles_device_binding.sqldb/migrations/20260513150000_auth_profiles_cdp_url.sqldb/schema.sqlpackages/connector-sdk/src/browser-network.tspackages/connector-sdk/src/browser/acquire.tspackages/connector-sdk/src/connector-types.tspackages/connectors/src/browser-scraper-utils.tspackages/connectors/src/capterra.tspackages/connectors/src/g2.tspackages/connectors/src/glassdoor.tspackages/connectors/src/linkedin.tspackages/connectors/src/local_directory.tspackages/connectors/src/revolut.tspackages/connectors/src/trustpilot.tspackages/connectors/src/x.tspackages/server/src/db/embedded-schema-patches.tspackages/server/src/index.tspackages/server/src/tools/admin/helpers/connection-helpers.tspackages/server/src/tools/admin/index.tspackages/server/src/tools/admin/manage_connections.tspackages/server/src/tools/admin/notify.tspackages/server/src/utils/auth-profiles.tspackages/server/src/utils/connector-catalog.tspackages/server/src/utils/execution-context.tspackages/server/src/worker-api.tspackages/server/src/worker-api/device-reconcile.tspackages/web
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
# Conflicts: # packages/web
* manage_connections create/connect: re-run the per-device duplicate- connection guard after inheriting the profile's device. Previously the check ran with the user's explicit device_worker_id only — for a device-bound profile the partial unique index could fire as a raw exception instead of a clean error. * createMyDeviceFeed: idempotent on (connection_id, feed_key, config.folder_id). Two concurrent reconciles no longer create duplicate feeds for the same folder. * listMyDeviceAuthProfiles: include cdp_url so the Mac inbox can tell copy-mode from attach-mode profiles. * Schema CHECK auth_profiles_device_browser_path_xor: a device-bound browser_session profile must set exactly one of (user_data_dir, cdp_url). Migration + embedded patch + schema.sql in lockstep. * AppState.syncNow: always reconcile folder feeds even when localFolders is empty so orphan server feeds get cleaned up when the user removed their last folder before its feed id was learned. * BrowserProfileManager.launchManaged: bridge NSWorkspace.openApplication completion-handler to async-throws. Caller surfaces the error instead of leaving a profile stuck in pending_auth forever. * BrowserProfilesView: clean up the materialized --user-data-dir if the server refuses the auth-profile create, and only mark the hub as loaded after a successful fetch so a transient error doesn't permanently hide the list. * browser-network / acquire persistent paths: wrap post-launch setup in try/catch, close the persistent context on failure so we don't leak a long-lived Chromium process holding the profile lock. * linkedin connector: skip the server-cookie cascade when either user_data_dir or an explicit cdp_url is in session_state. Attach-via-CDP with no stored cookies should not be a hard error. * Submodule packages/web: rebased onto owletto-web/main (Notion-style nav + connectors→connections rename), parent pointer bumped to the new tip.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CI's per-package typecheck (strict noUnusedLocals) caught the stale binding from when user_data_dir was a top-level worker-job field; it now flows via sessionState.user_data_dir set inside resolveExecutionAuth.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mac/Lobu/BrowserProfileManager.swift`:
- Around line 174-179: autoDetectCdpUrl() currently returns the first reachable
CDP port and can accidentally attach to the wrong Chromium when multiple
instances expose CDP; instead, iterate the candidate ports using
isCdpReachable(port:) to build a list of reachable ports, and only return
"http://127.0.0.1:\(port)" when exactly one candidate is reachable; if zero or
more than one are reachable return nil so callers must explicitly select a port.
- Around line 166-167: The removeManagedProfile(at:) currently swallows errors
with try? causing silent failures; change its signature to throw (static func
removeManagedProfile(at:) throws) and replace try?
FileManager.default.removeItem(at: path) with try
FileManager.default.removeItem(at: path) so failures propagate; update all
callers of removeManagedProfile(at:) to either handle the thrown error
(do/catch) or propagate it further, and ensure any UI or logging surfaces the
error to the user or diagnostics.
- Around line 128-136: materializeManagedProfile currently creates the
managedRoot instead of the specific new target and copies the seeded profile
into the user-data root; Chromium expects profile data to live under a profile
folder like "Default". Change materializeManagedProfile to create the new target
directory (use FileManager.default.createDirectory(at: target, ...)), then
create a "Default" subdirectory (let defaultDir =
target.appendingPathComponent("Default", isDirectory: true)), and copy the
contents from source.sourcePath into that Default directory
(FileManager.default.copyItem(at: source.sourcePath, to: defaultDir)); keep
returning target so launchManaged still passes the correct --user-data-dir. Use
the function name materializeManagedProfile and the
InstalledBrowserProfile.sourcePath symbol to locate the code to edit.
In `@db/migrations/20260513150000_auth_profiles_cdp_url.sql`:
- Around line 7-10: The leading block comment in the migration script
20260513150000_auth_profiles_cdp_url.sql is stale and contradicts the migration
(which adds a CHECK constraint); update or remove that comment so it accurately
reflects the migration behavior—either delete the lines describing “we don't add
a CHECK constraint” or replace them with a note that the migration does add a
CHECK constraint for the auth_profiles CDP URL and why (include any OR-on-NULL
rationale if still relevant).
In `@packages/connectors/src/linkedin.ts`:
- Around line 342-345: The connector currently always returns auth_update
containing cookies (e.g., returning auth_update: { cookies: result.cookies } in
the LinkedIn sync paths invoked by methods that call this.syncJobs and
this.syncUpdates), which allows device-bound profiles to leak cookies back to
the server; update the LinkedIn connector so it only returns auth_update when
the profile is a server-stored browser_session (i.e., authProfile.profile_kind
=== 'browser_session' AND authProfile.device_worker_id is falsy). Concretely,
locate the places that build/return auth_update (references: auth_update,
result.cookies, and the methods syncJobs/syncUpdates) and wrap the return so
cookies are included only if !authProfile.device_worker_id (skip returning
auth_update or omit cookies when device_worker_id is set).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2e3ef08-615e-4a5b-b1bd-9450c21a4f2f
📒 Files selected for processing (12)
apps/mac/Lobu/AppState.swiftapps/mac/Lobu/BrowserProfileManager.swiftapps/mac/Lobu/BrowserProfilesView.swiftdb/migrations/20260513150000_auth_profiles_cdp_url.sqldb/schema.sqlpackages/connector-sdk/src/browser-network.tspackages/connector-sdk/src/browser/acquire.tspackages/connectors/src/linkedin.tspackages/server/src/db/embedded-schema-patches.tspackages/server/src/tools/admin/manage_connections.tspackages/server/src/worker-api.tspackages/web
✅ Files skipped from review due to trivial changes (1)
- packages/web
🚧 Files skipped from review as they are similar to previous changes (7)
- db/schema.sql
- packages/connector-sdk/src/browser-network.ts
- packages/connector-sdk/src/browser/acquire.ts
- apps/mac/Lobu/BrowserProfilesView.swift
- apps/mac/Lobu/AppState.swift
- packages/server/src/tools/admin/manage_connections.ts
- packages/server/src/worker-api.ts
Comment on lines
+128
to
+136
| static func materializeManagedProfile(from source: InstalledBrowserProfile, named name: String) throws -> URL { | ||
| let dirName = "\(source.browser.kind.rawValue)-\(slugify(name))-\(UUID().uuidString.prefix(8))" | ||
| let target = managedRoot.appendingPathComponent(dirName, isDirectory: true) | ||
| try FileManager.default.createDirectory(at: managedRoot, withIntermediateDirectories: true) | ||
| // Copy the full source profile dir. For a fresh-blank profile, callers | ||
| // can skip this and just createDirectory(target) — but most users want | ||
| // to inherit their existing cookies. | ||
| try FileManager.default.copyItem(at: source.sourcePath, to: target) | ||
| return target |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Chromium/Chrome and Playwright launchPersistentContext, does --user-data-dir / userDataDir need to point to the browser's root user data directory containing a Default/Profile N subfolder, rather than directly to a single profile directory?
💡 Result:
Yes. For Chromium/Chrome with Playwright’s chromium/BrowserType.launchPersistentContext(userDataDir), the userDataDir must point to Chrome/Chromium’s User Data directory (the “root” that contains profile subfolders like “Default” or “Profile 1”), not directly to a single profile directory. Evidence: - Playwright’s launchPersistentContext “uses persistent storage located at userDataDir” and notes that Chromium’s user data directory is the parent directory of the “Profile Path” shown at chrome://version, meaning userDataDir is the parent/root that contains the profile directory underneath. [1] - Chromium documentation states “Each profile is a subdirectory (often Default) within the user data directory,” and that the --user-data-dir flag overrides the user data directory. [2] - A Playwright issue discussion also restates that “Docs say that Chromium’s user data directory is the parent directory of the ‘Profile Path’ seen at chrome://version.” [3] - Users commonly hit problems when they try to pass a profile folder (e.g., “User Data/Profile 1”) as userDataDir; one workaround is to use the root User Data dir and pass --profile-directory=Profile 1 via args. [4] Practical rule: - Pass.../Google Chrome/User Data (root “User Data” directory) to launchPersistentContext. - If you need a specific profile inside it, also pass chrome args like --profile-directory=Default or --profile-directory=Profile 2 (not a path). [4][2]
Citations:
- 1: https://playwright.dev/docs/api/class-browsertype
- 2: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/user_data_dir.md
- 3: [Question] How to use launchPersistentContext properly to open a non-incognito tab? microsoft/playwright#13878
- 4: https://stackoverflow.com/questions/74951776/python-playwright-wont-let-me-open-user-profile-other-than-default-profile
🏁 Script executed:
cat -n apps/mac/Lobu/BrowserProfileManager.swift | head -150Repository: lobu-ai/lobu
Length of output: 8677
🏁 Script executed:
cat -n apps/mac/Lobu/BrowserProfileManager.swift | sed -n '144,200p'Repository: lobu-ai/lobu
Length of output: 3020
Copy the seeded profile into Default, not the user-data root.
This method returns a path that launchManaged passes as --user-data-dir, but it copies the selected profile directly into that root. Chromium expects profile subdirectories (like Default) within the user-data root and will ignore cookies placed at the root level, causing copy-mode profiles to start blank instead of inheriting the user's cookies.
🛠️ Minimal fix
static func materializeManagedProfile(from source: InstalledBrowserProfile, named name: String) throws -> URL {
let dirName = "\(source.browser.kind.rawValue)-\(slugify(name))-\(UUID().uuidString.prefix(8))"
- let target = managedRoot.appendingPathComponent(dirName, isDirectory: true)
+ let targetRoot = managedRoot.appendingPathComponent(dirName, isDirectory: true)
+ let targetProfile = targetRoot.appendingPathComponent("Default", isDirectory: true)
try FileManager.default.createDirectory(at: managedRoot, withIntermediateDirectories: true)
- // Copy the full source profile dir. For a fresh-blank profile, callers
- // can skip this and just createDirectory(target) — but most users want
- // to inherit their existing cookies.
- try FileManager.default.copyItem(at: source.sourcePath, to: target)
- return target
+ try FileManager.default.createDirectory(at: targetRoot, withIntermediateDirectories: false)
+ // Copy the selected source profile into the managed root's default profile.
+ try FileManager.default.copyItem(at: source.sourcePath, to: targetProfile)
+ return targetRoot
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mac/Lobu/BrowserProfileManager.swift` around lines 128 - 136,
materializeManagedProfile currently creates the managedRoot instead of the
specific new target and copies the seeded profile into the user-data root;
Chromium expects profile data to live under a profile folder like "Default".
Change materializeManagedProfile to create the new target directory (use
FileManager.default.createDirectory(at: target, ...)), then create a "Default"
subdirectory (let defaultDir = target.appendingPathComponent("Default",
isDirectory: true)), and copy the contents from source.sourcePath into that
Default directory (FileManager.default.copyItem(at: source.sourcePath, to:
defaultDir)); keep returning target so launchManaged still passes the correct
--user-data-dir. Use the function name materializeManagedProfile and the
InstalledBrowserProfile.sourcePath symbol to locate the code to edit.
Comment on lines
+166
to
+167
| static func removeManagedProfile(at path: URL) { | ||
| try? FileManager.default.removeItem(at: path) |
There was a problem hiding this comment.
Surface managed-profile deletion failures.
If the browser still has files open, removeItem can fail and the auth profile can disappear from the app while its cookies remain on disk. This should throw or otherwise report failure to the caller.
🛠️ Minimal fix
-static func removeManagedProfile(at path: URL) {
- try? FileManager.default.removeItem(at: path)
+static func removeManagedProfile(at path: URL) throws {
+ try FileManager.default.removeItem(at: path)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mac/Lobu/BrowserProfileManager.swift` around lines 166 - 167, The
removeManagedProfile(at:) currently swallows errors with try? causing silent
failures; change its signature to throw (static func removeManagedProfile(at:)
throws) and replace try? FileManager.default.removeItem(at: path) with try
FileManager.default.removeItem(at: path) so failures propagate; update all
callers of removeManagedProfile(at:) to either handle the thrown error
(do/catch) or propagate it further, and ensure any UI or logging surfaces the
error to the user or diagnostics.
Comment on lines
+174
to
+179
| static func autoDetectCdpUrl() async -> String? { | ||
| for port in [9222, 9223, 9224, 9225] { | ||
| if await isCdpReachable(port: port) { | ||
| return "http://127.0.0.1:\(port)" | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't auto-bind attach mode to the first open CDP port.
If more than one Chromium instance is exposing CDP, this silently attaches the selected browser row to whichever port answers first. Only auto-fill when exactly one candidate is reachable; otherwise require explicit input.
🛠️ Minimal fix
static func autoDetectCdpUrl() async -> String? {
- for port in [9222, 9223, 9224, 9225] {
- if await isCdpReachable(port: port) {
- return "http://127.0.0.1:\(port)"
- }
- }
- return nil
+ var reachablePorts: [Int] = []
+ for port in [9222, 9223, 9224, 9225] {
+ if await isCdpReachable(port: port) {
+ reachablePorts.append(port)
+ }
+ }
+ guard reachablePorts.count == 1 else { return nil }
+ return "http://127.0.0.1:\(reachablePorts[0])"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static func autoDetectCdpUrl() async -> String? { | |
| for port in [9222, 9223, 9224, 9225] { | |
| if await isCdpReachable(port: port) { | |
| return "http://127.0.0.1:\(port)" | |
| } | |
| } | |
| static func autoDetectCdpUrl() async -> String? { | |
| var reachablePorts: [Int] = [] | |
| for port in [9222, 9223, 9224, 9225] { | |
| if await isCdpReachable(port: port) { | |
| reachablePorts.append(port) | |
| } | |
| } | |
| guard reachablePorts.count == 1 else { return nil } | |
| return "http://127.0.0.1:\(reachablePorts[0])" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mac/Lobu/BrowserProfileManager.swift` around lines 174 - 179,
autoDetectCdpUrl() currently returns the first reachable CDP port and can
accidentally attach to the wrong Chromium when multiple instances expose CDP;
instead, iterate the candidate ports using isCdpReachable(port:) to build a list
of reachable ports, and only return "http://127.0.0.1:\(port)" when exactly one
candidate is reachable; if zero or more than one are reachable return nil so
callers must explicitly select a port.
Comment on lines
+7
to
+10
| -- The application enforces this invariant; we don't add a CHECK constraint | ||
| -- because the OR-on-NULL semantics are awkward to express and the column | ||
| -- is harmless when both are NULL (legacy fleet path with cookies in | ||
| -- auth_data jsonb). |
There was a problem hiding this comment.
Remove stale comment about skipping DB CHECK enforcement.
This note now conflicts with the migration behavior below, which does add the CHECK constraint. Keeping it will mislead future maintenance/debugging.
Suggested edit
--- a/db/migrations/20260513150000_auth_profiles_cdp_url.sql
+++ b/db/migrations/20260513150000_auth_profiles_cdp_url.sql
@@
--- The application enforces this invariant; we don't add a CHECK constraint
--- because the OR-on-NULL semantics are awkward to express and the column
--- is harmless when both are NULL (legacy fleet path with cookies in
--- auth_data jsonb).
+-- This migration also enforces the invariant with a DB CHECK constraint
+-- for device-bound browser_session profiles.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| -- The application enforces this invariant; we don't add a CHECK constraint | |
| -- because the OR-on-NULL semantics are awkward to express and the column | |
| -- is harmless when both are NULL (legacy fleet path with cookies in | |
| -- auth_data jsonb). | |
| -- This migration also enforces the invariant with a DB CHECK constraint | |
| -- for device-bound browser_session profiles. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/migrations/20260513150000_auth_profiles_cdp_url.sql` around lines 7 - 10,
The leading block comment in the migration script
20260513150000_auth_profiles_cdp_url.sql is stale and contradicts the migration
(which adds a CHECK constraint); update or remove that comment so it accurately
reflects the migration behavior—either delete the lines describing “we don't add
a CHECK constraint” or replace them with a note that the migration does add a
CHECK constraint for the auth_profiles CDP URL and why (include any OR-on-NULL
rationale if still relevant).
Comment on lines
+342
to
+345
| return this.syncJobs(baseUrl, cookies, maxScrolls, checkpoint, userDataDir, cdpUrl); | ||
| } | ||
|
|
||
| return this.syncUpdates(baseUrl, cookies, maxScrolls, checkpoint); | ||
| return this.syncUpdates(baseUrl, cookies, maxScrolls, checkpoint, userDataDir, cdpUrl); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== browserNetworkSync cookie handling =="
fd -i 'browser-network.ts' packages | while read -r file; do
echo "--- $file ---"
rg -n -C3 'result\.cookies|cookies:' "$file"
done
echo
echo "== auth_update persistence paths =="
rg -n -C4 '\bauth_update\b|device_worker_id|user_data_dir|cdp_url' packagesRepository: lobu-ai/lobu
Length of output: 50368
🏁 Script executed:
#!/bin/bash
# Check the syncJobs and syncUpdates methods in LinkedIn connector
echo "=== LinkedIn syncJobs and syncUpdates methods ==="
rg -n -A20 'syncJobs|syncUpdates' packages/connectors/src/linkedin.ts | head -100
# Check where auth_update is returned from LinkedIn
echo
echo "=== auth_update construction in LinkedIn connector ==="
rg -n -B5 -A5 'auth_update' packages/connectors/src/linkedin.ts
# Check browserNetworkSync usage in LinkedIn
echo
echo "=== How browserNetworkSync result is used ==="
rg -n -B3 -A3 'browserNetworkSync' packages/connectors/src/linkedin.tsRepository: lobu-ai/lobu
Length of output: 3603
Add device_worker_id check to auth_update persistence for browser_session profiles.
The auth_update handler at packages/server/src/worker-api.ts (lines 828–835) persists cookies for any browser_session profile, but device-bound profiles should never store cookies on the server. Add a guard to skip auth_update persistence when authProfile.device_worker_id is set:
if (authProfile?.profile_kind === 'browser_session' && !authProfile.device_worker_id) {
const nextAuthData = {
...(authProfile.auth_data ?? {}),
...req.auth_update,
};
// ...
}
Connectors like LinkedIn (lines 342–345, 411, 483) unconditionally return auth_update: { cookies: result.cookies } for both device-bound and server-bound syncs. Without this guard, device-bound sessions leak cookies back to the server, defeating the isolated auth model documented in POST /api/workers/me/auth-profiles (line 1762: "Cookies stay on the device — server's auth_data is empty").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/connectors/src/linkedin.ts` around lines 342 - 345, The connector
currently always returns auth_update containing cookies (e.g., returning
auth_update: { cookies: result.cookies } in the LinkedIn sync paths invoked by
methods that call this.syncJobs and this.syncUpdates), which allows device-bound
profiles to leak cookies back to the server; update the LinkedIn connector so it
only returns auth_update when the profile is a server-stored browser_session
(i.e., authProfile.profile_kind === 'browser_session' AND
authProfile.device_worker_id is falsy). Concretely, locate the places that
build/return auth_update (references: auth_update, result.cookies, and the
methods syncJobs/syncUpdates) and wrap the return so cookies are included only
if !authProfile.device_worker_id (skip returning auth_update or omit cookies
when device_worker_id is set).
* packages/web → 965ec489 (owletto-web#102 squash-merged into main). Companion PR that surfaces device-bound browser auth profiles in the connection picker is now on main, so check-drift sees a reachable SHA. * oauth-utils.test.ts imported `bun:test` and ran under vitest in the CI integration suite, failing the whole suite with "Failed to load url bun:test". Switched the import to vitest — 36 tests pass locally; unblocks the integration check on both this PR and #707.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/web (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSubmodule SHA is not reachable from
owletto-web/main(CI hard blocker).At Line 1, the pinned submodule commit fails the drift check (
merge-base --is-ancestor), so this PR cannot pass policy/CI as-is. Repointpackages/webto a commit that is reachable fromowletto-web/main(or merge this SHA into that branch first), then update the parent pointer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web` at line 1, The pinned submodule commit for packages/web is not an ancestor of owletto-web/main so CI fails; either update the packages/web submodule pointer to a commit that is reachable from owletto-web/main or merge the current submodule SHA into owletto-web/main and then update the pointer. To fix: fetch the remote in the submodule, checkout or reset the submodule to a commit that exists on owletto-web/main (or merge the missing SHA into owletto-web/main), stage the updated packages/web pointer (git add packages/web) and commit the change to the parent repo so the submodule SHA points to a reachable commit; ensure the commit message references packages/web and owletto-web/main.packages/server/src/worker-api.ts (1)
1984-2017:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe pre-insert probe still doesn't make folder feed creation idempotent.
Two concurrent reconciles can both observe “no existing row” and both hit the
INSERT, so this still permits duplicate active feeds for the same deterministicfolder_id. The one-feed-per-folder invariant needs a database-enforced unique key plus an UPSERT (or equivalent locking), not a best-effortSELECTfirst.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/worker-api.ts` around lines 1984 - 2017, The SELECT probe around folderIdInConfig is racy and won't prevent duplicate feeds; enforce uniqueness in the DB and switch the INSERT into an idempotent upsert: create a unique partial index/constraint on feeds for (connection_id, feed_key, (config->>'folder_id')) WHERE deleted_at IS NULL (or a named unique constraint) and replace the INSERT INTO feeds ... RETURNING ... with an INSERT ... ON CONFLICT ON CONSTRAINT <your_constraint_name> DO NOTHING (or DO UPDATE to return the existing row) then fetch/return the existing row when a conflict occurs; refer to the folderIdInConfig variable, the INSERT INTO feeds statement, and the config->>'folder_id' expression when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/worker-api.ts`:
- Around line 1704-1726: resolveDeviceWorkerForRequest currently only validates
(user_id, worker_id) but must also enforce that the device's organization_id is
one of the worker-scoped org IDs in c.var.workerOrgIds; after fetching row in
resolveDeviceWorkerForRequest (the row variable from device_workers) add a check
that row.organization_id is included in c.var.workerOrgIds (treat
c.var.workerOrgIds as the allowed org ID list) and if not return an error
response (e.g. c.json({ error: 'Forbidden' }, 403)); keep the existing
null/empty organization_id check but perform the scope inclusion check before
returning the device object.
---
Duplicate comments:
In `@packages/server/src/worker-api.ts`:
- Around line 1984-2017: The SELECT probe around folderIdInConfig is racy and
won't prevent duplicate feeds; enforce uniqueness in the DB and switch the
INSERT into an idempotent upsert: create a unique partial index/constraint on
feeds for (connection_id, feed_key, (config->>'folder_id')) WHERE deleted_at IS
NULL (or a named unique constraint) and replace the INSERT INTO feeds ...
RETURNING ... with an INSERT ... ON CONFLICT ON CONSTRAINT
<your_constraint_name> DO NOTHING (or DO UPDATE to return the existing row) then
fetch/return the existing row when a conflict occurs; refer to the
folderIdInConfig variable, the INSERT INTO feeds statement, and the
config->>'folder_id' expression when making these changes.
In `@packages/web`:
- Line 1: The pinned submodule commit for packages/web is not an ancestor of
owletto-web/main so CI fails; either update the packages/web submodule pointer
to a commit that is reachable from owletto-web/main or merge the current
submodule SHA into owletto-web/main and then update the pointer. To fix: fetch
the remote in the submodule, checkout or reset the submodule to a commit that
exists on owletto-web/main (or merge the missing SHA into owletto-web/main),
stage the updated packages/web pointer (git add packages/web) and commit the
change to the parent repo so the submodule SHA points to a reachable commit;
ensure the commit message references packages/web and owletto-web/main.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10d5792b-f07e-427d-a99d-8aa4f73fb80d
📒 Files selected for processing (3)
packages/server/src/auth/__tests__/oauth-utils.test.tspackages/server/src/worker-api.tspackages/web
✅ Files skipped from review due to trivial changes (1)
- packages/server/src/auth/tests/oauth-utils.test.ts
Comment on lines
+1704
to
+1726
| async function resolveDeviceWorkerForRequest( | ||
| c: Context<{ Bindings: Env }>, | ||
| workerId: string | ||
| ): Promise<{ device: { id: string; organization_id: string } | null; error?: Response }> { | ||
| const userId = c.var.workerUserId; | ||
| if (!userId) { | ||
| return { device: null, error: c.json({ error: 'Unauthorized' }, 401) }; | ||
| } | ||
| const sql = getDb(); | ||
| const rows = (await sql` | ||
| SELECT id, organization_id | ||
| FROM device_workers | ||
| WHERE user_id = ${userId} AND worker_id = ${workerId} | ||
| LIMIT 1 | ||
| `) as unknown as Array<{ id: string; organization_id: string | null }>; | ||
| const row = rows[0]; | ||
| if (!row) { | ||
| return { device: null, error: c.json({ error: 'Device not registered yet — poll first' }, 404) }; | ||
| } | ||
| if (!row.organization_id) { | ||
| return { device: null, error: c.json({ error: 'Device has no organization attached' }, 409) }; | ||
| } | ||
| return { device: { id: row.id, organization_id: row.organization_id } }; |
There was a problem hiding this comment.
Enforce worker-token org scope before returning a device.
resolveDeviceWorkerForRequest() only checks (user_id, worker_id). A worker JWT issued for workspace A can still pass the worker_id of the same user's device attached to workspace B and then use these /api/workers/me/* endpoints against B. Please reject devices whose organization_id is not in c.var.workerOrgIds.
Suggested fix
async function resolveDeviceWorkerForRequest(
c: Context<{ Bindings: Env }>,
workerId: string
): Promise<{ device: { id: string; organization_id: string } | null; error?: Response }> {
const userId = c.var.workerUserId;
if (!userId) {
return { device: null, error: c.json({ error: 'Unauthorized' }, 401) };
}
const sql = getDb();
const rows = (await sql`
SELECT id, organization_id
FROM device_workers
WHERE user_id = ${userId} AND worker_id = ${workerId}
LIMIT 1
`) as unknown as Array<{ id: string; organization_id: string | null }>;
const row = rows[0];
if (!row) {
return { device: null, error: c.json({ error: 'Device not registered yet — poll first' }, 404) };
}
if (!row.organization_id) {
return { device: null, error: c.json({ error: 'Device has no organization attached' }, 409) };
}
+ const workerOrgIds = c.var.workerOrgIds ?? [];
+ if (!workerOrgIds.includes(row.organization_id)) {
+ return { device: null, error: c.json({ error: 'Forbidden' }, 403) };
+ }
return { device: { id: row.id, organization_id: row.organization_id } };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/worker-api.ts` around lines 1704 - 1726,
resolveDeviceWorkerForRequest currently only validates (user_id, worker_id) but
must also enforce that the device's organization_id is one of the worker-scoped
org IDs in c.var.workerOrgIds; after fetching row in
resolveDeviceWorkerForRequest (the row variable from device_workers) add a check
that row.organization_id is included in c.var.workerOrgIds (treat
c.var.workerOrgIds as the allowed org ID list) and if not return an error
response (e.g. c.json({ error: 'Forbidden' }, 403)); keep the existing
null/empty organization_id check but perform the scope inclusion check before
returning the device object.
# Conflicts: # db/schema.sql # packages/server/src/db/embedded-schema-patches.ts
This was referenced May 14, 2026
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
2 participants
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.
Summary
Three intertwined changes that share the same plumbing:
browser_sessionauth profile can now live on a specific Mac, with cookies in a managed--user-data-dir(copy mode) or attached to a running Chrome over CDP (attach mode). Server stores only metadata; cookies never leave the device.local.directory.filesis nowuserManaged; the Mac app creates one feed per folder via the new/api/workers/me/feedsendpoints, withfolder_id(deterministic SHA256-of-bookmark) +display_namein the feed config. Auto-wire no longer creates a default empty feed, and existing orphans are cleaned up on first reconcile./<slug>/notificationswhen an item has noresource_url.Schema
auth_profilesgainsdevice_worker_id uuid+browser_kind text(chrome|brave|arc|edge) +user_data_dir text+cdp_url text. Migrations20260513120000+20260513150000. Invariant (app-enforced): a device-boundbrowser_sessionhas exactly one of{user_data_dir, cdp_url}non-null.New endpoints (worker-JWT scoped)
GET/POST/DELETE /api/workers/me/auth-profiles— Mac CRUDs its own device's browser auth profiles.GET/POST/DELETE /api/workers/me/feeds— Mac CRUDs feeds on its auto-wired device connections.GET /api/workers/me/browser-connectors— dynamic list of installed connectors with abrowserauth method, so the Mac picker stays in sync.Test plan
make dev) boots clean with new schema patches applied to PGlite.filesfeeds are deleted on first reconcile.connector_definitionswith abrowserauth method. Copy mode materializes a--user-data-dir. CDP mode probes 9222–9225.bun run typecheckclean,make build-packagesclean,xcodebuildMac app clean.requiresBrowser).Companion
Web submodule branch:
lobu-ai/owletto-web/feat/browser-profile-pickers(already pushed) — picker label + device-lock changes that consume the newdevice_worker_idfield onAuthProfileItem. Parent's submodule pointer bumped to that branch's tip.Out of scope (follow-ups)
browser_sessionauth_data path in favor of opt-in cookie upload from the device.Summary by CodeRabbit
Release Notes
New Features
Improvements
Database