docs: investigate typed domain-event / lifecycle bus seam#246
Conversation
![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: 239e7abc34
ℹ️ 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".
| **`workspace:switched`, emitted from `Repo.setActiveWorkspaceId`.** It is the | ||
| single event with *no* current observability, it is one line at one choke, its | ||
| payload is two strings, and it directly closes gap I3's correctness hole | ||
| ("plugins that auto-create blocks need a reliable active-workspace-changed | ||
| signal rather than racing the async bootstrap"). Shipping just this proves the |
There was a problem hiding this comment.
Emitting workspace:switched directly from Repo.setActiveWorkspaceId does not provide the post-switch lifecycle point this paragraph promises. The switcher calls the setter before even updating the hash (src/components/workspace/WorkspaceSwitcher.tsx:50-66), and app bootstrap calls it before the access gate/read-only check and before bootstrapWorkspace (src/App.tsx:72-115). In those paths, handlers that auto-create blocks would run against a workspace that may still be locked/waiting or not bootstrapped, and new workspace-scoped effects may not be mounted yet; the design should distinguish the low-level active pin change from an after-ready/after-bootstrap lifecycle event.
Useful? React with 👍 / 👎.
| | `block:created` | `{block, type, workspaceId, txId}` | `Repo._runAndDispatch` post-commit, line ~1166 (`processorRunner.dispatch`) — the snapshots walk already classifies before/after | `before===null && after!==null`. Type read from `after.properties`/`type`. | | ||
| | `block:updated` | `{id, before, after, changedFields, type, workspaceId, txId}` | same | `before!==null && after!==null`. `changedFields` already computable (cf. `fieldChanged` in `processorRunner.ts`). | | ||
| | `block:deleted` | `{id, before, type, workspaceId, txId}` | same | soft-delete = `deleted` flips; hard-delete = `after===null`. | |
There was a problem hiding this comment.
types list
The proposed block-event payloads and extraction logic use a singular type, but this codebase stores block membership as the types property with a list codec (src/data/properties.ts:119-123) and reads it through getBlockTypes/hasBlockType (src/data/properties.ts:250-258). Following this sketch would emit undefined/a single type for real multi-typed blocks and make type-filtered block:* subscriptions incorrect; the payload/filter contract should be based on types: readonly string[] or explicit membership deltas.
Useful? React with 👍 / 👎.
| - **Implementation:** `Map<eventName, CallbackSet>` — reuse `CallbackSet` | ||
| (already the codebase's isolated-fan-out primitive; `repo.userErrorListeners` | ||
| uses it). No new machinery. |
There was a problem hiding this comment.
This recommends CallbackSet for the bus implementation, but the bus contract below allows Promise handlers and requires awaited, sequential, isolated delivery. CallbackSet.notify only calls listeners synchronously inside a try/catch and returns void (src/utils/callbackSet.ts:35-42), so rejected promises are neither awaited nor caught and async handlers would run concurrently rather than sequentially. The design should call out a separate async fan-out primitive or an adapted handler loop instead of reusing CallbackSet directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11df20f9b3
ℹ️ 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".
| - One event declared: `workspace:ready`. One emit line at the **end of | ||
| `bootstrapWorkspace`** (the `{kind:'ready'}` return) — *not* | ||
| `setActiveWorkspaceId`, so handlers run post-gate/post-bootstrap (the timing |
There was a problem hiding this comment.
When the recommended workspace:ready consumer is an AppEffect, emitting this one-shot event at the end of bootstrapWorkspace makes cold-start handlers miss it: getInitialLayout awaits bootstrapWorkspace and returns {kind:'ready'} before App renders AppRuntimeProvider (src/App.tsx:115-123, src/App.tsx:178-180, src/App.tsx:296-300), while AppEffects are only registered later from AppRuntimeProvider's useEffect (src/extensions/AppRuntimeProvider.tsx:190-195). In that scenario, the auto-create/watch-switch effect shown above is not subscribed yet, so the thin slice would not close I3 unless the design moves the emission to after effect reconciliation or makes workspace:ready sticky/replayable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f007113f2
ℹ️ 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".
| | `block:updated` | `{id, before, after, changedFields, types, addedTypes, removedTypes, workspaceId, txId}` | same | `before!==null && after!==null`. `changedFields` via `fieldChanged` (`processorRunner.ts`); membership deltas via the `getBlockTypes`-diff helpers (`properties.ts:260+`). | | ||
| | `block:deleted` | `{id, before, types, workspaceId, txId}` | same | soft-delete = `deleted` flips; hard-delete = `after===null`. `types` via `getBlockTypes(before)`. | |
There was a problem hiding this comment.
With the proposed before!==null && after!==null classification, a normal soft delete also satisfies block:updated, because tx.delete updates the existing row by flipping deleted (src/data/internals/txEngine.ts:309-316) and delete mutators rely on that soft-delete path (src/data/mutators.ts:214-243). That means a single deletion would notify both block:updated and block:deleted, contradicting the later “exactly once” contract and causing update observers to run for removals unless the update case explicitly excludes deleted transitions.
Useful? React with 👍 / 👎.
| in `sameTxProcessor`; pre-decision interception stays in verb `decorators`. | ||
| This is the single most important boundary: the bus is *notification*, which | ||
| is why it can be fire-and-forget and why it's safe for any plugin to listen. | ||
| - **Ordering across events is the emit order;** within an event it's |
There was a problem hiding this comment.
With the API above, emit returns void and starts an async delivery loop; a second emit can start another loop while a slow handler from the first is still being awaited. In that scenario later events can overtake earlier ones, so the stated emit-order guarantee is false unless the bus serializes emissions through an internal promise queue or the doc removes the guarantee.
Useful? React with 👍 / 👎.
| **Phase 1 — lifecycle events.** `workspace:created` (from `bootstrapWorkspace`, | ||
| reusing `freshlyCreated`), `app:booted`, `runtime:swapped` (forward from the | ||
| existing `refreshAppRuntime` signal). Turns the bus into the "lifecycle hook" |
There was a problem hiding this comment.
In the current flow, refreshAppRuntime() only dispatches app-runtime-update; useOverrides consumes that to bump generation before resolveAppRuntime runs, while the actual swap happens later in AppRuntimeProvider after setRuntime(nextRuntime)/repo.setFacetRuntime(nextRuntime) and reconciliation. Forwarding this signal as runtime:swapped fires too early, and even on a failed async resolve, so handlers that read new facets/effects can still see the old runtime; either name this a refresh-requested event or emit after the provider installs the new runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0509b54bab
ℹ️ 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".
| | `block:created` | `{block, types, workspaceId, txId}` | `Repo._runAndDispatch` post-commit, line ~1166 (`processorRunner.dispatch`) — the snapshots walk already classifies before/after | `!liveBefore && liveAfter` (insert *or* un-delete). `types` via `getBlockTypes(after)` — a block carries a **`types` list**, not a single type (`properties.ts:119`). | | ||
| | `block:updated` | `{id, before, after, changedFields, types, addedTypes, removedTypes, workspaceId, txId}` | same | `liveBefore && liveAfter && content changed`. `changedFields` via `fieldChanged` (`processorRunner.ts`); membership deltas via the `getBlockTypes`-diff helpers (`properties.ts:260+`). | | ||
| | `block:deleted` | `{id, before, types, workspaceId, txId}` | same | `liveBefore && !liveAfter`. Covers **soft-delete** (`tx.delete` flips `deleted=true` on the existing row, `txEngine.ts:306` — `before`/`after` both present) *and* hard-delete (`after===null`). `types` via `getBlockTypes(before)`. | | ||
| | `block:synced` (down-sync applied) | `{ids, workspaceId}` | `startBlocksSyncedObserver` → `applyOutcome` (`syncObserver/observer.ts:167`) — already walks materialized snapshots | distinct from the local-write `block:*` events: these are *remote* rows landing. Pairs with `invalidationRules`. | |
There was a problem hiding this comment.
At this choke, a single drain window is not workspace-scoped: drainQueueOnce reads blocks_synced_changes ordered only by seq and passes all ids in the window to materializeStagingRows, whose MaterializeOutcome.snapshots can therefore contain rows from multiple workspaces. Emitting one block:synced payload shaped as {ids, workspaceId} would mislabel some ids (or drop them) whenever one sync batch contains rows from two workspaces, so workspace-filtered subscribers would run for the wrong data; the bridge needs to group snapshots by before/after.workspaceId or carry workspace per id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4068ce4d35
ℹ️ 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".
| | `block:updated` | `{id, before, after, changedFields, types, addedTypes, removedTypes, workspaceId, txId}` | same | `liveBefore && liveAfter && content changed`. `changedFields` via `fieldChanged` (`processorRunner.ts`); membership deltas via the `getBlockTypes`-diff helpers (`properties.ts:260+`). | | ||
| | `block:deleted` | `{id, before, types, workspaceId, txId}` | same | `liveBefore && !liveAfter`. Covers **soft-delete** (`tx.delete` flips `deleted=true` on the existing row, `txEngine.ts:306` — `before`/`after` both present) *and* hard-delete (`after===null`). `types` via `getBlockTypes(before)`. | | ||
| | `block:synced` (down-sync applied) | `{workspaceId, ids}` — **one emit per workspace** present in the window | `startBlocksSyncedObserver` → `applyOutcome` (`syncObserver/observer.ts:167`) — walks materialized snapshots | distinct from local-write `block:*`: these are *remote* rows landing. A drain window is **seq-ordered, not workspace-scoped** (`drainQueueOnce` reads `blocks_synced_changes ORDER BY seq`, `observer.ts:193`), so its `MaterializeOutcome.snapshots` can span workspaces — the bridge must **group by `after/before.workspaceId`** and emit one event per workspace, exactly as the observer already does for cycle scans (`cycleScanCandidatesByWorkspace`, `observer.ts:114`). A single `{ids, workspaceId}` payload would mislabel cross-workspace batches. Pairs with `invalidationRules`. | | ||
| | `navigation:completed` | `{result: NavigationResult, input, origin}` | `navigationVerb.after` (`utils/navigation.ts:289`) — **already exists** as a Sum observer slot | bridge, don't re-emit: an internal `after` observer forwards onto the bus. | |
There was a problem hiding this comment.
In the inspected src/utils/navigation.ts/defineVerbFacet flow, navigationVerb.after receives a VerbOutcome<NavigationResult | null> and runs for vetoes, null results, and thrown navigation failures, not just completed navigations. If the bridge forwards every after call into the proposed navigation:completed payload with result: NavigationResult, subscribers will either see a completed event for a navigation that did not land or get a missing/null result; the design should explicitly emit only when outcome.ok && outcome.result !== null (or define a separate failed/cancelled event).
Useful? React with 👍 / 👎.
| | `workspace:active-changed` (low-level pin) | `{previousId, workspaceId}` | `Repo.setActiveWorkspaceId` (`repo.ts:943`) — **fires nothing today** | optional/secondary: reflects the *pin*, fires early (pre-gate, pre-bootstrap). For UI that just tracks "which workspace is selected", not for auto-create handlers. | | ||
| | `workspace:created` | `{workspaceId, freshlyCreated}` | `bootstrapWorkspace` (has `freshlyCreated`) and/or `CreateWorkspaceDialog.onCreated` | `freshlyCreated` is already threaded through bootstrap; `workspace:ready` with `freshlyCreated:true` may subsume this. | | ||
| | `sync:status` (online/offline/synced) | `{connected, hasSynced, uploading, downloading, …}` | PowerSync status listener — already consumed by `system-status` (`SyncIndicatorInput`) | the chip already derives this; the bus would expose the *transitions* to plugins. | | ||
| | `app:booted` | `{repo, workspaceId}` | end of `bootstrapWorkspace` / `App.tsx` post-bootstrap | one-shot per session. | |
There was a problem hiding this comment.
Using the end of bootstrapWorkspace as the app:booted choke conflicts with the stated “one-shot per session” semantics: in the inspected App flow, a workspace hash change re-runs resolveInitialLayout, which calls bootstrapWorkspace again for each successfully opened workspace. Session-level observers would therefore run once per workspace switch rather than once per app boot; either gate this event once per Repo/session or emit it from an actual app-mount/initial-ready point, leaving per-workspace readiness to workspace:ready.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Design investigation (no code) for the general 'plugins react to application events' seam: enumerate the domain events worth emitting and the existing chokes to emit them from, survey the partial mechanisms that already exist (post-commit + same-tx processors, invalidation rules, projectors, verb-facet before/after observers, app effects, toggle stores), and propose a typed observer bus. Key findings vs the earlier gap docs: - defineVerbFacet + navigationVerb.before/after now exist, so the navigation event already has an observer slot to bridge from. - sameTxProcessor already ships a declaration-merged typed event registry (SameTxEventRegistry + tx.emitEvent), so a typed event registry is the established house style. - workspace:switched (Repo.setActiveWorkspaceId) is the one event with no observability at all and real demand (gap I3) -> recommended thin slice. Recommends a narrow parallel observe-only bus (not widening the processor system), keyed by a declaration-merged AppEventRegistry, delivered with the verb-observer contract (async, sequential, error-isolated, no veto), hosted on Repo, with subscriptions living in app effects / a useAppEvent hook. Explicitly stays within the B3 typed-channel rules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…pace event, async delivery Three P2 review corrections to the design doc (all verified against the tree): - block events: a block carries a `types` LIST (typesProp = list(string), read via getBlockTypes/hasBlockType), not a singular `type`. Payloads now carry `types: readonly string[]` + add/removed membership deltas; "filter by type" is list-membership, not equality. - workspace lifecycle: emitting from Repo.setActiveWorkspaceId fires too early — it runs before the hash change (WorkspaceSwitcher) and before the access gate + bootstrapWorkspace (App.tsx:72), so handlers could run against a locked/un-bootstrapped workspace, recreating the I3 race. Split into a post-bootstrap `workspace:ready` (emitted at the end of bootstrapWorkspace) vs a low-level `workspace:active-changed` pin. Thin slice is now `workspace:ready`. - delivery: CallbackSet.notify is synchronous/void and neither awaits nor catches promise rejections, so it can't back the async/sequential/ isolated contract. Reuse only its Set storage; deliver via a dedicated await-loop (the verbFacet before/after pattern). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…AppEffect registration Codex review (commit 11df20f): a one-shot workspace:ready emitted at the end of bootstrapWorkspace is missed by AppEffect subscribers on cold start. Verified: AppRuntimeProvider reconciles AppEffects in a useEffect (AppRuntimeProvider.tsx:184) that runs after first render, while bootstrapWorkspace resolves earlier (consumed via React use()), so the emit fires before any AppEffect has subscribed. Fix: promote 'sticky / replay-last-value-on-subscribe' from an open question to a design requirement for current-state events (workspace:ready, app:booted, latest sync:status). The bus retains the last payload and replays it to a handler at subscribe time, so a late-registering AppEffect (cold start, or mid-session plugin load) still receives it. Sticky handlers must be idempotent (replay + later live emit) — auto-create handlers already are. Transient events (block:*, navigation:*, workspace:active-changed) stay non-sticky. workspace:ready brings the sticky mechanism into the Phase 0 thin slice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
… runtime:swapped timing Three more Codex review corrections (commit 3f00711), all verified: - block:updated double-fires on deletes. tx.delete soft-deletes by flipping deleted=true on the existing row (txEngine.ts:306), so before/after are both non-null and the 'before!==null && after!==null' rule would emit BOTH block:updated and block:deleted for one deletion. Reclassify on live ≡ present && !deleted: created = !liveBefore && liveAfter; deleted = liveBefore && !liveAfter (covers soft+hard); updated = liveBefore && liveAfter && content changed. Now mutually exclusive, honoring the exactly-once contract. - emit-order over-promise. emit() returns void and starts its own async loop, so a slow handler in one emit lets a later emit overtake it — the 'ordering across events is emit order' claim was false. Drop it: promise per-emit handler order only, explicitly no cross-event order (serializing would head-of-line-block the whole stream on one slow observer). - runtime:swapped fired too early. refreshAppRuntime is refresh-REQUESTED (bumps useOverrides generation before the async resolve, fires even on a failed resolve); the swap actually installs later via repo.setFacetRuntime (AppRuntimeProvider.tsx:172) + reconcile (:194). Emit after install so handlers see the new runtime, not the old one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…kspaces) Codex review (commit 0509b54): a {ids, workspaceId} block:synced payload is wrong because drainQueueOnce reads blocks_synced_changes ORDER BY seq (observer.ts:193) with no workspace filter, so one drain window's MaterializeOutcome.snapshots can span workspaces. Group by after/before.workspaceId and emit one event per workspace — the observer already does exactly this for cycle scans (cycleScanCandidatesByWorkspace, observer.ts:114). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…e app:booted per session Codex review (commit 4068ce4), both verified: - navigation:completed: navigationVerb.after fires for EVERY VerbOutcome (success, veto/null, throw — verbFacet.ts), so the bridge must emit only when outcome.ok && result !== null; a veto/null/failure is a separate navigation:cancelled or dropped, never a completed with a null result. - app:booted: resolveInitialLayout re-runs bootstrapWorkspace on every workspace switch, so emitting from there fires per-switch, not once per session. Gate behind a once-per-Repo/session latch (or an app-mount point); per-workspace readiness stays with workspace:ready. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…workspace:created rows Consolidation pass framing the whole event inventory: every choke must fire where the event's promised semantics actually hold (state-is-true not requested; filter verb-observer bridges to the intended outcome; gate one-shots per session; scope payloads to one workspace). Pre-empts the per-row timing/correctness issues by stating the shared contract once, and applies it to the two remaining un-hardened rows: - navigation:requested fires pre-resolution for every gesture, so it does not imply the navigation lands (may be vetoed/null). - workspace:created: emit once via the freshlyCreated-gated bootstrap path, not also from CreateWorkspaceDialog.onCreated (double-emit); likely subsumed by workspace:ready carrying freshlyCreated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…x {kind:'ready'} attribution, drop CallbackSet over-claim
Two parallel adversarial review agents (grounding + design soundness).
Findings I stood behind, all verified against the tree:
- BLOCKER (design): sticky workspace:ready was specified as a single
global last-value slot. On the per-user singleton Repo (reused across
switches), a late/above-boundary subscriber could be replayed a STALE
workspace's payload -> auto-create handler acts on the wrong workspace,
the exact cross-workspace leak ProjectorRuntime.dispose exists to
prevent. Spec now requires sticky replay GATED on the active workspace
(replay only if payload.workspaceId === repo.activeWorkspaceId), and
states idempotency is necessary but not sufficient (wrong-workspace is
not a double-write). Added a Phase-0 switch-replay test.
- MAJOR (design): no cross-event ordering means workspace:ready does not
imply the workspace's bootstrap block:* events were delivered; lifecycle
handlers must query current state, not assume they observed bootstrap
data events. Now called out.
- MAJOR (design): internal contradiction — section 3.2 proves
CallbackSet.notify can't be the (async/sequential/isolated) delivery
path, yet 4.1 sold the bus as '~a Map<name, CallbackSet>... not a new
subsystem'. Reworded: only the Set storage is reused; delivery + gated
sticky + filtering are new (small, pattern-following) code.
- MAJOR (grounding): {kind:'ready'} was attributed to bootstrapWorkspace,
which returns a Block (return layoutSessionBlock). {kind:'ready'} is its
caller resolveInitialLayout's return (App.tsx:123). Fixed both cites.
- MINOR (design): sync:status is connection-global / app:booted is
session-global, so the 'one workspace per payload' rule applies only to
workspace-scoped events; clarified.
Both reviews confirmed the survey + remaining citations are accurate; no
false positives required overruling.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
Stvad
force-pushed
the
claude/sleepy-edison-ths7cy
branch
from
d73492c to
c096771
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c096771ed0
ℹ️ 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".
| | `block:synced` (down-sync applied) | `{workspaceId, ids}` — **one emit per workspace** present in the window | `startBlocksSyncedObserver` → `applyOutcome` (`syncObserver/observer.ts:167`) — walks materialized snapshots | distinct from local-write `block:*`: these are *remote* rows landing. A drain window is **seq-ordered, not workspace-scoped** (`drainQueueOnce` reads `blocks_synced_changes ORDER BY seq`, `observer.ts:193`), so its `MaterializeOutcome.snapshots` can span workspaces — the bridge must **group by `after/before.workspaceId`** and emit one event per workspace, exactly as the observer already does for cycle scans (`cycleScanCandidatesByWorkspace`, `observer.ts:114`). A single `{ids, workspaceId}` payload would mislabel cross-workspace batches. Pairs with `invalidationRules`. | | ||
| | `navigation:completed` | `{result: NavigationResult, input, origin}` | `navigationVerb.after` (`utils/navigation.ts:289`) — **already exists** as a Sum observer slot | bridge, don't re-emit: an internal `after` observer forwards onto the bus. But `after` fires for **every** outcome (`VerbOutcome<NavigationResult \| null>` — success, veto/`null`, throw), so emit `navigation:completed` **only when `outcome.ok && outcome.result !== null`**. A veto/`null`/failure becomes a separate `navigation:cancelled` (or is dropped) — never a `completed` carrying a missing/null result. | | ||
| | `navigation:requested` (pre) | `{input}` | `navigationVerb.before` — **already exists** | optional; most demand is for `completed`. Fires *before* resolution for every gesture, so it does **not** imply the navigation will land (it may be vetoed/`null`) — purely an "about to attempt" hook. | | ||
| | `workspace:ready` (a.k.a. switched-and-bootstrapped) | `{previousId, workspaceId, freshlyCreated}` | **end of `bootstrapWorkspace`** (`workspaceBootstrap.ts`, after the page/ui-state writes, just before `return layoutSessionBlock` — equivalently the `{kind:'ready'}` branch of its caller `resolveInitialLayout`, `App.tsx:123`; `bootstrapWorkspace` itself returns a `Block`, not the layout union) — past the access gate + bootstrap writes | the I3 lifecycle point: workspace is materializable, scoped pages exist. **Not** `setActiveWorkspaceId` — see the caveat below. | |
There was a problem hiding this comment.
At this choke the old active id is already gone: resolveInitialLayout calls repo.setActiveWorkspaceId(workspaceId) before the access gate/bootstrap (src/App.tsx:68-73), and WorkspaceBootstrapArgs only carries the new workspaceId/freshlyCreated/hash inputs (src/bootstrap/workspaceBootstrap.ts:90-98). A one-line emit at the end of bootstrapWorkspace therefore cannot populate previousId correctly; using repo.activeWorkspaceId there reports the new workspace, so capture the old id before the setter and thread it through, or drop previousId from the payload.
Useful? React with 👍 / 👎.
| | `workspace:active-changed` (low-level pin) | `{previousId, workspaceId}` | `Repo.setActiveWorkspaceId` (`repo.ts:943`) — **fires nothing today** | optional/secondary: reflects the *pin*, fires early (pre-gate, pre-bootstrap). For UI that just tracks "which workspace is selected", not for auto-create handlers. | | ||
| | `workspace:created` | `{workspaceId}` | gate on `freshlyCreated` at the `bootstrapWorkspace` choke (fires per switch, but `freshlyCreated` is only true on actual creation) — **one** path | don't *also* emit from `CreateWorkspaceDialog.onCreated` (double-emit). `workspace:ready` already carries `freshlyCreated:true`, so this may be redundant — keep only if a "created but not yet landed-on" signal is genuinely needed. | | ||
| | `sync:status` (online/offline/synced) | `{connected, hasSynced, uploading, downloading, …}` | PowerSync status listener — already consumed by `system-status` (`SyncIndicatorInput`) | the chip already derives this; the bus would expose the *transitions* to plugins. | | ||
| | `app:booted` | `{repo, workspaceId}` | a **once-per-Repo/session latch** (or an app-mount/initial-ready point) — **not** the raw end of `bootstrapWorkspace` | one-shot per session. `resolveInitialLayout` re-runs `bootstrapWorkspace` on *every* workspace switch, so emitting from there would fire per-switch; gate behind a session latch on the Repo and leave per-workspace readiness to `workspace:ready`. | |
There was a problem hiding this comment.
This row defines app:booted as carrying workspaceId, but the emit-choke contract below says app:booted is session-global and carries no workspaceId (docs/domain-event-lifecycle-bus.md:96-98), and sticky replay for global events is not workspace-gated. If implemented with this payload, a late subscriber after a workspace switch can receive the one-shot boot event with the initial workspace's id and act in the wrong scope; either remove workspaceId from app:booted or make it a per-workspace event with the same gating as workspace:ready.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,564 @@ | |||
| # Typed domain-event / lifecycle bus — design investigation (2026-06-23) | |||
|
|
|||
| Status: **investigation / proposal**. No code. Companion to | |||
There was a problem hiding this comment.
/workspace/knowledge-medium/AGENTS.md says docs/*.md design docs must use a > **Status:** … blockquote with both Status and "last verified against code" fields. This new design doc has only an inline Status: sentence and no last-verified field, so future agents following the documented process cannot tell which claims were checked against code before relying on the proposal.
Useful? React with 👍 / 👎.
…citations) Adversarial review (2 agents) + quality agent + Codex, post-rebase. Both adversarial agents: no blocker/major — design sound, thin slice closes I3. Minor/nit defects fixed: - workspace:ready dropped previousId: at the bootstrapWorkspace choke the prior active id is already gone (setActiveWorkspaceId(new) runs early at App.tsx:73), so the field can't be sourced there; it belongs on workspace:active-changed (choke = the setter, both ids in hand). [cross-agent: Codex + design agent] - app:booted dropped workspaceId: the row contradicted the emit-choke contract's 'session-global, no workspaceId'; a late/replayed subscriber would get the initial workspace's id. [Codex] - Added the required AGENTS.md:27 status banner (Status + last-verified- against-code blockquote); it was an inline sentence. [Codex] - Reframed the sticky workspace-gate from 'mandatory for the slice' to defense-in-depth: the AppEffect I3 path is safe ungated (resubscribes after the emit); the gate protects the useAppEvent/module-singleton shapes. Noted the single-slot gate (not key+evict) is the Phase-0 form, and pinned replay as synchronous-in-on(). [design agent] - Added the §3.4 bail-if-moved guard to the §3.3 flagship example (it violated its own handler contract). [design agent] - Citation drift from the rebase: verbFacet.ts:197->305 (the actual await-loop; was a signature), processorRunner.dispatch ~1174, setActiveWorkspaceId repo.ts:951, navigationVerb :294, invalidation repo.ts:1166, App.tsx 72->73/123->126, WorkspaceSwitcher :57. [grounding] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…, add BLUF Quality-agent editorial pass (separate from the defect fixes). Behavior/ meaning-preserving: - Added a 'Recommendation in one breath' BLUF near the top so a maintainer gets the verdict before ~440 lines of evidence (Q5). - Folded §4.3 'Smallest first thin slice' into a one-line pointer to Phase 0, which it fully duplicated (Q2). - Trimmed Open Questions: the 'Replay / late-subscribe' bullet was mostly a recap of already-*decided* §3.5 material — reduced to the genuinely open mechanism question; de-duped the 'Bus on Repo' sticky caveat and the declarative-facet bullet to cross-refs (Q4/Q1). The ~6x-restated 'sticky must be workspace-gated' point now has one canonical home (§3.5); Phase 0 / §4.3 / open-questions cross-ref it instead of re-deriving. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1843110db7
ℹ️ 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".
| | `navigation:requested` (pre) | `{input}` | `navigationVerb.before` — **already exists** | optional; most demand is for `completed`. Fires *before* resolution for every gesture, so it does **not** imply the navigation will land (it may be vetoed/`null`) — purely an "about to attempt" hook. | | ||
| | `workspace:ready` (a.k.a. switched-and-bootstrapped) | `{workspaceId, freshlyCreated}` | **end of `bootstrapWorkspace`** (`workspaceBootstrap.ts`, after the page/ui-state writes, just before `return layoutSessionBlock` — equivalently the `{kind:'ready'}` branch of its caller `resolveInitialLayout`, `App.tsx:126`; `bootstrapWorkspace` itself returns a `Block`, not the layout union) — past the access gate + bootstrap writes | the I3 lifecycle point: workspace is materializable, scoped pages exist. **Not** `setActiveWorkspaceId` — see the caveat below. **No `previousId`:** by this choke `setActiveWorkspaceId(new)` (early, `App.tsx:73`) has already overwritten the pin, so the prior id is gone; it belongs on `workspace:active-changed`, whose choke *is* the setter (both ids in hand). | | ||
| | `workspace:active-changed` (low-level pin) | `{previousId, workspaceId}` | `Repo.setActiveWorkspaceId` (`repo.ts:951`) — **fires nothing today** | optional/secondary: reflects the *pin*, fires early (pre-gate, pre-bootstrap). For UI that just tracks "which workspace is selected", not for auto-create handlers. | | ||
| | `workspace:created` | `{workspaceId}` | gate on `freshlyCreated` at the `bootstrapWorkspace` choke (fires per switch, but `freshlyCreated` is only true on actual creation) — **one** path | don't *also* emit from `CreateWorkspaceDialog.onCreated` (double-emit). `workspace:ready` already carries `freshlyCreated:true`, so this may be redundant — keep only if a "created but not yet landed-on" signal is genuinely needed. | |
There was a problem hiding this comment.
This choke misses normal user-created workspaces: freshlyCreated is only set when resolveWorkspace inserts/ensures the personal workspace (src/bootstrap/resolveWorkspace.ts:13-18), while a workspace created from the dialog is primed locally and then navigated to (src/components/workspace/CreateWorkspaceDialog.tsx:80-95), after which resolveWorkspace takes the existing-local fast path with freshlyCreated: false (src/bootstrap/resolveWorkspace.ts:40-44). If Phase 1 follows this guidance and avoids the dialog/create path, workspace:created subscribers will never hear about those newly created workspaces.
Useful? React with 👍 / 👎.
| - **Replay timing.** For a sticky event the replay runs synchronously inside | ||
| `on()` (before it returns), so it can never be reordered behind a future live | ||
| emit, and a throwing replayed handler is isolated the same way a live one is | ||
| (logged, not propagated to the `on()` caller / the AppEffect `start`). Benign | ||
| for `workspace:ready` (no concurrent live emit at subscribe), but pin it so an | ||
| implementer doesn't make replay a deferred microtask and reintroduce a race. |
There was a problem hiding this comment.
With on() returning only an unsubscribe and handlers allowed to return promises, a sticky replay cannot be delivered to completion “synchronously inside on()”; it can only start an async delivery loop. Because the design above also says each emit starts its own fire-and-forget loop, a newer live sync:status/other sticky update emitted immediately after subscription can overtake a slow replay, so the handler may process stale current-state after fresh current-state unless sticky replay and live emits share a per-event queue or the API exposes an awaited subscribe/replay boundary.
Useful? React with 👍 / 👎.
…ticky replay ordering) Round-2 review: the 3 areview agents converged (grounding clean, design consistent, quality cleanups-only), but Codex found 2 real above-nit defects my agents missed — both on later-phase events, not the thin slice. Verified and fixed: - workspace:created: gating on freshlyCreated at the bootstrap choke MISSES dialog-created workspaces. freshlyCreated is only true for the auto-ensured personal workspace (resolveWorkspace.ts:98/113); a dialog-created ws is primed+navigated, so resolveWorkspace returns it via the existing-local fast path with freshlyCreated:false (:40-44). Corrected to emit from the real insert sites (ensure*PersonalWorkspace when inserted + the dialog create path), dedupe by id. - sticky replay ordering: round-1 claimed replay runs 'synchronously inside on()' so it can't be reordered behind a live emit. True for INVOCATION order, but an async handler's COMPLETION isn't synchronous, so a concurrent-emit sticky event (sync:status) can go stale-after- fresh. workspace:ready is immune (no concurrent emit at subscribe); concurrent-emit sticky events need per-event serialization — folded into the sticky open question. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…t, compress Phase 0 Quality-agent editorial pass (separate from round-2 defect fixes): - §4.3 collapsed to a true one-line pointer to Phase 0 (round-1 under- delivered; it still re-listed the whole shape). - §1 timing caveat trimmed 13->7 lines (kept the line refs + the 'recreate the I3 race' punch + the active-changed split; the emit-choke contract bullet already states the general principle). - Phase 0 sticky bullet compressed to cross-ref §3.5 instead of re-deriving the sticky/gate rationale. - Open-questions 'must'->'should be workspace-gated (defense-in-depth)' to echo §3.5 exactly; recorded the replay-vs-live serialization item. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
…o Phase 1 Round-3 confirmation caught an un-propagated fix: §4.2 Phase 1 still said 'workspace:created (from bootstrapWorkspace, reusing freshlyCreated)', contradicting the §1 row just corrected in round 2 and re-prescribing the 'silently miss every dialog-created workspace' bug. Matched Phase 1 to §1 (emit from the real insert sites; not freshlyCreated). Both round-2 spots (workspace:created §1 row, sticky replay-ordering bullet) verified correct in isolation; this was the missed echo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9407557017
ℹ️ 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".
| - **Data events are bridged, not double-emitted.** `block:created/updated/deleted` | ||
| are emitted by *one* internal post-commit hook reading the same snapshots the | ||
| processor runner already has, so they fire exactly once per committed tx and | ||
| inherit the "registry snapshotted at tx start" / fire-after-resolve timing. |
There was a problem hiding this comment.
For a transaction that touches multiple blocks, Repo._runAndDispatch receives a SnapshotsMap, and bootstrap/mutators can add several entries to that map in one commit. Since the proposed block:* payloads are singular (block/id), saying these events “fire exactly once per committed tx” would either drop all but one changed block or force an undeclared aggregate payload; the bridge should walk every snapshot entry and emit one mutually-exclusive event per affected row.
Useful? React with 👍 / 👎.
| - **Workspace scope is in the payload, not the channel.** Every event carries | ||
| `workspaceId`; subscribers filter (`options.filter` or in-handler). This keeps |
There was a problem hiding this comment.
This blanket rule conflicts with the inventory/contract that app:booted is session-global and sync:status is PowerSync-wide. If implementers follow this literally, those sticky global events must fabricate a workspace id and can replay the initial/current workspace after a switch, reintroducing the stale-scope bug the table explicitly avoids; narrow this paragraph to workspace-scoped events only.
Useful? React with 👍 / 👎.
…per-row block events) Codex on 9407557, both verified real: - §3.4 said 'Every event carries workspaceId' — contradicts app:booted / sync:status, which were made session/connection-global (no workspaceId). Narrowed to workspace-scoped events. - §3.5 said block:* 'fire exactly once per committed tx' — wrong for a multi-block tx: the SnapshotsMap has one entry per changed row and the payloads are singular, so the bridge emits one event per changed block (each classified into exactly one of created/updated/deleted), not once per tx. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28706d7547
ℹ️ 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".
| export type AppEventName = keyof AppEventRegistry & string | ||
| export type AppEventPayload<N extends AppEventName> = AppEventRegistry[N] |
There was a problem hiding this comment.
In the dynamic-plugin case described just below, this makes AppEventName only the merged registry keys, and emit/on<N extends AppEventName> therefore reject any unaugmented event name at compile time instead of falling back to an unknown payload. The existing same-tx pattern this claims to mirror uses a P extends string conditional payload fallback, so copying this sketch would force dynamic plugins into unsafe casts or prevent their events entirely.
Useful? React with 👍 / 👎.
| **Phase 2 — navigation + sync.** Bridge `navigation:completed` from | ||
| `navigationVerb.after` (an internal `.after` observer that calls `repo.events | ||
| .emit`), and `sync:synced`/`sync:status` from the PowerSync status listener + | ||
| `syncObserver` `applyOutcome`. Both are pure bridges over existing observer |
There was a problem hiding this comment.
This phase plan asks implementers to bridge sync:synced, but the inventory defines the down-sync block event as block:synced with {workspaceId, ids} and all the grouping guidance is written for that name. If implemented literally, subscribers following the inventory would never see the Phase 2 event (or the codebase would grow two names for the same sync-applied block signal), so the plan should use the same event name throughout.
Useful? React with 👍 / 👎.
…wn-sync event name Codex on 28706d7, both verified: - §3.1/§3.2 type sketch: AppEventPayload<N extends AppEventName> + emit/on<N extends AppEventName> constrained N to registry keys, so a dynamic plugin's unaugmented event name would be a compile error — contradicting the 'dynamic plugins fall back to unknown' claim. Changed to the P-extends-string conditional-fallback pattern the doc says it mirrors (SameTxEventPayload / tx.emitEvent<P extends string>). - Phase 2 said 'sync:synced' but the inventory's down-sync event is 'block:synced' — unified to block:synced (+ sync:status), one name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…nt-shaped
The user asked to fix all Codex findings (done — every thread is resolved/
outdated) and reflect on a deeper architectural source. Added §5:
- 5.1: the majority of review churn orbited making workspace:ready a
sticky/replayable EVENT. That need (run X per active+ready workspace,
undo on switch) is EFFECT-shaped, and the machinery already exists:
AppRuntimeProvider (→ EffectReconciler) renders only in the {kind:'ready'}
branch post-bootstrap (App.tsx:306) and restarts effects on workspaceId
change. So an AppEffect.start({repo, workspaceId}) already IS the I3
signal — no event/sticky/replay/gate. Revised the thin slice to a
workspace-scoped effect; the bus stays for genuine broadcast (block:*,
navigation, sync).
- 5.2: 'emit from any existing choke' is the wrong default — non-data
lifecycle has no single funnel (unlike repo.tx for data), which is the
timing/missing-path bug source and is itself gap I3. Principle: canonical
per-event chokepoint; give lifecycle an owner.
- 5.3: prose-over-a-matrix caused the consistency drift; make the event set
one structured registry with scope/sticky as typed fields and payloads
derived from data-model types; share one async-observer primitive.
Updated the BLUF, §4.3, §3.5 sticky lead, and the workspace:ready row to
point at §5.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014iSKcoucAUf6LAmfDNhFyK
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
What
Investigation-only design doc (
docs/domain-event-lifecycle-bus.md) for the typed domain-event / lifecycle bus gap: the general seam for plugins/extensions to react to application events. No code changes.Today the only reaction surface is data-layer post-commit processors — data-only, post-commit, can't veto, not React-subscribable for pure observers, and blind to non-data events (navigation, workspace switch, app lifecycle, sync).
Deliverables (per the brief)
block:created/updated/deleted(filterable by type),navigation:completed/requested,workspace:switched/created,sync:status/block:synced,app:booted,runtime:swapped— each mapped to an existing choke so there's no call-site convergence cost.before/afterobservers, app effects +LiveRuntimeHandle, toggle stores /useHandle; what each covers and what's missing.AppEventRegistry(same pattern asPostCommitProcessorRegistry/SameTxEventRegistry),repo.events.emit/on, auseAppEventhook, subscriptions hosted in app effects, workspace scope carried in the payload, and the verb-observer delivery contract (async, sequential, error-isolated, observe-only / no veto). Includes an explicit table of how it stays within the B3 typed-channel rules.Notable findings vs the earlier gap docs
defineVerbFacet+navigationVerb.before/afterhave since landed (PR feat(navigation): navigationVerb intent seam + robust defineVerbFacet #241), so the navigation event already has an observer slot to bridge from.sameTxProcessoralready ships a declaration-merged typed event registry (SameTxEventRegistry+tx.emitEvent) — a typed event registry is the established house style.workspace:switched(Repo.setActiveWorkspaceId) is the one event with no observability at all and real demand (gap I3) → recommended as the smallest first thin slice (one emit line, two-string payload, closes the I3 race).Verification
Docs-only change (single new markdown file); no effect on typecheck/lint/tests, so the full
yarn run checkwas not run.🤖 Generated with Claude Code
Generated by Claude Code