Repository navigation
fix(canvas): publish button back in header, real Settings menu - #2286
Conversation
Canvas pages had publish hidden behind a "Settings" tab (PR #2250) that rendered nothing but a bare, unstyled Publish button. Restores publish to the page header like every other publishable page type, and rebuilds Settings into an actual per-canvas settings menu: - content-header/index.tsx: drop the canvas exclusion so HeaderPublishControls renders in the header for canvas pages too. - PublishControls.tsx: restyle raw <button> elements to the shared Button component; extract the SEO override form into PublishSettingsFields so it's reusable outside the post-publish dialog. - New canvas/settings/ folder: a category menu (styled like the Drive/User settings row list) — Publish, Appearance, Home & 404 page, Forms — each URL-addressable via ?tab=settings&category=... . Forms moves from its own tab into a category here. - New published_pages.themeBridgeEnabled column: lets a canvas opt out of PageSpace's forced light/dark override, threaded through the publish API, renderer, and the in-editor live preview. - Home/404 page assignment reuses the existing sidebar context-menu and domains-settings API calls rather than new plumbing. Tab bar: View | Code | Settings (down from View | Code | Forms | Settings).
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 972c0ceda7
ℹ️ 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".
…ions The publish route's JSON response now includes themeBridgeEnabled, which broke 4 exact-match toEqual() assertions in route.test.ts that didn't account for the new field. Fixed those, and added coverage for the new behavior: - publish-page.test.ts: themeBridgeEnabled threads into injectThemeBridge, defaults true on first publish, persists across a republish that omits it (mirrors the existing noindex coverage). - render-published.test.ts: injectThemeBridge defaults true; false omits the theme-bridge script from the rendered document. - route.test.ts: POST accepts themeBridgeEnabled and returns it. - PublishControls.test.tsx: new describe block smoke-tests the available-state buttons (Publish / Copy link / Unpublish) render via the Button-component refactor with working accessible names — previously only the unavailable-state branches had coverage.
… staleness Two real bugs flagged by the Codex automated review on the first commit: 1. CanvasPageView.tsx: `?tab=` was cast to CanvasTab without validation, so an old/foreign value (e.g. a bookmarked ?tab=forms from before the Forms tab was removed) matched none of the render branches and left the page blank. Now validated against the known tab list, falling back to 'view'. 2. CanvasPublishSettingsSection and CanvasAppearanceSettingsSection each independently fetched publish status once on mount, with no way to learn about a publish/unpublish action taken from the header while the category stayed open — leaving fields stuck disabled after a header-publish, or a stale-but-enabled Save button after a header-unpublish that would silently republish the page if clicked. Fixed by introducing usePublishStatusStore, a shared cache (keyed by pageId) that PublishControls (header), CanvasPublishSettingsSection, CanvasAppearanceSettingsSection, and CanvasPageView's View-tab preview all read/write through — an action taken on any one surface is immediately visible on the others, matching the existing useDocumentManagerStore / useDriveStore pattern for cross-component page state in this codebase. Net simplification too: removes three independent mount-time fetch effects in favor of one deduped fetch per pageId, and drops the onThemeBridgeEnabledChange prop that was threading the same information through props instead of the store. Also removed PublishSettingsFields.tsx's now-dead PublishOverrides export (superseded by the store's PublishSettingsSnapshot), and added usePublishStatusStore.test.ts covering fetch/cache/dedupe and — directly — the cross-reader sync that fixes bug 2.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb9a4ec097
ℹ️ 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".
Two more real bugs Codex caught on the re-review of the previous fix (commit eb9a4ec): 1. CanvasHomePageSettingsSection: toggling this page as the drive's home page updated the drive store but not usePublishStatusStore. Since a canvas page's published URL depends on isHomePage (root vs slug — see the publish GET route), the header could keep displaying and letting Copy link copy a URL that no longer matched what's served, until remount. Fixed by refetching this page's publish status right after the drive PATCH succeeds. 2. CanvasPublishSettingsSection: the one-shot "seed once ever" ref guard from the previous fix over-corrected — it stopped an in-progress edit from being clobbered, but also stopped a PRISTINE form from ever reseeding again. If the header's own Publish settings dialog saved different SEO values while this category stayed open, this form kept showing the old ones, and clicking Save would silently roll the header's change back. Replaced the one-shot ref with isDirty tracking: reseed whenever the store changes AND the form hasn't been touched since its last seed, still skipping the reseed once the user starts editing. Added CanvasHomePageSettingsSection.test.tsx and CanvasPublishSettingsSection.test.tsx covering both fixes directly (the refetch-after-PATCH call, and reseed-while-pristine vs preserve-while-dirty).
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1032b23c6
ℹ️ 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".
Two more findings from Codex's review of the previous fix (commit d1032b2): 1. The canvas View tab's theme-bridge preview fetches through GET /api/pages/[pageId]/publish, which was edit-gated — a view-only collaborator got a 403 and the preview silently fell back to themeBridgeEnabled: true, diverging from the real published artifact whenever the owner had disabled it. The route now also accepts view-level access (canPrincipalViewPage) and returns a minimal { published, canEdit: false, themeBridgeEnabled } shape for non-editors — everything else (SEO overrides, publish URL, staleness) stays edit-only. Added `canEdit` to the shared PublishStatus so the Settings tab's Publish/Appearance categories can tell "published" and "I can edit this" apart: without it, a view-only collaborator would see published: true and get an enabled Save button for fields the view-only response doesn't even populate. 2. usePublishStatusStore.fetchStatus wiped a page's cached status to EMPTY_PUBLISH_STATUS on ANY failed refresh, including a transient 5xx or network blip — since every consumer shares the same cache entry, a re-entered Settings category hitting a blip could make the header's controls disappear and re-disable already-published settings. Now preserves the last-known-good status (flagging hasLoadError) on a transient failure; a 403 still resets it, since that's a definitive permission signal, not a failure. Extended route.test.ts, CanvasPublishSettingsSection.test.tsx, and usePublishStatusStore.test.ts to cover both directly.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49a1a18fa6
ℹ️ 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".
Two more findings from Codex's review of the previous fix (commit 49a1a18): 1. Widening the GET route's permission to view-level (for the theme-bridge preview fix) left the UNPUBLISHED branch's response as { published: false, available } with no canEdit. PublishControls' "show a Publish button" branch keys off `available` alone, so a view-only viewer on an unpublished page got an enabled Publish button that only revealed the permission failure once its own POST 403'd. The unpublished branch now also returns canEdit, and PublishControls checks it alongside `available` before offering the button. 2. handlePublish reused the pre-publish cached settings whenever no dialog overrides were passed, on the assumption that "no overrides" means "nothing changed". That's false across an unpublish/republish: DELETE removes the published_pages row entirely, so a bare republish gets fresh server defaults (title/description/ogImage cleared, noindex false, themeBridgeEnabled true) — not the pre-unpublish values. The stale cache used to survive this and could later be silently re-persisted via Save, resurrecting settings the server had already reset. Now always reads settings back from the POST response. Added regression tests for both in route.test.ts and PublishControls.test.tsx (the second one drives a real unpublish→republish click sequence and asserts the store ends up with the server's fresh values, not the cache).
|
@codex review |
…toggle Proactive follow-up to the stale-settings bug Codex caught in PublishControls' handlePublish (commit 23bfd8c): CanvasAppearanceSettingsSection's handleToggle only sends { themeBridgeEnabled } in its POST body, but spread the locally cached settings for every other field instead of reading them back from the server's response — the same "trust the cache, not the response" assumption that broke there. The server always returns the full effective settings regardless of which fields were in the request, so there's no reason to prefer a local copy for the untouched ones. Added a test forcing a mismatch between the cached and response title to prove the response wins.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a77c584524
ℹ️ 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".
Two more findings from Codex's review of the previous fix (commit a77c584): 1. PublishSettingsDialog reseeded its form whenever its `initial` prop changed, but the parent recreates that object (a plain literal derived from status.settings) on every render. While the dialog stayed open, ANY unrelated parent re-render — contentDirty flipping, a shared status refresh completing elsewhere — recreated the object, changed `initial`'s reference, and retriggered the reseed effect, silently discarding whatever SEO edits the user had already typed. Fixed by dropping `initial` from the effect's dependency array so it only reseeds on the closed→open transition, not on every render while open. 2. usePublishStatusStore had no defense against out-of-order responses: mounting a settings category starts a GET while the header stays actionable; if that GET is still in flight when a faster mutation (e.g. Unpublish) commits fresher data, the GET's stale response could land afterward and overwrite it — e.g. resurrecting the URL of a page that was just successfully unpublished. Added a per-pageId generation counter: `setStatus` bumps it on every write (so any older in-flight fetch sees a mismatch), and `fetchStatus` captures its own generation before the network call and discards its result if superseded by the time it resolves. Added regression tests for both: one drives an open-dialog + unrelated store update and asserts the typed value survives; the other simulates the exact race (a mutation committing while a slower fetch is in flight) and asserts the mutation's result wins.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd080691bf
ℹ️ 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".
…ange
fetchStatus's in-flight dedup meant CanvasHomePageSettingsSection's
post-mutation refresh could silently reuse an already-in-flight fetch
that read the row before the drive PATCH, leaving the header showing
the pre-change URL. Add fetchStatus(pageId, { force: true }) to bypass
the dedup and start a fresh request, guarding the in-flight map's
cleanup so a superseded fetch's finally doesn't clear a newer one's slot.
Codex-flagged (PR #2286 review round 6).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vjis5JRK87HGksa6UtRanT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ed5a35bec
ℹ️ 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".
… image picker Two Codex findings from PR #2286 review round 7: - A canvas page assigned as the drive's (or a custom domain's) 404 page rendered through renderNotFoundPageHtml, which never looked up its persisted themeBridgeEnabled and always fell back to the injectThemeBridge default (true) — so disabling the theme bridge had no effect once the page became a 404 target. Now looks up the published_pages row and threads its themeBridgeEnabled through. - PublishSettingsFields disabled every field except the uploaded-image picker, whose disabled prop wasn't forwarded to PagePickerPopover — a disabled form (unpublished page, view-only collaborator) still let a user open the picker and select an image. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vjis5JRK87HGksa6UtRanT
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
<button>Publish</button>. Publish now lives in the page header for canvas pages, the same as documents/sheets/code.?tab=settings&category=.... Forms moves from its own tab into a category here.published_pages.themeBridgeEnabledso a canvas can opt out of PageSpace's forced light/dark theme override — threaded through the publish API, the published-page renderer, and the in-editor live preview (accurate for both editors and view-only collaborators).patch /api/drives/:driveId) rather than new plumbing.PublishControls' raw<button>elements to the sharedButtoncomponent and extracted its SEO override form into a reusablePublishSettingsFieldscomponent.usePublishStatusStore(shared, keyed by pageId, with acanEditfield and a generation counter guarding against out-of-order responses) so the header's PublishControls, the Settings tab's Publish/Appearance categories, and the View tab's live preview all stay in sync, never show editable UI to a viewer who can't save, and never let a stale background fetch clobber a fresher mutation.Review feedback addressed
The
chatgpt-codex-connectorautomated review went through eight passes and caught a real, fixed bug every time — full detail in the inline replies on each thread:?tab=value left the canvas page blank instead of falling back toview.2–4. The Settings tab's categories and the header went stale relative to actions taken elsewhere, because each fetched its own status once on mount.
5–6. The View tab's theme-bridge preview was wrongly edit-gated, and the shared cache reset to empty on any transient failure.
7–8. Widening the GET route for Expand structured logging across client and API usage flows #5/Add sheet unit tests and document the new page type #6 left the unpublished branch without
canEdit, andhandlePublishreused pre-unpublish cached settings on a bare republish instead of the server's reset ones.9–10.
PublishSettingsDialogreseeded (and silently discarded edits) on every parent render while open, not just when it opened; and the shared store had no defense against a slower in-flight fetch overwriting a faster mutation's result with stale data.setStatus, butCanvasHomePageSettingsSection's post-PATCH refresh goes throughfetchStatusitself — its in-flight dedup could hand back an already-in-flight (pre-mutation) request instead of reading fresh.fetchStatusnow takes aforceoption to bypass the dedup for exactly this caller.12–13. A canvas page assigned as the drive's (or a custom domain's) 404 page never had its own
themeBridgeEnabledlooked up when rendered via that path, so disabling the theme bridge had no effect once the page became a 404 target — now threaded through the same way the normal publish path does it. Separately,PublishSettingsFieldsdisabled every field except the uploaded-image picker, whosedisabledprop wasn't forwarded toPagePickerPopover.All of 2–11 converged on
usePublishStatusStore: a shared cache withcanEdit(so "published" and "editable" are never conflated), "trust the response over the cache" settings handling, a generation counter for write-ordering, and aforceescape hatch for the one caller that refreshes via a route the generation counter doesn't cover — plus one proactive fix (the Appearance toggle had the same stale-cache pattern as #8, fixed before it was flagged). #12–13 are independent, narrower fixes in the theme-bridge and settings-form surfaces.Codex's review of the final commit (
32a29d3f7) came back clean: "Didn't find any major issues."Test plan
bun run typecheck,bun run lint,bun run knip:check(web)bun run db:generateproduced a clean additive migration (0243_early_corsair.sql)?tab=settings&category=...Note: built and verified in an environment without a browser-automation tool available, so the manual UI checks above have not been run by me — please verify before merge.
https://claude.ai/code/session_01Vjis5JRK87HGksa6UtRanT