CMS-67: Wire environment-specific field badges in editor UI#42
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPropagates prepared document-route metadata from server-side config into client Studio init; computes per-environment schema hashes and environment→field mappings; extends mount context with environment-aware write info; updates loader, runtime UI, tests, and docs to surface environment-specific field badges and prevent stale async updates. Changes
Sequence DiagramsequenceDiagram
participant Server as Server Config
participant Resolver as DocumentRouteResolver
participant Client as AdminStudioClient
participant Loader as StudioLoader
participant UI as ContentDocumentPage
Server->>Resolver: resolveStudioDocumentRoutePreparedMetadata(config)
Resolver->>Resolver: compute schemaHashesByEnvironment
Resolver->>Resolver: compute environmentFieldTargets
Resolver-->>Server: StudioDocumentRoutePreparedMetadata
Server->>Client: pass documentRouteMetadata prop
Client->>Client: createClientStudioConfig(documentRouteMetadata)
Client-->>Server: MdcmsConfig (with _documentRouteMetadata)
Server->>Loader: mount studio with config
Loader->>Loader: readPreparedDocumentRouteMetadata(config)
Loader->>Loader: toWriteByEnvironment(schemaHashesByEnvironment)
Loader-->>UI: mount context with writeByEnvironment & environmentFieldTargets
UI->>UI: resolveActiveDocumentRouteContext(selectedEnvironment)
UI->>UI: getEnvironmentSpecificFieldBadges(environmentFieldTargets)
UI-->>UI: render badges ("staging only", etc.)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx`:
- Around line 1986-2005: The async-result guards that now only compare document
identity (documentId) must also validate the environment/revision token so
late-arriving staging responses cannot overwrite a switched environment; update
the guards in saveDraft, publishDocument and all version-loading flows to
compare route.initialEnvironment (or a route revision token) in addition to
documentId, and ensure activeContext/route (from
resolveActiveDocumentRouteContext) is passed into those async callbacks so they
bail when the environment token differs from the one captured when the request
was started.
In `@packages/studio/src/lib/studio-loader.ts`:
- Around line 244-274: isPreparedDocumentRouteMetadata currently accepts
loosely-shaped nested values which lets malformed _documentRouteMetadata slip
through and fail later in assertStudioMountContext; update
isPreparedDocumentRouteMetadata to strictly validate the nested structure:
verify value.schemaHashesByEnvironment is a record whose keys are non-empty
strings and values are non-empty trimmed strings; verify
value.environmentFieldTargets is a record whose keys are the same non-empty
environment keys (or at least a subset) and whose values are records mapping
non-empty type-name strings to non-empty string arrays; for each typeFields
ensure every key is a string and every entry in each array is a trimmed
non-empty string (use existing helpers isRecord and isStringArray as part of the
checks), and return false if any of these stricter invariants fail so malformed
metadata fails fast instead of later in assertStudioMountContext.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a5c9b6d-37a2-48a2-92c3-8d65e57e6ba5
📒 Files selected for processing (19)
apps/studio-example/app/admin/[[...path]]/page.tsxapps/studio-example/app/admin/admin-studio-client.tsxapps/studio-example/app/admin/studio-config.tsapps/studio-review/app/review/[scenario]/admin/[[...path]]/page.tsxapps/studio-review/app/review/[scenario]/admin/admin-studio-client.tsxapps/studio-review/app/review/[scenario]/admin/studio-config.tsapps/studio-review/mdcms.config.tsapps/studio-review/review/scenarios.test.tsapps/studio-review/review/scenarios.tspackages/shared/src/lib/contracts/extensibility.test.tspackages/shared/src/lib/contracts/extensibility.tspackages/studio/README.mdpackages/studio/src/lib/document-route-schema.test.tspackages/studio/src/lib/document-route-schema.tspackages/studio/src/lib/runtime-ui/pages/content-document-page.test.tsxpackages/studio/src/lib/runtime-ui/pages/content-document-page.tsxpackages/studio/src/lib/studio-loader.test.tspackages/studio/src/lib/studio-loader.tspackages/studio/src/lib/studio.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx (1)
198-278:⚠️ Potential issue | 🔴 CriticalApply the route token to
syncSchema()and variant creation too.Now that
initialEnvironmentis part of the active route, two async paths still commit stale results after an environment switch:syncSchema()only checks document identity on Line 2291, andhandleCreateVariant()still unconditionally navigates / sets errors after its awaits on Lines 2570-2585. A staging response can still overwrite production write state or redirect the user from the wrong environment.Also applies to: 2015-2034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx` around lines 198 - 278, syncSchema() and handleCreateVariant() are still vulnerable to stale results after an environment switch; generate and pass a ContentDocumentRouteRequestToken (use createContentDocumentRouteRequestToken) for the current request and, after each await in syncSchema() and in handleCreateVariant() before committing state, navigation or setting errors, validate the token with matchesContentDocumentRouteRequestToken using the current documentId and route.initialEnvironment—if it doesn't match, abort applying the response (no state changes, navigation or error updates).packages/studio/src/lib/studio-loader.ts (1)
240-245:⚠️ Potential issue | 🟠 MajorExclude arrays from prepared-metadata validation.
isRecord()still accepts arrays, so malformed_documentRouteMetadatalikeschemaHashesByEnvironment: ["hash"]can pass here as environment"0"instead of being rejected and falling back cleanly.Proposed fix
function isRecord(value: unknown): value is Record<string, unknown> { - return typeof value === "object" && value !== null; + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) + ); }Also applies to: 249-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/studio/src/lib/studio-loader.ts` around lines 240 - 245, The isRecord type guard currently treats arrays as objects, allowing arrays like schemaHashesByEnvironment to pass validation; update isRecord (and any related checks around prepared-metadata handling) to explicitly exclude arrays (e.g., add a check like !Array.isArray(value)) so that arrays are not considered records and will be rejected/fall back cleanly; also review the surrounding prepared-metadata logic referenced in the diff (functions/blocks around isRecord and isStringArray between lines ~249-304) to ensure arrays are handled consistently and that isStringArray remains the correct predicate for string-array fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx`:
- Around line 198-278: syncSchema() and handleCreateVariant() are still
vulnerable to stale results after an environment switch; generate and pass a
ContentDocumentRouteRequestToken (use createContentDocumentRouteRequestToken)
for the current request and, after each await in syncSchema() and in
handleCreateVariant() before committing state, navigation or setting errors,
validate the token with matchesContentDocumentRouteRequestToken using the
current documentId and route.initialEnvironment—if it doesn't match, abort
applying the response (no state changes, navigation or error updates).
In `@packages/studio/src/lib/studio-loader.ts`:
- Around line 240-245: The isRecord type guard currently treats arrays as
objects, allowing arrays like schemaHashesByEnvironment to pass validation;
update isRecord (and any related checks around prepared-metadata handling) to
explicitly exclude arrays (e.g., add a check like !Array.isArray(value)) so that
arrays are not considered records and will be rejected/fall back cleanly; also
review the surrounding prepared-metadata logic referenced in the diff
(functions/blocks around isRecord and isStringArray between lines ~249-304) to
ensure arrays are handled consistently and that isStringArray remains the
correct predicate for string-array fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45115eb3-0acb-41b9-91a3-8b1765f1300c
📒 Files selected for processing (4)
packages/studio/src/lib/runtime-ui/pages/content-document-page.test.tsxpackages/studio/src/lib/runtime-ui/pages/content-document-page.tsxpackages/studio/src/lib/studio-loader.test.tspackages/studio/src/lib/studio-loader.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx (1)
1612-1630: Add nullable-environment test coverage for route resolution.Line 1614 accepts
string | null | undefined, but behavior fornull/undefinedand unknown environments should be explicitly locked in tests (especially fallback at Line 1625-Line 1628).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx` around lines 1612 - 1630, The function resolveActiveDocumentRouteContext accepts environment: string | null | undefined but tests are missing for null/undefined and unknown environments; add unit tests for resolveActiveDocumentRouteContext that call it with environment = null, environment = undefined, and with an unknown environment string (not present in route.writeByEnvironment) to assert that when environment is null/undefined it returns the original route (route.initialEnvironment unchanged) and when environment is unknown it returns a route with initialEnvironment set to the trimmed unknown value and write equal to the fallback object (canWrite: false and the message containing the unknown environment); reference resolveActiveDocumentRouteContext, route.initialEnvironment and route.writeByEnvironment in your test to locate the behaviour to validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx`:
- Around line 1612-1630: The function resolveActiveDocumentRouteContext accepts
environment: string | null | undefined but tests are missing for null/undefined
and unknown environments; add unit tests for resolveActiveDocumentRouteContext
that call it with environment = null, environment = undefined, and with an
unknown environment string (not present in route.writeByEnvironment) to assert
that when environment is null/undefined it returns the original route
(route.initialEnvironment unchanged) and when environment is unknown it returns
a route with initialEnvironment set to the trimmed unknown value and write equal
to the fallback object (canWrite: false and the message containing the unknown
environment); reference resolveActiveDocumentRouteContext,
route.initialEnvironment and route.writeByEnvironment in your test to locate the
behaviour to validate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e37819b-62d7-4e31-8adb-ba31575930b5
📒 Files selected for processing (3)
packages/studio/src/lib/runtime-ui/pages/content-document-page.tsxpackages/studio/src/lib/studio-loader.test.tspackages/studio/src/lib/studio-loader.ts
✅ Files skipped from review due to trivial changes (1)
- packages/studio/src/lib/studio-loader.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/studio/src/lib/studio-loader.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/studio-example/mdcms.config.test.ts (1)
63-71: Consider narrowing assertion scope to reduce fixture brittleness.Line 64 currently asserts the entire
environmentFieldTargetsobject. If future demo fields are added for other types/environments, this test can fail even when thepostcontract still works.Proposed tweak
- assert.deepEqual( - preparedConfig._documentRouteMetadata?.environmentFieldTargets, - { - post: { - abTestVariant: ["staging"], - featured: ["staging"], - }, - }, - ); + assert.deepEqual( + preparedConfig._documentRouteMetadata?.environmentFieldTargets?.post, + { + abTestVariant: ["staging"], + featured: ["staging"], + }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/studio-example/mdcms.config.test.ts` around lines 63 - 71, The test is asserting the entire preparedConfig._documentRouteMetadata?.environmentFieldTargets object, making it brittle; narrow the assertion to only the 'post' entry by changing the assertion to check preparedConfig._documentRouteMetadata?.environmentFieldTargets?.post equals the expected object (abTestVariant and featured arrays) so other future keys won't break the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/studio-example/mdcms.config.test.ts`:
- Around line 63-71: The test is asserting the entire
preparedConfig._documentRouteMetadata?.environmentFieldTargets object, making it
brittle; narrow the assertion to only the 'post' entry by changing the assertion
to check preparedConfig._documentRouteMetadata?.environmentFieldTargets?.post
equals the expected object (abTestVariant and featured arrays) so other future
keys won't break the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 388d113a-4142-4297-b024-3f95088a6031
📒 Files selected for processing (2)
apps/studio-example/mdcms.config.test.tsapps/studio-example/mdcms.config.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/SPEC-006-studio-runtime-and-ui.md`:
- Line 329: The current doc line for GET /api/v1/content/:documentId?draft=true
is ambiguous; change the wording to state that the draft=true query parameter is
an authorization/read-model switch that returns the mutable head content
(subject to caller authorization) and explicitly warn that it is not a
server-side status filter for "show unpublished" or "draft-only" documents;
update the description around the GET /api/v1/content/:documentId?draft=true
entry to use that precise phrasing and add a short note that authorization
controls access to the mutable head snapshot.
In `@docs/specs/SPEC-007-editor-mdx-and-collaboration.md`:
- Around line 103-104: The draftChanged reducer in content-document-page.tsx
currently only updates body via event.body and state.draftBody; update it to
also detect and update frontmatter/property changes by comparing
event.properties with state.draftProperties and setting state.draftProperties
(and corresponding unsaved flag) so frontmatter-only edits enter the same
unsaved/saving/saved flow; ensure the same saveDraft/save pipeline (the existing
draft save handlers) reads state.draftProperties as well as state.draftBody so
frontmatter edits are persisted through the same state machine and triggers.
- Around line 98-99: The spec says auto-save should persist frontmatter but the
implementation's updateDraft call in content-document-page.tsx currently only
sends { body: input.state.draftBody }; update the client-side auto-save to
include the current frontmatter draft (e.g., include
input.state.draftFrontmatter or the Properties tab state) in the payload sent to
updateDraft, and ensure the updateDraft API handler/endpoint accepts and
persists that frontmatter field as well; alternatively, if you intend not to
persist frontmatter on auto-save, update the SPEC-007 text to remove the claim
about persisting frontmatter so the spec matches the current updateDraft
behavior.
- Around line 109-110: The spec claims Publish "copies the current draft body
and frontmatter into a new immutable row in document_versions" but the publish
handler in content-document-page.tsx (the function that sends { documentId,
locale, changeSummary }) does not include frontmatter or draft body; either
update the spec to state that frontmatter is captured server-side from draft
state, or change the publish handler to explicitly collect the current draft
body and frontmatter and include them in the API payload (alongside documentId,
locale, changeSummary) so the server can write the immutable row to
document_versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a417a003-68d4-4fb8-86c7-ecfbac7515fa
📒 Files selected for processing (2)
docs/specs/SPEC-006-studio-runtime-and-ui.mddocs/specs/SPEC-007-editor-mdx-and-collaboration.md
Collaborator
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Replace bordered cards with flat divider-separated rows, demote type labels to right-aligned monospace hints, use red * for required fields instead of Optional badges, render environment badges as colored amber pills, inline boolean toggle controls, and dim unsupported fields.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/studio/src/lib/runtime-ui/pages/content-document-page.tsx`:
- Around line 433-449: The function mapErrorToFrontmatterField currently maps
any string in RuntimeError.details.field to a frontmatter property; change it so
it only returns a field name when the candidate explicitly starts with
"frontmatter." — check mapErrorToFrontmatterField, the local variable candidate,
and the normalized logic and bail out (return undefined) if candidate does not
startWith("frontmatter."); apply the same change to the other identical mapping
occurrence (the second implementation around the 1593-1604 region) so
non-frontmatter paths (e.g., body-level validation) are not treated as
frontmatter errors.
- Around line 2921-2934: The mutation paths (publishDocument, saveDraft,
handleCreateVariant) are creating an API using the current activeContext/route
while using a stale document snapshot from stateRef.current, allowing writes to
be sent to the wrong route; fix by capturing the relevant snapshot-bound
routing/context at the start of each mutation (e.g., read const snapshot =
stateRef.current; const requestContext = snapshot.context || activeContext;
const requestRoute = snapshot.route || route) and then call createRouteApi with
those snapshot-bound requestContext/requestRoute before performing any mutation
so the API route always matches the document snapshot being mutated (apply same
pattern in publishDocument, saveDraft, handleCreateVariant and the other
indicated blocks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b83c0449-2edc-4a64-a541-7ce4f3f98cab
📒 Files selected for processing (6)
docs/specs/SPEC-006-studio-runtime-and-ui.mddocs/specs/SPEC-007-editor-mdx-and-collaboration.mdpackages/studio/src/lib/document-shell.test.tspackages/studio/src/lib/document-shell.tspackages/studio/src/lib/runtime-ui/pages/content-document-page.test.tsxpackages/studio/src/lib/runtime-ui/pages/content-document-page.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/specs/SPEC-006-studio-runtime-and-ui.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/studio/src/lib/runtime-ui/pages/content-document-page.test.tsx
Bail out early when the error field path does not start with "frontmatter." so non-frontmatter paths (e.g. body-level validation) are not incorrectly mapped as frontmatter field errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
Notes
bun run format:checkstill fails because of a pre-existing unrelated formatting issue inpackages/studio/src/lib/runtime-ui/app/admin/environments-page.tsx.Summary by CodeRabbit