feat(notifications): unify with events; per-user delivery via notification_targets#707
Merged
Conversation
…ation_targets
Notifications were a separate table with its own per-user `is_read` state.
Conceptually a notification is just a timestamped, addressable event with
specific delivery — the content belongs in `events` (searchable, indexable,
linkable from /knowledge), and the per-user inbox state belongs in a thin
join table.
Schema
* New table `notification_targets (event_id, user_id, delivered_at, read_at)`.
* Indexes for "list my unread, newest first" and "list all of mine".
* Migration 20260513200000 backfills existing notifications into events
(semantic_type='notification', notification_type + resource_url etc. in
metadata) and into per-user target rows, then drops public.notifications.
* Embedded-schema patch mirrors the migration for PGlite dev DBs (no-ops
when the legacy table is absent).
Service refactor (packages/server/src/notifications/service.ts)
* createNotificationForUsers inserts ONE event + N targets in a
transaction (was N notification rows). "Send to admins" no longer
fans out content duplicates.
* listNotifications JOINs events ⋈ notification_targets, projects the
same NotificationRow shape (id = event_id, is_read derived from
read_at IS NOT NULL) so the REST surface + Mac client + web inbox
are byte-identical.
* markAsRead / markAllAsRead UPDATE notification_targets.read_at.
* deleteNotification drops the user's target row only (the event stays
in the org-wide knowledge stream — other users' targets unaffected).
* Scoping is preserved: org membership + matching user_id are still
required for inbox reads/writes; another org member never sees your
inbox row even though they can see the event in /knowledge.
Click-fallback parity
* Notifications still carry `resource_url` in metadata; clients open it
when set. With everything backed by events, callers can also link
directly to /<slug>/knowledge?event=<id> for the unified-detail view.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates notifications from a denormalized ChangesNotifications Events Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant Events
participant Targets
Client->>Service: createNotificationForUsers(userIds, ...)
Service->>Events: INSERT events (semantic_type=notification, metadata, payload_text)
Service->>Targets: INSERT notification_targets for each userId (event_id, user_id, delivered_at)
Client->>Service: listNotifications(userId, cursor)
Service->>Targets: SELECT notification_targets WHERE user_id
Service->>Events: JOIN events ON event_id to fetch type/body/resource_*
Service-->>Client: return inbox rows with is_read derived from read_at
Client->>Service: markAsRead(eventId, userId)
Service->>Targets: UPDATE notification_targets SET read_at = now()
Client->>Service: deleteNotification(eventId, userId)
Service->>Targets: DELETE notification_targets WHERE event_id AND user_id
Events->>Targets: (CASCADE) remove targets when event deleted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 unit tests (beta)
Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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: e95326060c
ℹ️ 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
+209
to
+211
| AND (${cursor}::bigint IS NULL OR e.id < ${cursor}) | ||
| AND (${!unreadOnly} OR t.read_at IS NULL) | ||
| ORDER BY t.delivered_at DESC, e.id DESC |
There was a problem hiding this comment.
When notifications are created concurrently (or when backfilled rows have delivered_at values that do not monotonically match events.id), this pages by ORDER BY t.delivered_at DESC, e.id DESC but resumes with only e.id < cursor. If the first page returns a lower-id row with a later delivered_at, the next request excludes higher-id rows that should follow it, so users can permanently miss notifications while paginating. Use an ordering that matches the cursor, or carry a composite (delivered_at, id) cursor.
Useful? React with 👍 / 👎.
Codex flagged on PR #707: pagination by 'ORDER BY t.delivered_at DESC, e.id DESC' combined with a cursor of 'e.id < $cursor' can permanently skip notifications when delivered_at and e.id disagree (concurrent inserts, or backfilled rows whose delivered_at = legacy created_at but events.id ordering differs). Drop the delivered_at primary sort — events.id is monotonic per org and matches the cursor exactly. The correlation between delivered_at and e.id stays close enough for the inbox UI (delivered events are inserted right after the event), and strict ordering by e.id is what the keyset pagination requires.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/notifications/service.ts (1)
161-162: 💤 Low valueConsider throwing an error instead of silently returning when eventId is missing.
If the INSERT succeeds but unexpectedly returns no rows, the transaction commits without inserting any targets, and the caller receives no indication of failure. Throwing here would surface unexpected edge cases during debugging.
Suggested change
const eventId = inserted[0]?.id; - if (!eventId) return; + if (!eventId) { + throw new Error('Failed to insert notification event'); + }🤖 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/notifications/service.ts` around lines 161 - 162, The code currently silently returns when inserted[0]?.id is missing (the eventId check), which hides a failed/partial insert; replace the early return with throwing a descriptive Error (e.g., `throw new Error("Notification event inserted but no id returned")`) so the transaction bubbles up and can be rolled back and the caller can detect the failure; update the check around inserted and eventId (the variables `inserted` and `eventId` in this function in service.ts) to throw instead of return, preserving any surrounding transactional context.
🤖 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.
Nitpick comments:
In `@packages/server/src/notifications/service.ts`:
- Around line 161-162: The code currently silently returns when inserted[0]?.id
is missing (the eventId check), which hides a failed/partial insert; replace the
early return with throwing a descriptive Error (e.g., `throw new
Error("Notification event inserted but no id returned")`) so the transaction
bubbles up and can be rolled back and the caller can detect the failure; update
the check around inserted and eventId (the variables `inserted` and `eventId` in
this function in service.ts) to throw instead of return, preserving any
surrounding transactional context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 303c33ec-9399-4ca8-8a3b-09286f79a587
📒 Files selected for processing (1)
packages/server/src/notifications/service.ts
buremba
added a commit
that referenced
this pull request
May 13, 2026
* 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.
Same fix as feat/browser-profiles a9a5e63 — the file imported bun:test which fails to resolve under the vitest integration suite, taking out the whole 67-test run. Including it here so #707's CI passes independently of the other branch. Once one branch lands on main the other will conflict cleanly (same byte change).
buremba
added a commit
that referenced
this pull request
May 13, 2026
…+ Mac UI overhaul (#706) * feat(browser-profiles): device-bound browser auth + per-folder feeds + 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. * fix(browser-profiles): address codex review on #706 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. * fix(browser-profiles): address coderabbit review on #706 * 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. * fix(worker-api): drop unused browserUserDataDir destructure 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. * chore: bump submodule pointer + fix vitest integration import * 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.
Merged
6 tasks
buremba
added a commit
that referenced
this pull request
May 14, 2026
… scheduled_jobs table (#710) * feat(scheduled-jobs): user-driven cron / one-shot via TaskScheduler + new scheduled_jobs table Background ========== TaskScheduler already gives us a pg-backed scheduler (claim / retry / idempotency / observability via the `runs` queue), but `register()` is code-side only and `spawn()` alone can't track recurring definitions. This PR adds the missing piece — a user-driven *registry* — without forking the scheduler. Schema ------ * `scheduled_jobs` (id, organization_id, action_type, action_args, cron, next_run_at, last_fired_at, paused, description, created_by_user, created_by_agent, source_run_id, source_event_id, source_thread_id). * FK ON DELETE CASCADE on `agents` (when present) so deleting an agent also drops its scheduled wake-ups — no orphan jobs firing into the void. Conditional FKs on runs/events for traceability. * Indexes on (next_run_at WHERE NOT paused), (org, agent), (org, user). * Migration 20260514000000 + embedded patch (idempotent). Wiring ------ * `registerScheduledJobsTicker(scheduler)` registers a `* * * * *` cron. Each tick: SELECT due rows FOR UPDATE SKIP LOCKED → spawn(action_type, payload, { idempotencyKey: 'scheduled_job:<id>:<tick-iso>' }) → advance next_run_at (or paused=true for one-shot). Failure between spawn and advance is fine — next minute's tick re-claims the same row, and the idempotency key dedups the re-spawn. * Two task handlers in scheduled/jobs.ts: - `send_notification` — resolves recipients (admins / all / list) and calls createNotificationForUsers (which now writes to events + notification_targets per PR #707). - `wake_agent` — creates a thread (or reuses one) and enqueues a synthetic user message via the existing agent-threads API. Lets an agent schedule its own follow-up wake-up. Tool ---- * `manage_schedules` MCP tool: create / list / pause / cancel. Create captures attribution from ToolContext (created_by_user; agent attribution wires up when the gateway agent path lands). * Per-action payload validation (typebox discriminated union): send_notification needs title; wake_agent needs agent_id + prompt. Smoke test (verified) --------------------- Scheduled a notification 60s out → ticker fired at next minute boundary → row marked paused (one-shot done) with last_fired_at set → notification appeared via the events/notification_targets path. The full chain (tool → table → cron tick → spawn → handler → notification) works end-to-end on a fresh PGlite dev DB. Out of scope (follow-ups) ------------------------- * Web UI to list/pause/cancel schedules (manage_schedules is REST- reachable today, just no dedicated page). * Agent attribution on ToolContext so a wake-up scheduled by an agent populates `created_by_agent`. Currently always created_by_user. * `last_fired_run_id` is wired as a column but not yet populated — the spawn() return path doesn't surface the runs.id back to the ticker. Trivial follow-up. * fix(scheduled-jobs): address codex review + CI typecheck * typecheck: drop unused pgTextArray import from scheduled-jobs-service. * wake_agent: read __created_by_user (not created_by_user) — the ticker injects the scheduling user under that __ prefix to avoid colliding with user-supplied action_args. Without this fix, user-scheduled wake-ups attributed their thread to the agent instead of the user. * wake_agent: existence-check the target agent before enqueuing. The scheduled_jobs.created_by_agent cascade only covers the *scheduler*'s identity; the *target* of a wake_agent payload lives in action_args and isn't FK-protected. Auto-pause the schedule when the target agent is gone so we don't silently enqueue messages for ghosts. * Bump web submodule to owletto-web/main (e222de8) — required for the CI check-drift gate.
7 tasks
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
Notifications used to be their own table with per-user
is_read. They're now events withsemantic_type='notification'+ a thinnotification_targets(event_id, user_id, delivered_at, read_at)table for per-user delivery / read state.events: searchable, addressable, linkable from/<slug>/knowledge?event=<id>. Watchers/agents that already emit events get a notification surface for free.events ⋈ notification_targets WHERE user_id = me. Scoping is preserved: org membership + matchinguser_idare still required to read/write your inbox; another org member never sees your inbox rows even though they can see the underlying event in the org-wide stream.Migration
20260513200000_notifications_as_events.sqlbackfills every existing notification into:Then drops
public.notifications. One-way migration — recovery is from backup.Embedded-schema patch mirrors this for PGlite dev DBs (no-op when the legacy table is absent).
API compatibility
NotificationRowshape is preserved —idis now the event id (still bigint),is_readis derived fromt.read_at IS NOT NULL. REST endpoints (/api/<slug>/notifications/*), the Mac client (LobuNotification), and the web inbox UI all see the same wire shape. Zero client-side changes needed.Test plan
dbmate upagainst a fresh Postgres applies migration cleanly + dumps stable schema.make build-packagesclean,bun run typecheckclean.notificationsrows → backfill preserves title, body, user assignment, is_read state.Follow-ups (separate PRs)
resource_url, deep-link to/<slug>/knowledge?event=<id>(already partially done in feat(browser-profiles): device-bound browser auth + per-folder feeds + Mac UI overhaul #706's Mac fallback).events.recipient_user_idfor genuinely private events (auth-token-expired, billing) where even the org-wide stream shouldn't surface them. Today's model treats those the same as any event; future PR can add a visibility filter when the use case crystallizes.Summary by CodeRabbit