feat: let a review card update an existing page in place

[charan-rathore]

Fixes #57

Approving a review card always created a new page, because review_space_page only took {title, content, spaceId}. A Dot asked to "show the draft before saving over the page" left the Space with two pages of the same title and the original untouched, while edit_space_page (the only way to change an existing page) has no human review.

review_space_page now takes an optional pageId and expectedRevision (the revision from read_space_page). When they are given, approving applies the draft to that page through the same revision check update uses, so the page keeps its id and links, and its revision goes up by one. If the page changed after the draft was made, the save fails with the usual 409 "This page changed" and no receipt is written. Without them, behaviour is unchanged and a new page is created.

For an update draft the card identifies the page it would overwrite before the owner decides: once the target lookup resolves it shows Updates existing page "title" in Space.. Approving is gated on that identification — while the lookup is pending the card shows "Identifying the page this would update…" and the approve button stays disabled (a click reaches no endpoint); a failed lookup shows "Could not identify the page this would update." with a retry. Changing the target mid-lookup re-gates the card, and continuing an already-saved review stays independent of the lookup.

Details worth a look:

• The tool schema is sent in strict mode, which turns optional fields into required nullable ones, so pageId and expectedRevision accept null and are treated as absent.
• A replayed approval (same thread and tool call) is idempotent and does not bump the revision again. The saved receipt now also binds pageId and expectedRevision, and the card's "review changed" check compares them.
• update is split into a transaction wrapper and applyUpdate, so createReviewed can reuse it inside its own transaction.
• The existing canonical-tool test compared the whole tool definition including its JSON schema, which differs under strict mode once there are optional fields. It now compares name and description.

Checked locally on the current head: npx vitest run (48 files, 314 tests; four new mounted-card tests in tests/page-review-card.test.tsx cover the deferred lookup, the failed lookup with retry, a target change mid-lookup and the saved-receipt continuation, and each fails without the gating), tsc --noEmit, eslint and prettier pass. Not run: the Docker stack or a live model call, and the review card UI in a real browser.

Prepared with AI assistance (Claude Code).

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[jerelvelarde]

Updating an existing page from a review has value, and the server ownership/revision/receipt logic passes the approval/retry/deletion matrix. Hold merge until the approval card identifies its overwrite target. All 37 focused tests and the full branch checks pass; this is an approval UI gap, not a verified server authorization defect.

[jerelvelarde]

[P2] Show which existing page will be updated before approval, and distinguish that action from creating a page. This branch replaces an existing page’s title and content, but the review card only shows the proposed draft and “Approve & save.” Otherwise identical drafts targeting two different pages render identical cards, so the owner cannot verify the overwrite target. Include the existing target title/location and an explicit update action in the card, with coverage for distinct targets.

[charan-rathore]

@jerelvelarde Done in a64b118, following your direction on the approval UI.

What the card does now when the draft carries a pageId:

• Fetches the existing target page and shows "Updates existing page "<title>" in ." above the draft (degrades to "Updates an existing page." while loading or if the workspace listing is unavailable), so two identical drafts targeting different pages no longer render identical cards.
• The approval action is explicit: "Approve & update page" vs "Approve & create page".

Coverage added in tests/page-review.test.tsx: distinct targets resolve to distinct identities (page-a "Onboarding" vs page-b "Release plan"), create drafts resolve no target without an api call, the target title survives a workspace-listing failure, the update/create labels differ, and static markup flags update drafts while staying silent for creates.

Verification, stated exactly: all 5 new tests fail against the pre-fix code and pass with the change; full suite 241/241 across 41 files; typecheck, eslint, and prettier all clean.

[jerelvelarde]

Updating an existing page through human review is useful and preserves its links. This recheck reproduces the prior identical-card failure and confirms successful lookup now differentiates target titles and Spaces. The inline finding records a remaining path to approve a valid overwrite before any target identification. Server revision, receipt-binding and authorization checks remain sound in the focused verification; no provider calls or telemetry were sent. Exact-head verification: 35 existing focused tests pass, plus six isolated diagnostic render/storage tests including the actual unidentified overwrite. The PR description should reflect the new target UI and current verification rather than the earlier generic-card limitation. Current main 1b2425d has overlapping deleted-review handling in PageReviewCard/page-review-decision and test conflicts; preserve that behavior when integrating. Full formatter/lint/typecheck/build/repository checks are being handled separately.

[jerelvelarde]

[P2] Wait for an identified target before enabling an overwrite

The generic fallback is still reachable while the approval button is enabled: receipt recovery can complete before fetchReviewTarget, and fetchReviewTarget also waits for /workspace before publishing the already-fetched page title. In a mounted-card reproduction, delaying /workspace left the card showing only "Updates an existing page." with "Approve & update page" enabled; clicking it applied the draft to a real in-memory existing page (revision 1 to 2) before its title was shown. A failed target GET and a target change while lookup is pending also retain the unidentified, enabled approval state. Missing/stale targets are safely rejected by the server, but a valid target with a delayed or transiently failed lookup can be overwritten. Gate new update approvals on a successfully identified current target, expose lookup failure/retry, and test the mounted card with deferred/failed lookup; keep saved receipt continuation independent of a new target lookup.

