feat(react): publish native Cezar cockpit facade - #931
andrzejewsky wants to merge 35 commits into
Conversation
|
🤖 om-auto-review-pr — re-review result Decision: CHANGES REQUESTED The source-level release defect found in this review is fixed in Validation passed locally: Remaining blocker is external CI configuration: Publish npm snapshot fails while publishing the first package, GitHub does not allow an author to submit a request-changes review on their own PR, so this disposition is recorded by this comment and the |
|
🤖 Source review is clean after |
|
🤖 Note: the only pre-existing claim signal was the author assignee left by the previous run's changes-requested handoff (that run posted |
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review
🎯 Summary
PR #931 introduces @open-mercato/cezar-react — a coarse, publishable facade that composes the existing cockpit for iframe-free embedding — and, to make that installable, flips @open-mercato/cezar-contract and @open-mercato/cezar-api-client from private to public. The engineering underneath is strong: the instance-scoped client, the provider-owned portal surface, the adopted-root appearance contract that restores exactly what it changed, the CSS scoper with its own verifier, and roughly 1,400 lines of new focused tests. The release-set defect from the previous review pass is genuinely fixed in ff81d86a — react is now stamped, dependency-pinned, and published in both channels, with tests covering it.
What is not ready is the delivery around that code. Three brand-new verification gates ship without ever being wired into CI; the CI job that vouches for publishable tarballs still vouches for the old two-package release set; the package advertises two public subpath exports that are empty files; and publishing three new packages into a scope the CI token cannot write to turns a required check red for every pull request in this repository, not just this one. None of these are in the facade itself — they are in the contract this PR is asking the project to take on permanently.
🧭 Verdict
Request changes. Two blockers (the publish failure that breaks CI for every future PR and the stable release path, and the missing compatibility record for three newly-public packages) and six majors (two unenforced gates, a stale release-pack gate, two empty public exports, and two runtime-correctness issues in the new public API).
🧪 Validation Gate
Run in an isolated worktree at the PR head ff81d86a, npm ci restored from the lockfile.
| Command | Result | Evidence |
|---|---|---|
npm run typecheck |
✅ Pass | Contract, API client, React, service, and web typechecks all clean. See the tsconfig.test.json finding below for what this gate does not cover. |
npm test |
8 failed / 6275 passed on the first run, 12 failed / 6271 passed on an identical re-run: non-deterministic, and no failing file appears in this diff. The cause is my environment, not the code: os.tmpdir() here resolves to /home/cezar/cezar/.ai/cezar/tmp/…, which is inside the repository, so every test that builds a temp dir and asserts the absence of a git repo fails on a git rev-parse that walks up and finds one — git.test.ts "returns null outside a git repository", git-worktree.test.ts "answers null when the path is not a git worktree", git-changes.test.ts "commitAll on a non-repo dir", and the health/parity suites that boot a server in such a dir. The rest are ENOENT races from RunManager.rescueStalledQueue writing into already-torn-down temp dirs. CI's equivalent job is green on this head. I am recording this so the gate result is honest, not as a finding. |
|
npm run test:unit |
✅ Pass | 36 passed, 0 failed. |
npm run build |
✅ Pass | All five workspaces built; scope-css, verify-css, verify-cockpit-artifact and check:pack all ran and passed (check:pack ok — 475 files, 85 under web/dist). |
npm run test:package |
✅ Pass | 15 passed, including the new release-order coverage. |
⛔ Blockers
-
[blocker]
.github/workflows/ci.yml:71— publishing three new packages into an unwritable npm scope breaks the snapshot job for every pull request in the repository, and breaks the stable release path too.Publish npm snapshotis red on this head:npm publishreturnsE404for@open-mercato/cezar-contractafter provenance creation, because the Actions token has no create/publish rights on the@open-mercatoscope.One correction to the previous review pass on this PR, which called this a required check: it is not.
mainhas no required status checks — the only ruleset on it ispull_requestwithrequired_approving_review_count: 1, which is whymergeStateStatusreadsBLOCKED. So this failure does not itself gate the merge button.It is still a blocker, for a larger reason than this PR:
publish-snapshotruns on every same-repo pull request (github.event.pull_request.head.repo.full_name == github.repository), andscripts/release-snapshot.mjs:113degrades to a dry run only whenNPM_TOKENis absent — never when the token is present but lacks scope rights. The same is true ofscripts/release.mjs:99. So merging as-is leaves every future PR in this repo with a red CI badge and leaves the maintainer's stablenpm run releasefailing on the first package it tries to publish, until an npm organization owner grants scope-level access. Either land the org permission before merging, or teach both scripts to treat anE404/E403on a not-yet-created scoped package as the same loud dry run they already have for a missing token.docs/publishing.mdstep 3 documents the manual setup, but nothing in code degrades without it. -
[blocker]
BACKWARD_COMPATIBILITY.md— three packages become public npm surfaces and the compatibility document gains no section, which §6 explicitly requires before this can happen. Section 6 says of the library surface: "There is noexports/library API — the package is CLI-only. Keep it that way deliberately: adding one creates a new compatibility surface; if it happens, this document gains a section first." This PR creates three such surfaces at once —@open-mercato/cezar-contract,@open-mercato/cezar-api-client(both flipped out ofprivate), and@open-mercato/cezar-reactwith its four subpath exports andstyles.css— and the document is untouched. Compounding it,docs/publishing.md:30deletes the recorded rationale for keeping the api-client private ("it still carries the hand-written DTOs, which shrink family by family as routes are converted, so publishing now would advertise a contract that changes materially every release") and replaces it with a statement that the set is public, without arguing anywhere that the stated precondition — the surface has stopped moving — is now met. The api-client still carries those hand-written DTOs on this branch.CODE_REVIEW.md's severity guidance makes breaking aBACKWARD_COMPATIBILITY.mdsurface without the required path a blocker. Required path: add the section describing what is now frozen in each of the three packages (theexportsmaps,CezarClient/CezarProvider/CezarCockpitprop shapes, thestyles.cssentry, the.cezar-rootclass anddata-cezar-*attribute contract), and either argue the api-client's surface has settled or say explicitly which parts are provisional.
⚠️ Majors
-
[major]
.github/workflows/ci.yml(unchanged) vspackage.json:38—npm run test:cockpit-packageis never run by anything. The PR adds 533 lines of packed-consumer verification (scripts/check-cockpit-pack.mjs+scripts/check-cockpit-pack.test.mjs) and afixtures/cockpit-consumerproject, wires them to atest:cockpit-packagescript, and then never references that script from a workflow, fromnpm run build, fromtest:package, or from.ai/agentic.config.json's validation gate. This is the one gate that exercises the PR's actual headline delivery path — pack the tarballs, cold-install them in a fresh consumer, typecheck and Vite-build it, and scan the installed runtime for#cezar-web-cockpit/packages/web/cockpit-implementationleaking into shipped.jsand.d.ts(scripts/check-cockpit-pack.mjs:26). I ran it by hand in the review worktree and it passes cleanly —cold cockpit consumer ok — 3 tarballs installed, 1 typecheck, 1 Vite build, 4 fonts resolved, 70 runtime/declaration files scanned— and the builtdist/*.d.tscarry no private markers. That is exactly why it needs to run automatically: it is correct and green today, and nothing will notice the day it stops being. Add it as a CI step. -
[major]
packages/react/package.json:52—check:boundariesis likewise never run.scripts/check-import-boundaries.mjswalkssrc/and rejects imports ofpackages/web,@/…,@open-mercato/cezar-contract, andnode:builtins — the invariant that keeps the published package from reaching into the private app or into Node. Its unit test (check-import-boundaries.test.mjs) is picked up by the react vitest config, but that test only exercisesfindProhibitedSpecifiersagainst string fixtures; it never scans the real source tree. So the guard is tested and not applied. It passes when run by hand (npm run check:boundaries -w @open-mercato/cezar-react, exit 0) — same argument as above: wire it intobuildor CI. -
[major]
.github/workflows/ci.yml:60-68— the "Verify release packages" step still vouches for the old two-package release set. It packs only@open-mercato/cezarandalias-cezar, and its comment still explains the omission as "the packages marked private (the cockpit SPA and, for now, the api-client)". After this PR three more packages publish, each with a hand-writtenfiles/exports/main/typesblock that has never been pack-verified. A wrongfilesentry inpackages/react/package.json:9(["dist", "licenses", "README.md"]) or a staleexportstarget would now be discovered by the publish step rather than by the gate that exists to catch it. Extend the step tonpm pack --dry-runthe contract, api-client and react workspaces, and update the comment, which is now factually wrong. -
[major]
packages/react/package.json:14-15—./tasksand./sessionare published subpath exports whose entry files are empty.packages/react/src/tasks.tsandsrc/session.tsare both literallyexport {};vite.config.ts:41-42builds them as entry points, and the built artifacts confirm it —dist/tasks.jsanddist/session.jsare 0 bytes,dist/tasks.d.tsanddist/session.d.tsareexport {};. A consumer writingimport { … } from '@open-mercato/cezar-react/tasks'gets a silently-successful import that resolves nothing. Publishedexportsentries are precisely the surface the blocker above is about: once a consumer depends on the path, removing it is a breaking change with a deprecation window. Either drop both fromexports,vite.config.tsand the source tree until they have content, or make them throw a named "not implemented yet" error so the failure is loud. -
[major]
packages/react/src/cockpit.tsx:48-52—useLegacyTransportBaseUrlmutates module-level state during render.activeLegacyTransportOwner = owner.currentandsetApiBaseUrl(baseUrl)run in the hook body, not in an effect. Two consequences. First, render-phase side effects are unsafe under React 19: a render that is discarded (Suspense, an interrupted concurrent render, StrictMode's double invoke) still mutates the module global, so the process-wide API base can be left pointing at a client whose tree never committed. Second — and this is the one an embedder will hit — the global is last-render-wins, so with twoCezarCockpitinstances mounted (the multi-sandbox layout the PR description itself describes), any re-render of instance A silently repoints the private composition's HTTP and workspace-SSE URLs at A's origin while B's queries are in flight. The PR notes and the README both acknowledge that only one instance is supported, but nothing in the code enforces or warns about it. At minimum: move the mutation into a layout effect, and emit a development-mode warning when a second owner claims the lease, so the failure is diagnosable instead of presenting as B fetching A's data. -
[major]
packages/api-client/src/client.ts:96andpackages/react/src/core/storage.ts:7—CezarClient.identityis a construction-order counter, and it namespaceslocalStoragekeys.identityiscezar-client-${++nextClientIdentity}off a module-level counter, andcreateCezarBrowserStoragebuilds keys ascezar:${identity}:${projectId}:${key}. Failure scenario: a host mounts cockpit A (identitycezar-client-1) then cockpit B (cezar-client-2), and each writes a draft throughuseCezarRuntime().storage. On the next page load the host renders B first — B is nowcezar-client-1and reads A's persisted draft, while A's own values are unreachable. Nothing about the identity is tied to the authority it actually represents. The tests do not catch this because they constructfakeCezarClient('client-a')with a literal identity (packages/react/src/core/provider.test.tsx:331), so the realidentityis never exercised — andprovider.test.tsx:212asserts a "stable cache namespace" against that fake, which reads as coverage for exactly the property the implementation does not have. Deriveidentityfrom something stable and meaningful — the normalizedbaseUrlplus the credential mode — and keep the counter only as a disambiguator for two clients that are genuinely identical.
🔽 Minors
-
[minor]
packages/react/tsconfig.test.json:6— the typecheck gate skips every.tsxtest file."include": ["src/**/*.ts"]with"exclude": []is unmistakably intended to add test files back into the check, but TypeScript's*.tsglob does not match.tsx, sosrc/cockpit.test.tsx(333 lines) andsrc/core/provider.test.tsx(406 lines) are outsidenpm run typecheck. Confirmed withtsc --noEmit -p tsconfig.test.json --listFiles: zero.test.tsxfiles in the program. Change the include to["src/**/*.ts", "src/**/*.tsx"]. -
[minor]
packages/react/src/styles/base.cssandpackages/react/src/styles/tokens.cssare unreferenced. 121 lines defining a--cezar-*token system and a.cezar-rootpreflight block, and nothing imports them —src/styles/index.cssonly pulls in../../../web/src/styles/index.css. The shipped stylesheet uses the web app's--background/--foregroundnames, not--cezar-background, so these are a competing token vocabulary left over from an earlier approach. Please confirm the drop was intentional (the root element does lose Tailwind preflight'sbox-sizing/border-color, whichbase.csswas written to restore — harmless today because the root carries no padding or border, but it is the kind of thing that is easier to reason about deleted than dormant) and remove them. -
[minor]
packages/react/package.json:24andpackages/react/vite.config.ts:14-17—react-routeris a declared runtime dependency but is bundled intodist.isReactPackageExternalexternalizes onlyreact,react-dom,@open-mercato/cezar-api-clientand@tanstack/react-query, so react-router is inlined into the artifact; the builtdist/cockpit.jshas no barereact-routerimport. Every consumer therefore installs a copy of react-router that is never loaded. Either add it toruntimeDependencies(and accept host dedup, which changes the router-isolation story) or remove it fromdependencies. -
[minor]
packages/api-client/src/subscriptions/run-events.ts:22—maxEventsis part of the publicRunEventSubscriptionOptionsbutsubscribeRunignores it. The destructure at line 96 omits it; only the privateuseRunEventsadapter slices. A consumer settingmaxEventson the public API gets no bound. Honor it in the subscription or move it to the hook's own options type. -
[minor]
packages/react/src/core/storage.ts:12-18— the localStorage guard covers access but not the operations.localStorageWhenAvailable()try/catches readingglobalThis.localStorage, thensetItemis called unguarded. In the third-party embedding contexts this package exists for,setItemthrowsQuotaExceededError, and Safari/ITP and some partitioned-storage configurations throwSecurityErroron the operation rather than on the property access. An uncaught throw from a storage write will take down the host's render. Wrap the three operations. -
[minor]
packages/api-client/src/domains/runs.ts:50—runs.listvalidates the run list all-or-nothing, and version skew is now possible for the first time.getRunspreviously went throughunwrap, which casts without validating (packages/web/src/api/client.ts:306); it now goes throughrequestJson(apiRunSchema.array(), …). Validating is the right direction and matchesCODE_REVIEW.md's "zod at every boundary" — the granularity is the issue.apiRunSchemais the fatrunRecordSchema, and one record that fails to parse now rejects the entire array, so the task list errors out instead of rendering the other 200 runs. That was acceptable while the client only ever shipped in lockstep with its server; once this package is published and version-pinned, an older client against a newer server is a real combination, andBACKWARD_COMPATIBILITY.md§3/§9's established posture for exactly this is per-entry salvage ("a corrupt registry entry is dropped per-entry, never the whole array"). Consider parsing per element and dropping the unparseable ones. -
[minor]
packages/web/src/api/run-events.ts:36— a fresh client is constructed on every effect run.createCezarClient({ baseUrl: getApiBaseUrl() })inside theuseEffectmeans everyrunId/option change allocates a new client and increments the module-level identity counter. Harmless whileidentityis unused here, but it compounds the finding above; hoisting it to auseMemokeyed on the base URL would be equivalent and cheaper. -
[minor]
.github/workflows/ci.yml:118andscripts/release-snapshot.mjs:160— the npm preview comment never names the package this PR exists to ship. The emitted result carriesrootName,apiClientNameandaliasNameonly, and the sticky PR comment renders "Packages:alias→root→apiClient" with install lines built from the alias alone.@open-mercato/cezar-contractand@open-mercato/cezar-reactare published but invisible in the preview, so a reviewer cannot find the snapshot version of the facade.publishedNamesis already computed — surface it. -
[minor] Scope: the
notifications-section.tsxsave-race fix is unrelated to the facade.packages/web/src/routes/settings/notifications-section.tsx:38-62adds apendingEnabledguard pluscancelQueriesto stop a stale server read from clobbering an optimistic toggle. It looks correct, but it is an independent bug fix inside a 105-file, 7,195-line PR, where it will not be found again by anyone reading the history for that bug.
📝 Nits
packages/react/src/core/provider.tsx:154-155andpackages/web/src/cockpit-implementation.tsx:54-55assignonErrorRef.current = onErrorduring render. It is a common idiom, but React's guidance is not to write refs while rendering; a layout effect oruseEffectEventsays the same thing without the caveat.packages/api-client/src/client.ts:216—resolveProjectUrlclassifies a protocol-relative//host/pathas relative (theabsolutetest requires a scheme), sonew URLresolves the host and the function then rebuilds the URL againstbaseUrl, silently dropping it. No caller produces one today.packages/react/distis 3.0 MB (2.8 MB of itassets/, including four.woff2faces) anddist/styles.cssis 134 KB. Worth stating in the README so an embedder knows the cost up front.
💥 Breaking-Changes Checklist
- No CLI command, flag, alias, env var, or exit code changed (
BACKWARD_COMPATIBILITY.md§1). - No
/api/v1route, response shape, or SSE event name changed;packages/web/src/api/run-events.tsmoves the subscription mechanics into the api-client while preserving the wire contract — both event names,seq > maxSeqreplay dedup, theafterSeq/cursorresume rule, the 40 s liveness watchdog, theCLOSED-only reopen, and the pagehide/pageshow/visibility handling all survive the move (packages/api-client/src/subscriptions/run-events.ts). - Zod validation at the boundary is preserved and in places tightened —
getRunHistory/getRunHistoryContextkeep their schemas viarequestJson(schema, …), andruns.listnow validates whereunwrappreviously did not. - No
.ai/cezar/or~/.cezar/state file shape changed. - Three packages become public npm surfaces with no entry in
BACKWARD_COMPATIBILITY.md— the blocker above. Per that document's §6 the section is required before the surface exists, and perCODE_REVIEW.mdthis is a blocker rather than a follow-up. - Two of the new public
exportspaths resolve to empty modules, committing the project to entry points that do nothing (major above).
🧪 Test Coverage
Coverage of the new code is genuinely good and I want to be clear about that: cockpit.test.tsx covers memory-routing containment, controlled-path echo, search/hash preservation across redirects, owned-vs-supplied query-client lifetimes including the StrictMode replay, and the error-boundary fallback; provider.test.tsx covers root adoption and exact restoration, appearance isolation, the system-theme listener, scoped error reporting, and the link/button navigation fallback; scope-css.test.ts, verify-css.test.ts and styles-source.test.ts pin the stylesheet transform; the release change ships tests in both snapshot.test.ts and stable.test.ts; and cockpit-facade.e2e.ts proves the standalone app really goes through the facade with no iframes.
Three gaps, in order of consequence:
- The packed-consumer path has tests but no gate.
scripts/check-cockpit-pack.test.mjsand the fixture exist; nothing runs them (major above). The strongest test in this PR is the one that never executes. - No test covers two concurrent
CezarCockpitinstances. That is the documented limitation and the documented use case (the PR body's sandbox integration mounts one cockpit per workspace), so a test pinning the current single-owner behavior — even as an explicit "this is what happens today" assertion — would keep the boundary honest. - The identity/storage tests assert against a literal fake. As noted above,
fakeCezarClient('client-a')means the counter-derivedidentityis never exercised; a test constructing two real clients and checking their storage keys is what would have caught it.
|
🤖
|
|
Thanks @andrzejewsky — review found actionable items, so I'm handing this PR back to you for the next pass. When the updates are pushed, re-request review and the automation can pick it up from the latest head. Two of the findings need a decision rather than a patch, so flagging them here:
The facade code itself reads well and the test coverage is genuinely strong; most of the remaining items are small and mechanical (wiring |
|
🤖 Reviewed Local validation: autofix: skipped (not my PR — re-run with --autofix to fix it here). |
|
Addressed the Aug 28 review in Delivery/public surface:
Runtime correctness:
Two review points were adjudicated rather than changed blindly:
Validation:
The full suite reaches 6,278/6,287 passing locally; the remaining 9 are the same nondeterministic temp-directory teardown races in the three untouched server suites called out in the review ( |
|
Follow-up pushed in |
|
CI follow-up: the complete code/package gate is green (typecheck, unit + server/cockpit tests, build, cold installed consumer, packaged CLI E2E, and all release tarballs). The separate snapshot job reaches npm and then fails on the first new package with |
📦 npm preview published —
|
|
🤖 Note: the only pre-existing claim signal was the author assignee left by this skill's own 2026-08-28 changes-requested handoff (that run posted |
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Re-review: feat(react): publish native Cezar cockpit facade
Reviewed a6fc6d31 in an isolated worktree (npm ci from the lockfile), against the Aug 28 pass on ff81d86a.
🎯 Summary
Every finding from the previous review is fixed, and the source-level review is now clean at major-and-above. Both blockers are cleared, all six majors are cleared, all seven minors and all three nits are cleared, and the three test-coverage gaps I named are closed by tests that target exactly the properties that were unpinned. The fixes are not cosmetic — the client identity is genuinely derived, the packed-consumer gate genuinely runs in CI, and the compatibility document genuinely describes the surface being frozen.
What now blocks the merge is not the code: main released 0.10.1 (#962) while this branch sat at 0.10.0, so the head no longer merges. That is the one blocker, and resolving it is more than a two-file fixup because this PR turns the release set into five lockstep-versioned packages.
🧭 Verdict
Request changes — one blocker, the unresolved conflict with main. No majors. Four minors, none of which need to hold the PR if you would rather take them as follow-ups; I would fix the first one in this pass because it undermines the diagnostic the previous review asked for.
⛔ Blocker
-
[blocker] The head conflicts with
mainand cannot merge (mergeable: CONFLICTING,mergeStateStatus: DIRTY). Conflicting paths:package-lock.jsonandpackages/web/package.json.The cause is a version-bump collision, not a code collision:
e8c95f3a chore(release): bump main to 0.10.1 and cut the changelog entry (#962)movedmain's whole release set to0.10.1, while this branch still carries0.10.0in all six manifests (packages/contract,packages/api-client,packages/react,packages/cezar,packages/web,alias-cezar) with^0.10.0intra-release pins.Please merge
mainforward rather than resolving only the two conflicted files. Takingmain's side onpackages/web/package.jsonwould drop the new"@open-mercato/cezar-react"dependency this PR adds; taking this branch's side would roll the version back to 0.10.0. The correct resolution stamps all five release manifests plusalias-cezarto 0.10.1 and rewrites every intra-release pin (cezar-contract← api-client,cezar-api-client← react and web,cezar-react← web,cezar← alias) to^0.10.1, then regeneratespackage-lock.jsonwithnpm install.packages/contractandpackages/reactare new in this PR and have never been through a bump, so they are the two most likely to be missed.Caveat, stated plainly: I reviewed the diff as pushed. The forward-merge touches manifests and the lockfile, so the release-order tests in
packages/cezar/test/e2e/release-snapshot.test.tsandstable.test.tsare worth a second look once it lands.
✅ Previous findings — disposition
Both blockers resolved:
- npm scope permission. Cleared by the route I flagged as the alternative to code changes: the
@open-mercatoscope grant has landed.Publish npm snapshotis green on this exact head (run33427089501, job99962821679) and actually published0.10.0-pr931.1376.2, so the E404 on@open-mercato/cezar-contractis gone and the stablenpm run releasepath is unblocked. See minor 3 for the residue. BACKWARD_COMPATIBILITY.md. §6b is added and does the job properly — it names the frozen exports per package, theCezarProvider/CezarCockpitprop shapes, theCezarCockpitRoutingmodes, the stylesheet entry, the.cezar-rootclass and all sixdata-cezar-*hooks, states what is not contract (hashed chunks, Tailwind classes, markup below the root), gives the pre-1.0 and post-1.0 change paths, and pinsnpm run test:cockpit-packageas a gate that must stay in CI. §6a's stale "there is noexports/library API" claim is corrected accurately (./app-typereally is the service package's only library export;.is the shebang CLI entry).docs/publishing.mdnow argues the public set coherently instead of just asserting it.
All six majors resolved, each verified rather than taken on trust:
test:cockpit-packagenow runs inci.yml,nightly.ymlandrelease.yml.check:boundariesis the first leg ofpackages/react'sbuild, which the rootbuildreaches viabuild:react— so it runs on every build, not just in CI.- "Verify release packages" packs all five publishable workspaces, and the misleading comment is corrected.
./tasksand./sessionare gone fromexports,vite.config.tsand the source tree — andassertReactTarballnow fails if they reappear, which is the better half of the fix.useLegacyTransportBaseUrlmoved intouseLayoutEffectwith a dev warning on lease contention.CezarClient.identityis derived from the normalized authority + credential mode + auth mode, with an explicitidentityescape hatch, documented in both §6b and the README.
Minors and nits: tsconfig.test.json now includes .tsx; the dead base.css/tokens.css are deleted; react-router moved to devDependencies; maxEvents moved out of the public RunEventSubscriptionOptions into the web adapter that actually implements it; browser storage wraps all three operations; runs.list salvages per entry; the cockpit's run-events client is cached per authority; publishedNames now drives the preview comment, the nightly summary and the release table; refs are assigned in a layout effect; protocol-relative URLs survive resolveProjectUrl; the README states the ~3 MB cost.
🔽 Minors
-
[minor]
packages/react/src/cockpit.tsx:49-58— the new lease-contention warning false-fires on the remount pattern this PR documents. The cleanup defers its reset intoqueueMicrotask, but React runs the outgoing tree's layout-effect destroy and the incoming tree's layout-effect create in the same commit. So on a keyed remount —key={JSON.stringify([sandboxId, origin])}, exactly what the PR description andpackages/react/README.mdtell hosts to do when switching sandboxes — the new instance's effect still sees the old instance's symbol inactiveLegacyTransportOwnerand warns.Reproduced in the review worktree with a scratch test: render
<CezarCockpit key="sandbox-a" …>, wait forgetApiBaseUrl()to behttps://first.example.test, rerender withkey="sandbox-b", wait forhttps://second.example.test— the base URL is correct throughout, andconsole.warnreceives "cezar: multiple CezarCockpit instances share a legacy transport; only the most recently mounted authority can be active" with exactly one cockpit mounted.Behavior is fine; the diagnostic is what breaks. The previous review asked for this warning so that two genuinely concurrent cockpits would be diagnosable instead of presenting as B fetching A's data. A warning that also fires on the documented single-instance happy path trains developers to ignore it, which costs the fix its value. Track live owners in a
Set(add on create, delete synchronously in cleanup, warn whensize > 1), or resetactiveLegacyTransportOwnersynchronously in cleanup and keepqueueMicrotaskonly for thesetApiBaseUrl(''). A test asserting silence on keyed remount would pin it. -
[minor]
packages/react/src/cockpit.tsx:48— moving the lease to a layout effect drops a render-time guarantee that one consumer still relies on. The comment previously read "Install the public client's authority synchronously so private render-time URL resolution sees it"; it now reads "before paint". That is a real weakening: during the embedded cockpit's first render pass,getApiBaseUrl()is still'', andpackages/web/src/components/zoomable-image.tsx:32callsresolveApiUrl(rawSrc)during render (packages/api-client/src/utils/project-scope.ts:144reads the module base). A URL resolved in that window would be root-relative — pointing at the host's origin rather than the Cezar origin.I could not construct a failure on this tree, and I want to be precise about why: both
ZoomableImagecall sites (routes/task-thread/thread-items.tsx:200,643andcomponents/diff/image-preview.tsx:49) are gated behind fetched data, so neither can render before the layout effect has run. The finding is that nothing pins that — the next render-time consumer ofresolveApiUrl/getApiBaseUrlthat is not data-gated will silently resolve against the wrong origin, and no test will notice. Either restore the render-phase install for the read path only, or state the "no render-time reads of the module base" invariant wheresetApiBaseUrlis defined and givecheck:boundariessomething to enforce. -
[minor]
scripts/release-snapshot.mjs:113andscripts/release.mjs:99still degrade only on a missingNPM_TOKEN, never on a token that lacks scope rights. Now that the scope grant has landed this is no longer blocking — but it is the same cliff, one credential rotation away. Both scripts hard-fail the job on anE403/E404fromnpm publish, which is what turned every PR in this repository red for five days. Treating a permissions error on a scoped package as the same loud dry run they already have for a missing token would make that failure mode self-describing instead of a red badge on unrelated PRs. Reasonable as a follow-up issue rather than a change here. -
[minor] Scope: the
notifications-section.tsxsave-race fix is still bundled.packages/web/src/routes/settings/notifications-section.tsxadds apendingEnabledguard pluscancelQueriesso a stale server read cannot clobber an optimistic toggle. It still looks correct and it is still an independent bug fix inside a 105-file, 7,200-line PR, where nobody reading the history for that bug will find it. Carried forward unchanged from the last pass — not worth a rebase now, just noting it stays true.
🧪 Validation Gate
Run in an isolated worktree at a6fc6d31, npm ci restored from the lockfile. All five commands pass.
| Command | Result | Evidence |
|---|---|---|
npm run typecheck |
✅ Pass | Contract, API client, React, service and web all clean — and the React leg now genuinely covers .tsx tests after the tsconfig.test.json fix. |
npm test |
✅ Pass | 343 files, 6324/6324 tests. First run showed 7 failures; all 7 were the sandbox artifact I recorded last time — os.tmpdir() resolves inside the repository here, so every test asserting "outside a git repository" finds one by walking up. Re-run with TMPDIR=/tmp/…: 0 failures. None of the 7 files appear in this diff. Recording the diagnosis, not a finding. |
npm run test:unit |
✅ Pass | 36 passed. |
npm run build |
✅ Pass | All five workspaces. check:boundaries now runs first in the React leg (exit 0); scope-css, verify-css, verify-cockpit-artifact and check:pack all pass. |
npm run test:package |
✅ Pass | 16 passed, up from 15 — including the rerun-attempt isolation added in 03272da4. |
CI on this head is fully green as well: Unit, build, E2E, and package, Publish npm snapshot, and license/cla all SUCCESS. No required check is failing and none is pending. main has no required status checks configured, so the merge button is gated by the review requirement, not by CI.
💥 Breaking-Changes Checklist
- No CLI command, flag, alias, env var or exit code changed (§1).
- No
/api/v1route, response shape or SSE event name changed; the run-event subscription move preserves the wire contract. - Zod validation at the boundary preserved, and
runs.listnow salvages per entry in line with §3/§9's established posture. - No
.ai/cezar/or~/.cezar/state file shape changed. - Three packages become public npm surfaces — now recorded in
BACKWARD_COMPATIBILITY.md§6b before the surface ships, which is the path §6 requires. Previously the second blocker; resolved. - No public
exportspath resolves to an empty module —./tasksand./sessionremoved, and the pack gate now fails if they return. Previously a major; resolved.
Two removals worth naming explicitly, both safe: ./tasks/./session and RunEventSubscriptionOptions.maxEvents were removed from packages that were private on main and have only ever been published under per-PR snapshot tags. No stable release advertised them, so no deprecation window is owed.
🧪 Test Coverage
All three gaps from the previous pass are closed, and closed at the right level:
- The packed-consumer path now has a gate, in
ci.yml,nightly.ymlandrelease.yml— the strongest test in this PR now actually executes. - Two concurrent cockpits are covered (
cockpit.test.tsx, "warns when a second cockpit contends for the legacy transport lease") — the documented limitation is pinned by an assertion instead of by prose. What it does not yet pin is the absence of that warning for one cockpit; see minor 1. - Identity is exercised against real clients (
client.test.ts, "derives a stable identity from the normalized authority and credential mode") — including trailing-slash normalization and credential-mode divergence, which is what the old fake could never have caught.
New tests are well targeted throughout: per-entry run salvage against a genuinely malformed row, storage degradation against a localStorage that throws SecurityError/QuotaExceededError on the operations rather than on property access, protocol-relative authority preservation, and a pack-gate assertion that the removed entries stay removed.
|
@andrzejewsky — this one is close. Every finding from the Aug 28 review is fixed, and I verified each rather than taking the commit message for it: the packed-consumer gate and the import-boundary scan really do run now, the derived client identity really is stable across trailing-slash normalization, The one blocker is the forward-merge, and it needs a little care: One thing I would fix in the same pass — minor 1 in the review: the new lease-contention warning fires on a single cockpit remounted by key, which is the sandbox-switching pattern your own PR description and README recommend. I reproduced it in the review worktree; behavior is correct, but a warning that cries wolf on the happy path is one developers learn to ignore, and it exists precisely to make the real two-instance bug diagnosable. The other three minors are genuinely optional and would be fine as follow-up issues. Push the update and re-request review. |
|
🤖 Re-reviewed One blocker remains: the head conflicts with autofix: skipped (not my PR — re-run with |
Summary
@open-mercato/cezar-react/cockpitfacade that composes the existing complete Cezar application instead of rebuilding features one by one.Motivation
Sandbox hosts need to embed the complete Cezar cockpit as React components without an iframe while preserving existing task, session, Git, skills, workflows, and settings functionality.
Sandbox usage
Import the stylesheet once in the host application, construct a credentialed client for the sandbox-specific Cezar origin, and use memory routing so Cezar navigation does not replace the outer sandbox URL.
Sandbox integration requirements:
baseUrlfrom the authorized Cezar app URL returned for that sandbox; do not use a process-global API URL.credentials: "include"so the sandbox access cookie is sent to HTTP and live-event endpoints.min-height: 0; the cockpit fills its parent.routing.mode: "memory"preserves the outer sandbox URL while the controlledpathcan be retained by the host.Validation
docs/publishing.mdfor the maintainer setup.Notes
mercato-sandboxesnative-cockpit integration.