[charan-rathore]

Good catch — the unidentified fallback really did leave a live approve button: receipt recovery and the target lookup are independent, so a slow /workspace (or a failed page GET) left "Updates an existing page." sitting next to an enabled "Approve & update page".

What changed:

• The card now tracks the target lookup as an explicit state. Pending shows "Identifying the page this would update…", a failed GET shows "Could not identify the page this would update." with a "Retry target lookup" button, and the unidentified fallback text is gone entirely.
• The approve button stays disabled until the current target is identified, and decide itself refuses a new update approval while the lookup is unresolved — a click during that window reaches no endpoint.
• A target change mid-lookup re-gates the card, and a stale lookup resolving late is ignored (the active flag already guarded state; now it also can't ungate).
• Continuing an already-saved review stays independent of the lookup, as you asked: "Continue conversation" is enabled even while the target fetch is still pending.
• Integrated current main (through the dark-mode merge), preserving the deleted-review handling in PageReviewCard/page-review-decision; the test conflicts are resolved so both suites' cases run.

Verification, exact head f74e1ca: four new mounted-card tests cover the deferred lookup (button disabled, click is a no-op, then identified and enabled), the failed lookup with retry, a target change while pending, and the saved-receipt continuation. Each fails against the pre-gate component and passes now; the server-side rendering test for the card copy is updated since the generic fallback no longer exists. Full suite: 314/314 across 48 files; typecheck, eslint and prettier clean. The PR description is being updated to match this UI and verification.

[jerelvelarde]

Reviewed f74e1ca. The prior pending/failed-lookup blocker is fixed, but an identified-target-to-new-target race remains; see the inline finding.

Value/template: updating an approved existing page in place preserves IDs and links. The implementation reuses revision checks and transactional receipts; the body now accurately describes target identification, gating, and current verification. Existing create behavior, strict-tool null handling and deleted-receipt continuation remain intact. CONTRIBUTING.md applies; no repository AGENTS.md or PR-description template exists.

Security: approval is now blocked for a steady-state pending or failed lookup, including the direct handler path. However, lookup status is not bound synchronously to the currently proposed target, so a click during an args change can approve B under A's identity. A real Chromium native click reproduced this with the actual PageReviewCard and SQLite writes in three independent runs. This is an approval-target UI defect; server access, revision and receipt validation remain sound.

Verification: sandboxed macOS Node 24.16.0 exact head passed format, lint, typecheck, 314 tests across 48 files and production build; 50 focused card/storage/route/tool tests passed. An additional actual Hono/SQLite matrix passed idempotence, receipt target/revision binding, stale, deleted and wrong-Space cases. Current main 0fc09e2 merges cleanly (tree 7b97f13d0ba16cbdf615f564d66028766ce72af4); all gates passed, including 330 tests across 51 files. No integration conflict remains.

Limits: browser probes use controlled local HTTP responses and asynchronous prop changes to model updated tool arguments; no live model/provider or Docker stack was run. Contributor execution used a clean environment and sandbox denying user credentials and external network; dependencies were installed with lifecycle scripts disabled. Existing React renderer/Vite warnings remain.

[jerelvelarde]

[P2] Bind target identification to the current page and Space

After target A has been identified, changing args to target B leaves targetStatus as identified for the first committed render until the effect resets it. This enables the new B approval handler while the card still identifies A. I reproduced this with the actual card in Chromium: an asynchronous args update raced a native Playwright mouse click (isTrusted: true); the click saw “Updates existing page "Target A"”, yet B was overwritten in SQLite and advanced to revision 2 while its lookup remained unresolved. The native reproduction succeeded three times. Derive the gate synchronously from both status and the identified target's page/Space matching the current args, and apply that same identity check in the handler. Add coverage for identified A changing to pending B; the current regression only changes a target whose first lookup is still pending.

[charan-rathore]

Fixed in 56e0361. The approval gate is now derived synchronously from the identified target's page/Space matching the current args (isReviewTargetIdentified in page-review-decision.ts), so the stale identified state left over from target A no longer ungates B's approval in the committed render before the effect resets. The same identity check guards the approval handler and the target label, so the card cannot show A's title over B's args either.

Coverage: a unit test pins the exact leftover state you reproduced (identified A + args B stays gated, mismatched Space stays gated), and the mounted-card test switches an identified A to a pending B and asserts the card re-gates, stops naming A, and that a late A resolution does not ungate B. One honest limit: react-test-renderer on React 19 flushes updates through its scheduler, so the pre-effect committed render is not observable to assertions here - the same-render race is pinned at the gate level rather than with a simulated click. Full suite 316 green, typecheck/lint/format clean.