Repository navigation
feat(studio): ship the polished authoring workspace and alpha.6 - #16
Conversation
|
Warning Review limit reached
Next review available in: 28 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 (9)
📝 WalkthroughWalkthroughThe PR updates Scribe Studio with Monaco-based editing, split preview, recovery-aware mutations, durable file handling, worker fault cooldowns, Tailwind preview mirroring, and revised table rendering. It also adds release metadata, integration checklists, package audits, and expanded automated coverage. ChangesStudio release and audit contracts
Compiler and durable file handling
Studio server and preview
Studio client
Rendering and browser validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
e4a4bae to
ab0acfd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release/package-manifests.test.ts (1)
6-7: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBump the test manifest version to
0.1.0-alpha.6.
tests/release/package-manifests.test.tsstill usesconst version = "0.1.0-alpha.5";, but the public packages are at0.1.0-alpha.6; the manifest assertions use this constant forversionand@scribe-sdkinternal dependencies, so the tests will fail against the published package versions. Update this expectation to0.1.0-alpha.6.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/release/package-manifests.test.ts` around lines 6 - 7, Update the version constant in package-manifests.test.ts from 0.1.0-alpha.5 to 0.1.0-alpha.6 so manifest assertions match the published package versions and internal `@scribe-sdk` dependencies.
🧹 Nitpick comments (13)
packages/cli/src/studio.ts (1)
400-422: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a
.catchto theunlink/errorwatcher transactions.
handleExternalChangeguards its promise chain, but these two do not. The bodies cannot currently reject, so this is defensive only — it protects against an unhandled rejection if the callbacks ever grow toawaitanything.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 400 - 422, Add defensive .catch handlers to both coordinator.system promise chains in the server.watcher "unlink" and "error" callbacks, matching the protection already used by handleExternalChange. Keep the existing revision updates and event publication unchanged, and ensure any rejection is handled rather than becoming unhandled.packages/cli/src/studio-files.ts (2)
41-42: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
isFile()guard runs too late to prevent a hang.
readFileandstatare issued concurrently, so opening a FIFO/character device would block before the regular-file check ever executes. Stat first, then read.♻️ Proposed reorder
- const [bytes, info] = await Promise.all([readFile(resolvedPath), stat(resolvedPath)]); - if (!info.isFile()) throw new Error(`Studio source is not a regular file: ${requestedPath}`); + const info = await stat(resolvedPath); + if (!info.isFile()) throw new Error(`Studio source is not a regular file: ${requestedPath}`); + const bytes = await readFile(resolvedPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.ts` around lines 41 - 42, Update the file-loading flow around the Promise.all call to await stat(resolvedPath) and validate info.isFile() before invoking readFile(resolvedPath). Only read the path after it is confirmed to be a regular file, preserving the existing error message and returned bytes/info behavior.
118-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueLone
\rline endings bypass the mixed-ending guard and get silently rewritten.
detectLineEndingonly counts\r\nand(?<!\r)\n, so a classic-Mac (CR-only) file reports"\n"andencodeStudioSourcerewrites every\r— exactly the silent normalization this check exists to prevent. Counting bare\ralongside the others closes the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.ts` around lines 118 - 125, Update detectLineEnding to count bare carriage returns separately from CRLF sequences, and treat them as mixed line endings when combined with LF or CRLF. Preserve the existing return values for valid uniform LF and CRLF files, while preventing encodeStudioSource from silently rewriting CR-only or mixed-ending content.packages/cli/src/studio-files.test.ts (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
readdirat the top instead of a mid-test dynamic import.♻️ Proposed cleanup
-import { chmod, lstat, mkdtemp, readFile, symlink, writeFile } from "node:fs/promises"; +import { chmod, lstat, mkdtemp, readdir, readFile, symlink, writeFile } from "node:fs/promises";- const directory = await import("node:fs/promises").then(({ readdir }) => readdir(join(path, ".."))); + const directory = await readdir(join(path, ".."));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.test.ts` around lines 59 - 60, Update the test setup in the affected studio-files test to import readdir from node:fs/promises at module scope, then call that imported symbol directly instead of using the mid-test dynamic import. Preserve the existing directory filtering and assertion behavior.scripts/audit-public-packages.mjs (2)
10-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a timeout for the audit subprocess.
spawnSynchere has notimeoutoption; a hungbun auditnetwork call (registry lookup) would block the release pipeline indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/audit-public-packages.mjs` around lines 10 - 13, Add a finite timeout option to the spawnSync invocation used for the bun audit subprocess in the audit script, keeping the existing command, working directory, and inherited stdio behavior unchanged. Configure the timeout through the spawn options so a hung registry request terminates instead of blocking indefinitely.
10-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
--prodforbun auditto avoid relying on undocumented flags.
bun audit --prodis the documented flag for excluding devDependencies. Keep the audit scoped to production dependencies, or remove the flag altogether if only the lockfile should be audited.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/audit-public-packages.mjs` around lines 10 - 13, Update the spawnSync invocation in the audit-public-packages script to replace the undocumented “--production” argument with Bun’s documented “--prod” flag, preserving the audit’s production-dependency scope.tests/release/ci.test.ts (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese assertions pin the audit script's source formatting, not its behavior.
Any reformat (prettier, reordering the package list) fails the test while the audit still works. Exporting the package list from
scripts/audit-public-packages.mjsand importing it here would assert the actual contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/release/ci.test.ts` around lines 63 - 64, Update the test around the auditScript assertions in ci.test.ts to validate the exported public package list from audit-public-packages.mjs rather than matching source-code formatting. Import and assert the shared package-list symbol, while removing the brittle string assertions that depend on declaration formatting or ordering.packages/cli/src/studio-ui.ts (2)
950-964: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree copies of the same draft PUT.
Lines 870-877, 953-961, and 988-995 build the identical
{ source, externalConflict, baseDiskVersion }payload with slightly different post-processing. Extracting a singleputDraft(source, { externalConflict })helper would keep the conflict-flag reset and error toast consistent as this logic evolves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 950 - 964, Extract the duplicated draft PUT request logic from the three call sites around the effects near lines 870, 953, and 988 into a shared putDraft helper. Have the helper construct the source, externalConflict, and baseDiskVersion payload, reset browserRecoveryConflict on successful responses, and consistently surface failures through the existing error toast, while preserving each caller’s distinct post-processing such as apply(body).
431-437: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve Monaco edit history when syncing external source values.
model.setValue(value)flushes the model contents and resets selection/cursor, which loses undo history during server-driven refreshes. Use a full-model edit operation withpushEditOperationsand an undo stop so the replacement is recorded in the editor’s redo/undo flow.♻️ Suggested change
useEffect(() => { const model = modelRef.current; if (!model || model.getValue() === value) return; applyingExternalValue.current = true; - model.setValue(value); + editor.pushUndoStop(); + editor.executeEdits('source-sync', [{ range: model.getFullModelRange(), text: value }]); + editor.pushUndoStop(); applyingExternalValue.current = false; }, [value]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 431 - 437, Update the external-value synchronization in the useEffect using modelRef and applyingExternalValue so it replaces the model text through Monaco’s full-model pushEditOperations API instead of model.setValue. Add an undo stop around the replacement so the change remains in the editor’s undo/redo history while preserving the existing equality guard and applyingExternalValue lifecycle.tests/studio-browser/studio.spec.ts (2)
98-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest is pinned to absolute fixture line numbers.
Control+g→9/35and the[data-scribe-source-line="33"]assertion break silently whenevertests/fixtures/studio-article.mdxgains or loses a line above those offsets — and this PR just edited that fixture. Anchoring by content (e.g. Monaco find, or clicking the rendered paragraph) would decouple the two files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/studio-browser/studio.spec.ts` around lines 98 - 117, Update the studio browser test around the Control+g navigation and synchronizedCode assertion to locate target content by stable text or rendered paragraph rather than absolute fixture line numbers. Replace the hardcoded 9, 35, and data-scribe-source-line="33" dependencies with content-based navigation and assertions that continue to verify editing and preview synchronization.
160-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed
waitForTimeout(160)before the screenshot is flake-prone.The preceding
toBeVisible()already waits for the option; the sleep only masks menu animation. Prefer waiting on a stable condition (e.g. the popup's animation-settled state ortoHaveCSS("opacity", "1")) so slower CI machines don't capture a mid-animation frame.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/studio-browser/studio.spec.ts` around lines 160 - 165, Replace the fixed waitForTimeout(160) in the viewport menu flow with an assertion on a stable animation-settled condition, such as the visible Mobile option reaching opacity 1 via toHaveCSS. Keep the screenshot and subsequent mobileOption.click() actions unchanged.packages/cli/src/studio-ui.test.ts (1)
5-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this into focused tests.
A single
itwith ~110 substring assertions reports only the first failure and mixes unrelated contracts (Monaco wiring, recovery, lease, viewport, styles). Grouping into a fewits (imports/editor, recovery+lease, shell/status, styles) would make regressions much easier to localize. Several assertions also pin exact CSS whitespace (Lines 98, 106, 110), so purely cosmetic formatting changes will fail them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.test.ts` around lines 5 - 114, Split the oversized test around studioClientModule and studioStyles into focused tests covering imports/editor wiring, recovery and lease behavior, shell/status behavior, and styles. Preserve all existing assertions while grouping them by contract so failures identify the affected area. Replace exact CSS-whitespace substring checks with formatting-insensitive assertions where possible, especially the viewport, Monaco, and toaster style checks.packages/mdx/src/rehype.ts (1)
170-195: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
colSpanis ignored when counting columns.A row like
<tr><th colspan="4">…</th></tr>counts as one column, so a genuinely wide table can miss thewidemarker. Safe fallback (it just renders in the compact layout), but worth handling if spanned headers are expected in article tables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mdx/src/rehype.ts` around lines 170 - 195, Update maximumTableColumnCount when processing tr rows to account for each cell’s colSpan, summing the span value for th and td elements instead of counting every cell as one column; default missing, invalid, or non-positive colSpan values to one while preserving the existing recursive behavior for non-row nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/studio-compiler.ts`:
- Around line 54-59: Update the worker lifecycle around `#restartWorker` and
`#createWorker` to prevent unbounded immediate restarts after repeated error,
exit, or job-timeout failures. Add a bounded consecutive-failure guard with
cooldown/backoff or a circuit-open state, reset it after a successful worker
recovery, and ensure further compile requests receive the established
unavailable/error result while the circuit is open instead of spawning workers
continuously.
In `@packages/cli/src/studio-files.ts`:
- Around line 74-81: The temporary file created in durableWriteStudioFile must
preserve the requested permission bits despite umask. In
packages/cli/src/studio-files.ts#L74-L81, chmod the open temporary handle to
options.mode & 0o7777 before sync(); in
packages/cli/src/studio-files.test.ts#L16-L39, change the asserted mode to
include group/other-writable bits such as 0o664 so the test exercises umask
handling.
In `@packages/cli/src/studio-ui.ts`:
- Around line 985-1003: Update flushMarkdown to return a falsy failure signal
instead of the previous state when the draft PUT fails or returns an invalid
body. In save(), check the awaited flushMarkdown result and abort before issuing
the save request when it is falsy; preserve the existing success flow and
enterRich handling.
- Around line 711-735: Unify the splitter bounds used by the rendered ARIA
attributes, pointer dragging, fitSelectedViewport, and keyboard handling. Derive
a shared minimumSplit from the available width using the existing 320px/42%
rule, set aria-valuemin/max to minimumSplit and 100 - minimumSplit, and clamp
updateSplit, fitSelectedViewport, and arrow-key adjustments to those same
limits.
- Around line 570-586: Update the RichEditor draft request flow and the
onAccepted session update so accepted saves use the current Rich Text base disk
version rather than the stale session.baseDiskVersion. Ensure subsequent
/__scribe/api/rich-draft requests send a baseDiskVersion matching
context.state.diskVersion after acceptance, while preserving the existing
revision, source, and projection updates.
- Around line 886-897: Update the effect containing openDocument so a false
initial load schedules retries instead of returning permanently without creating
EventSource. Re-run openDocument until it succeeds or the effect is deactivated,
then establish EventSource and retain the existing refresh and connection
handlers. Ensure cleanup clears any pending retry and closes the event source.
In `@packages/cli/src/studio.ts`:
- Around line 806-812: The StudioFileConflictError branch in the request catch
block must broadcast the updated conflict state before returning the 409
response. After setting context.state.conflict, publish the current revision
using the existing events.publish pattern from the watcher error path, then
preserve the current response payload and status.
- Around line 631-643: In both draft mutation handlers, preserve the prior
context.state.recoveryBaseVersion before assigning the client base version, and
restore it in a catch if applyDraft throws; update the handlers at
packages/cli/src/studio.ts lines 631-643 and 687-697, following the
save-and-restore pattern used by /recover-discard. Keep successful applyDraft
behavior unchanged.
- Around line 1431-1459: Prevent unhandled rejections in closeStudioServers and
shutdownStep by attaching a no-op rejection handler immediately when creating
httpClosed and when receiving the operation promise, while preserving the
existing awaited error propagation and timeout behavior.
---
Outside diff comments:
In `@tests/release/package-manifests.test.ts`:
- Around line 6-7: Update the version constant in package-manifests.test.ts from
0.1.0-alpha.5 to 0.1.0-alpha.6 so manifest assertions match the published
package versions and internal `@scribe-sdk` dependencies.
---
Nitpick comments:
In `@packages/cli/src/studio-files.test.ts`:
- Around line 59-60: Update the test setup in the affected studio-files test to
import readdir from node:fs/promises at module scope, then call that imported
symbol directly instead of using the mid-test dynamic import. Preserve the
existing directory filtering and assertion behavior.
In `@packages/cli/src/studio-files.ts`:
- Around line 41-42: Update the file-loading flow around the Promise.all call to
await stat(resolvedPath) and validate info.isFile() before invoking
readFile(resolvedPath). Only read the path after it is confirmed to be a regular
file, preserving the existing error message and returned bytes/info behavior.
- Around line 118-125: Update detectLineEnding to count bare carriage returns
separately from CRLF sequences, and treat them as mixed line endings when
combined with LF or CRLF. Preserve the existing return values for valid uniform
LF and CRLF files, while preventing encodeStudioSource from silently rewriting
CR-only or mixed-ending content.
In `@packages/cli/src/studio-ui.test.ts`:
- Around line 5-114: Split the oversized test around studioClientModule and
studioStyles into focused tests covering imports/editor wiring, recovery and
lease behavior, shell/status behavior, and styles. Preserve all existing
assertions while grouping them by contract so failures identify the affected
area. Replace exact CSS-whitespace substring checks with formatting-insensitive
assertions where possible, especially the viewport, Monaco, and toaster style
checks.
In `@packages/cli/src/studio-ui.ts`:
- Around line 950-964: Extract the duplicated draft PUT request logic from the
three call sites around the effects near lines 870, 953, and 988 into a shared
putDraft helper. Have the helper construct the source, externalConflict, and
baseDiskVersion payload, reset browserRecoveryConflict on successful responses,
and consistently surface failures through the existing error toast, while
preserving each caller’s distinct post-processing such as apply(body).
- Around line 431-437: Update the external-value synchronization in the
useEffect using modelRef and applyingExternalValue so it replaces the model text
through Monaco’s full-model pushEditOperations API instead of model.setValue.
Add an undo stop around the replacement so the change remains in the editor’s
undo/redo history while preserving the existing equality guard and
applyingExternalValue lifecycle.
In `@packages/cli/src/studio.ts`:
- Around line 400-422: Add defensive .catch handlers to both coordinator.system
promise chains in the server.watcher "unlink" and "error" callbacks, matching
the protection already used by handleExternalChange. Keep the existing revision
updates and event publication unchanged, and ensure any rejection is handled
rather than becoming unhandled.
In `@packages/mdx/src/rehype.ts`:
- Around line 170-195: Update maximumTableColumnCount when processing tr rows to
account for each cell’s colSpan, summing the span value for th and td elements
instead of counting every cell as one column; default missing, invalid, or
non-positive colSpan values to one while preserving the existing recursive
behavior for non-row nodes.
In `@scripts/audit-public-packages.mjs`:
- Around line 10-13: Add a finite timeout option to the spawnSync invocation
used for the bun audit subprocess in the audit script, keeping the existing
command, working directory, and inherited stdio behavior unchanged. Configure
the timeout through the spawn options so a hung registry request terminates
instead of blocking indefinitely.
- Around line 10-13: Update the spawnSync invocation in the
audit-public-packages script to replace the undocumented “--production” argument
with Bun’s documented “--prod” flag, preserving the audit’s
production-dependency scope.
In `@tests/release/ci.test.ts`:
- Around line 63-64: Update the test around the auditScript assertions in
ci.test.ts to validate the exported public package list from
audit-public-packages.mjs rather than matching source-code formatting. Import
and assert the shared package-list symbol, while removing the brittle string
assertions that depend on declaration formatting or ordering.
In `@tests/studio-browser/studio.spec.ts`:
- Around line 98-117: Update the studio browser test around the Control+g
navigation and synchronizedCode assertion to locate target content by stable
text or rendered paragraph rather than absolute fixture line numbers. Replace
the hardcoded 9, 35, and data-scribe-source-line="33" dependencies with
content-based navigation and assertions that continue to verify editing and
preview synchronization.
- Around line 160-165: Replace the fixed waitForTimeout(160) in the viewport
menu flow with an assertion on a stable animation-settled condition, such as the
visible Mobile option reaching opacity 1 via toHaveCSS. Keep the screenshot and
subsequent mobileOption.click() actions unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34d7a6f7-6664-4cf1-8a17-d80be5fe78ce
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.changeset/clean-studios-compose.md.changeset/durable-studios-recover.md.changeset/pre.json.github/workflows/ci.ymlREADME.mdRELEASING.mdpackage.jsonpackages/cli/CHANGELOG.mdpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/init.test.tspackages/cli/src/rich-preservation.tspackages/cli/src/studio-compiler.test.tspackages/cli/src/studio-compiler.tspackages/cli/src/studio-events.test.tspackages/cli/src/studio-events.tspackages/cli/src/studio-files.test.tspackages/cli/src/studio-files.tspackages/cli/src/studio-fs.tspackages/cli/src/studio-lease.test.tspackages/cli/src/studio-lease.tspackages/cli/src/studio-recovery.test.tspackages/cli/src/studio-recovery.tspackages/cli/src/studio-security.test.tspackages/cli/src/studio-security.tspackages/cli/src/studio-transactions.test.tspackages/cli/src/studio-transactions.tspackages/cli/src/studio-ui.test.tspackages/cli/src/studio-ui.tspackages/cli/src/studio.test.tspackages/cli/src/studio.tspackages/mdx/CHANGELOG.mdpackages/mdx/README.mdpackages/mdx/package.jsonpackages/mdx/src/rehype.tspackages/mdx/src/table-normalization.test.tspackages/react/CHANGELOG.mdpackages/react/README.mdpackages/react/package.jsonpackages/styles/CHANGELOG.mdpackages/styles/README.mdpackages/styles/default.csspackages/styles/default.test.tspackages/styles/foundation.csspackages/styles/package.jsonplaywright.studio.config.tsscripts/audit-public-packages.mjsscripts/test-packed-consumers.mjstests/fixtures/studio-article.mdxtests/integration/next-css-modules/package.jsontests/integration/next-remote/package.jsontests/integration/next/package.jsontests/release/ci.test.tstests/release/documentation.test.tstests/release/package-manifests.test.tstests/studio-browser/studio.spec.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release/package-manifests.test.ts (1)
6-7: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBump the test manifest version to
0.1.0-alpha.6.
tests/release/package-manifests.test.tsstill usesconst version = "0.1.0-alpha.5";, but the public packages are at0.1.0-alpha.6; the manifest assertions use this constant forversionand@scribe-sdkinternal dependencies, so the tests will fail against the published package versions. Update this expectation to0.1.0-alpha.6.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/release/package-manifests.test.ts` around lines 6 - 7, Update the version constant in package-manifests.test.ts from 0.1.0-alpha.5 to 0.1.0-alpha.6 so manifest assertions match the published package versions and internal `@scribe-sdk` dependencies.
🧹 Nitpick comments (13)
packages/cli/src/studio.ts (1)
400-422: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a
.catchto theunlink/errorwatcher transactions.
handleExternalChangeguards its promise chain, but these two do not. The bodies cannot currently reject, so this is defensive only — it protects against an unhandled rejection if the callbacks ever grow toawaitanything.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 400 - 422, Add defensive .catch handlers to both coordinator.system promise chains in the server.watcher "unlink" and "error" callbacks, matching the protection already used by handleExternalChange. Keep the existing revision updates and event publication unchanged, and ensure any rejection is handled rather than becoming unhandled.packages/cli/src/studio-files.ts (2)
41-42: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
isFile()guard runs too late to prevent a hang.
readFileandstatare issued concurrently, so opening a FIFO/character device would block before the regular-file check ever executes. Stat first, then read.♻️ Proposed reorder
- const [bytes, info] = await Promise.all([readFile(resolvedPath), stat(resolvedPath)]); - if (!info.isFile()) throw new Error(`Studio source is not a regular file: ${requestedPath}`); + const info = await stat(resolvedPath); + if (!info.isFile()) throw new Error(`Studio source is not a regular file: ${requestedPath}`); + const bytes = await readFile(resolvedPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.ts` around lines 41 - 42, Update the file-loading flow around the Promise.all call to await stat(resolvedPath) and validate info.isFile() before invoking readFile(resolvedPath). Only read the path after it is confirmed to be a regular file, preserving the existing error message and returned bytes/info behavior.
118-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueLone
\rline endings bypass the mixed-ending guard and get silently rewritten.
detectLineEndingonly counts\r\nand(?<!\r)\n, so a classic-Mac (CR-only) file reports"\n"andencodeStudioSourcerewrites every\r— exactly the silent normalization this check exists to prevent. Counting bare\ralongside the others closes the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.ts` around lines 118 - 125, Update detectLineEnding to count bare carriage returns separately from CRLF sequences, and treat them as mixed line endings when combined with LF or CRLF. Preserve the existing return values for valid uniform LF and CRLF files, while preventing encodeStudioSource from silently rewriting CR-only or mixed-ending content.packages/cli/src/studio-files.test.ts (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
readdirat the top instead of a mid-test dynamic import.♻️ Proposed cleanup
-import { chmod, lstat, mkdtemp, readFile, symlink, writeFile } from "node:fs/promises"; +import { chmod, lstat, mkdtemp, readdir, readFile, symlink, writeFile } from "node:fs/promises";- const directory = await import("node:fs/promises").then(({ readdir }) => readdir(join(path, ".."))); + const directory = await readdir(join(path, ".."));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.test.ts` around lines 59 - 60, Update the test setup in the affected studio-files test to import readdir from node:fs/promises at module scope, then call that imported symbol directly instead of using the mid-test dynamic import. Preserve the existing directory filtering and assertion behavior.scripts/audit-public-packages.mjs (2)
10-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a timeout for the audit subprocess.
spawnSynchere has notimeoutoption; a hungbun auditnetwork call (registry lookup) would block the release pipeline indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/audit-public-packages.mjs` around lines 10 - 13, Add a finite timeout option to the spawnSync invocation used for the bun audit subprocess in the audit script, keeping the existing command, working directory, and inherited stdio behavior unchanged. Configure the timeout through the spawn options so a hung registry request terminates instead of blocking indefinitely.
10-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
--prodforbun auditto avoid relying on undocumented flags.
bun audit --prodis the documented flag for excluding devDependencies. Keep the audit scoped to production dependencies, or remove the flag altogether if only the lockfile should be audited.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/audit-public-packages.mjs` around lines 10 - 13, Update the spawnSync invocation in the audit-public-packages script to replace the undocumented “--production” argument with Bun’s documented “--prod” flag, preserving the audit’s production-dependency scope.tests/release/ci.test.ts (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese assertions pin the audit script's source formatting, not its behavior.
Any reformat (prettier, reordering the package list) fails the test while the audit still works. Exporting the package list from
scripts/audit-public-packages.mjsand importing it here would assert the actual contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/release/ci.test.ts` around lines 63 - 64, Update the test around the auditScript assertions in ci.test.ts to validate the exported public package list from audit-public-packages.mjs rather than matching source-code formatting. Import and assert the shared package-list symbol, while removing the brittle string assertions that depend on declaration formatting or ordering.packages/cli/src/studio-ui.ts (2)
950-964: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree copies of the same draft PUT.
Lines 870-877, 953-961, and 988-995 build the identical
{ source, externalConflict, baseDiskVersion }payload with slightly different post-processing. Extracting a singleputDraft(source, { externalConflict })helper would keep the conflict-flag reset and error toast consistent as this logic evolves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 950 - 964, Extract the duplicated draft PUT request logic from the three call sites around the effects near lines 870, 953, and 988 into a shared putDraft helper. Have the helper construct the source, externalConflict, and baseDiskVersion payload, reset browserRecoveryConflict on successful responses, and consistently surface failures through the existing error toast, while preserving each caller’s distinct post-processing such as apply(body).
431-437: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve Monaco edit history when syncing external source values.
model.setValue(value)flushes the model contents and resets selection/cursor, which loses undo history during server-driven refreshes. Use a full-model edit operation withpushEditOperationsand an undo stop so the replacement is recorded in the editor’s redo/undo flow.♻️ Suggested change
useEffect(() => { const model = modelRef.current; if (!model || model.getValue() === value) return; applyingExternalValue.current = true; - model.setValue(value); + editor.pushUndoStop(); + editor.executeEdits('source-sync', [{ range: model.getFullModelRange(), text: value }]); + editor.pushUndoStop(); applyingExternalValue.current = false; }, [value]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 431 - 437, Update the external-value synchronization in the useEffect using modelRef and applyingExternalValue so it replaces the model text through Monaco’s full-model pushEditOperations API instead of model.setValue. Add an undo stop around the replacement so the change remains in the editor’s undo/redo history while preserving the existing equality guard and applyingExternalValue lifecycle.tests/studio-browser/studio.spec.ts (2)
98-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest is pinned to absolute fixture line numbers.
Control+g→9/35and the[data-scribe-source-line="33"]assertion break silently whenevertests/fixtures/studio-article.mdxgains or loses a line above those offsets — and this PR just edited that fixture. Anchoring by content (e.g. Monaco find, or clicking the rendered paragraph) would decouple the two files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/studio-browser/studio.spec.ts` around lines 98 - 117, Update the studio browser test around the Control+g navigation and synchronizedCode assertion to locate target content by stable text or rendered paragraph rather than absolute fixture line numbers. Replace the hardcoded 9, 35, and data-scribe-source-line="33" dependencies with content-based navigation and assertions that continue to verify editing and preview synchronization.
160-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed
waitForTimeout(160)before the screenshot is flake-prone.The preceding
toBeVisible()already waits for the option; the sleep only masks menu animation. Prefer waiting on a stable condition (e.g. the popup's animation-settled state ortoHaveCSS("opacity", "1")) so slower CI machines don't capture a mid-animation frame.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/studio-browser/studio.spec.ts` around lines 160 - 165, Replace the fixed waitForTimeout(160) in the viewport menu flow with an assertion on a stable animation-settled condition, such as the visible Mobile option reaching opacity 1 via toHaveCSS. Keep the screenshot and subsequent mobileOption.click() actions unchanged.packages/cli/src/studio-ui.test.ts (1)
5-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this into focused tests.
A single
itwith ~110 substring assertions reports only the first failure and mixes unrelated contracts (Monaco wiring, recovery, lease, viewport, styles). Grouping into a fewits (imports/editor, recovery+lease, shell/status, styles) would make regressions much easier to localize. Several assertions also pin exact CSS whitespace (Lines 98, 106, 110), so purely cosmetic formatting changes will fail them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.test.ts` around lines 5 - 114, Split the oversized test around studioClientModule and studioStyles into focused tests covering imports/editor wiring, recovery and lease behavior, shell/status behavior, and styles. Preserve all existing assertions while grouping them by contract so failures identify the affected area. Replace exact CSS-whitespace substring checks with formatting-insensitive assertions where possible, especially the viewport, Monaco, and toaster style checks.packages/mdx/src/rehype.ts (1)
170-195: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
colSpanis ignored when counting columns.A row like
<tr><th colspan="4">…</th></tr>counts as one column, so a genuinely wide table can miss thewidemarker. Safe fallback (it just renders in the compact layout), but worth handling if spanned headers are expected in article tables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mdx/src/rehype.ts` around lines 170 - 195, Update maximumTableColumnCount when processing tr rows to account for each cell’s colSpan, summing the span value for th and td elements instead of counting every cell as one column; default missing, invalid, or non-positive colSpan values to one while preserving the existing recursive behavior for non-row nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/studio-compiler.ts`:
- Around line 54-59: Update the worker lifecycle around `#restartWorker` and
`#createWorker` to prevent unbounded immediate restarts after repeated error,
exit, or job-timeout failures. Add a bounded consecutive-failure guard with
cooldown/backoff or a circuit-open state, reset it after a successful worker
recovery, and ensure further compile requests receive the established
unavailable/error result while the circuit is open instead of spawning workers
continuously.
In `@packages/cli/src/studio-files.ts`:
- Around line 74-81: The temporary file created in durableWriteStudioFile must
preserve the requested permission bits despite umask. In
packages/cli/src/studio-files.ts#L74-L81, chmod the open temporary handle to
options.mode & 0o7777 before sync(); in
packages/cli/src/studio-files.test.ts#L16-L39, change the asserted mode to
include group/other-writable bits such as 0o664 so the test exercises umask
handling.
In `@packages/cli/src/studio-ui.ts`:
- Around line 985-1003: Update flushMarkdown to return a falsy failure signal
instead of the previous state when the draft PUT fails or returns an invalid
body. In save(), check the awaited flushMarkdown result and abort before issuing
the save request when it is falsy; preserve the existing success flow and
enterRich handling.
- Around line 711-735: Unify the splitter bounds used by the rendered ARIA
attributes, pointer dragging, fitSelectedViewport, and keyboard handling. Derive
a shared minimumSplit from the available width using the existing 320px/42%
rule, set aria-valuemin/max to minimumSplit and 100 - minimumSplit, and clamp
updateSplit, fitSelectedViewport, and arrow-key adjustments to those same
limits.
- Around line 570-586: Update the RichEditor draft request flow and the
onAccepted session update so accepted saves use the current Rich Text base disk
version rather than the stale session.baseDiskVersion. Ensure subsequent
/__scribe/api/rich-draft requests send a baseDiskVersion matching
context.state.diskVersion after acceptance, while preserving the existing
revision, source, and projection updates.
- Around line 886-897: Update the effect containing openDocument so a false
initial load schedules retries instead of returning permanently without creating
EventSource. Re-run openDocument until it succeeds or the effect is deactivated,
then establish EventSource and retain the existing refresh and connection
handlers. Ensure cleanup clears any pending retry and closes the event source.
In `@packages/cli/src/studio.ts`:
- Around line 806-812: The StudioFileConflictError branch in the request catch
block must broadcast the updated conflict state before returning the 409
response. After setting context.state.conflict, publish the current revision
using the existing events.publish pattern from the watcher error path, then
preserve the current response payload and status.
- Around line 631-643: In both draft mutation handlers, preserve the prior
context.state.recoveryBaseVersion before assigning the client base version, and
restore it in a catch if applyDraft throws; update the handlers at
packages/cli/src/studio.ts lines 631-643 and 687-697, following the
save-and-restore pattern used by /recover-discard. Keep successful applyDraft
behavior unchanged.
- Around line 1431-1459: Prevent unhandled rejections in closeStudioServers and
shutdownStep by attaching a no-op rejection handler immediately when creating
httpClosed and when receiving the operation promise, while preserving the
existing awaited error propagation and timeout behavior.
---
Outside diff comments:
In `@tests/release/package-manifests.test.ts`:
- Around line 6-7: Update the version constant in package-manifests.test.ts from
0.1.0-alpha.5 to 0.1.0-alpha.6 so manifest assertions match the published
package versions and internal `@scribe-sdk` dependencies.
---
Nitpick comments:
In `@packages/cli/src/studio-files.test.ts`:
- Around line 59-60: Update the test setup in the affected studio-files test to
import readdir from node:fs/promises at module scope, then call that imported
symbol directly instead of using the mid-test dynamic import. Preserve the
existing directory filtering and assertion behavior.
In `@packages/cli/src/studio-files.ts`:
- Around line 41-42: Update the file-loading flow around the Promise.all call to
await stat(resolvedPath) and validate info.isFile() before invoking
readFile(resolvedPath). Only read the path after it is confirmed to be a regular
file, preserving the existing error message and returned bytes/info behavior.
- Around line 118-125: Update detectLineEnding to count bare carriage returns
separately from CRLF sequences, and treat them as mixed line endings when
combined with LF or CRLF. Preserve the existing return values for valid uniform
LF and CRLF files, while preventing encodeStudioSource from silently rewriting
CR-only or mixed-ending content.
In `@packages/cli/src/studio-ui.test.ts`:
- Around line 5-114: Split the oversized test around studioClientModule and
studioStyles into focused tests covering imports/editor wiring, recovery and
lease behavior, shell/status behavior, and styles. Preserve all existing
assertions while grouping them by contract so failures identify the affected
area. Replace exact CSS-whitespace substring checks with formatting-insensitive
assertions where possible, especially the viewport, Monaco, and toaster style
checks.
In `@packages/cli/src/studio-ui.ts`:
- Around line 950-964: Extract the duplicated draft PUT request logic from the
three call sites around the effects near lines 870, 953, and 988 into a shared
putDraft helper. Have the helper construct the source, externalConflict, and
baseDiskVersion payload, reset browserRecoveryConflict on successful responses,
and consistently surface failures through the existing error toast, while
preserving each caller’s distinct post-processing such as apply(body).
- Around line 431-437: Update the external-value synchronization in the
useEffect using modelRef and applyingExternalValue so it replaces the model text
through Monaco’s full-model pushEditOperations API instead of model.setValue.
Add an undo stop around the replacement so the change remains in the editor’s
undo/redo history while preserving the existing equality guard and
applyingExternalValue lifecycle.
In `@packages/cli/src/studio.ts`:
- Around line 400-422: Add defensive .catch handlers to both coordinator.system
promise chains in the server.watcher "unlink" and "error" callbacks, matching
the protection already used by handleExternalChange. Keep the existing revision
updates and event publication unchanged, and ensure any rejection is handled
rather than becoming unhandled.
In `@packages/mdx/src/rehype.ts`:
- Around line 170-195: Update maximumTableColumnCount when processing tr rows to
account for each cell’s colSpan, summing the span value for th and td elements
instead of counting every cell as one column; default missing, invalid, or
non-positive colSpan values to one while preserving the existing recursive
behavior for non-row nodes.
In `@scripts/audit-public-packages.mjs`:
- Around line 10-13: Add a finite timeout option to the spawnSync invocation
used for the bun audit subprocess in the audit script, keeping the existing
command, working directory, and inherited stdio behavior unchanged. Configure
the timeout through the spawn options so a hung registry request terminates
instead of blocking indefinitely.
- Around line 10-13: Update the spawnSync invocation in the
audit-public-packages script to replace the undocumented “--production” argument
with Bun’s documented “--prod” flag, preserving the audit’s
production-dependency scope.
In `@tests/release/ci.test.ts`:
- Around line 63-64: Update the test around the auditScript assertions in
ci.test.ts to validate the exported public package list from
audit-public-packages.mjs rather than matching source-code formatting. Import
and assert the shared package-list symbol, while removing the brittle string
assertions that depend on declaration formatting or ordering.
In `@tests/studio-browser/studio.spec.ts`:
- Around line 98-117: Update the studio browser test around the Control+g
navigation and synchronizedCode assertion to locate target content by stable
text or rendered paragraph rather than absolute fixture line numbers. Replace
the hardcoded 9, 35, and data-scribe-source-line="33" dependencies with
content-based navigation and assertions that continue to verify editing and
preview synchronization.
- Around line 160-165: Replace the fixed waitForTimeout(160) in the viewport
menu flow with an assertion on a stable animation-settled condition, such as the
visible Mobile option reaching opacity 1 via toHaveCSS. Keep the screenshot and
subsequent mobileOption.click() actions unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34d7a6f7-6664-4cf1-8a17-d80be5fe78ce
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.changeset/clean-studios-compose.md.changeset/durable-studios-recover.md.changeset/pre.json.github/workflows/ci.ymlREADME.mdRELEASING.mdpackage.jsonpackages/cli/CHANGELOG.mdpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/init.test.tspackages/cli/src/rich-preservation.tspackages/cli/src/studio-compiler.test.tspackages/cli/src/studio-compiler.tspackages/cli/src/studio-events.test.tspackages/cli/src/studio-events.tspackages/cli/src/studio-files.test.tspackages/cli/src/studio-files.tspackages/cli/src/studio-fs.tspackages/cli/src/studio-lease.test.tspackages/cli/src/studio-lease.tspackages/cli/src/studio-recovery.test.tspackages/cli/src/studio-recovery.tspackages/cli/src/studio-security.test.tspackages/cli/src/studio-security.tspackages/cli/src/studio-transactions.test.tspackages/cli/src/studio-transactions.tspackages/cli/src/studio-ui.test.tspackages/cli/src/studio-ui.tspackages/cli/src/studio.test.tspackages/cli/src/studio.tspackages/mdx/CHANGELOG.mdpackages/mdx/README.mdpackages/mdx/package.jsonpackages/mdx/src/rehype.tspackages/mdx/src/table-normalization.test.tspackages/react/CHANGELOG.mdpackages/react/README.mdpackages/react/package.jsonpackages/styles/CHANGELOG.mdpackages/styles/README.mdpackages/styles/default.csspackages/styles/default.test.tspackages/styles/foundation.csspackages/styles/package.jsonplaywright.studio.config.tsscripts/audit-public-packages.mjsscripts/test-packed-consumers.mjstests/fixtures/studio-article.mdxtests/integration/next-css-modules/package.jsontests/integration/next-remote/package.jsontests/integration/next/package.jsontests/release/ci.test.tstests/release/documentation.test.tstests/release/package-manifests.test.tstests/studio-browser/studio.spec.ts
🛑 Comments failed to post (9)
packages/cli/src/studio-compiler.ts (1)
54-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No backoff/limit on worker restarts under persistent failure.
Every worker "error"/"exit" event or per-job timeout triggers an immediate
#createWorker()(Line 96). If the underlying failure is persistent (e.g. brokenmdxModuleUrl, a corrupted@scribe-sdk/mdxinstall, or module resolution failure), the newly spawned worker will fail again right away, causing an unbounded crash-restart loop with no cooldown — every subsequentcompile()call keeps getting rejected while the instance continuously churns through worker threads.Consider adding a restart counter/backoff (or a "circuit open" state after N consecutive failures within a window) so a persistent misconfiguration degrades gracefully (e.g. Studio surfaces "compiler unavailable" instead of spinning workers indefinitely).
♻️ Sketch of a restart guard
`#restartWorker`(worker: Worker, error: Error): void { if (this.#closed || this.#worker !== worker) return; this.#rejectPending(error); - this.#worker = this.#createWorker(); + const now = Date.now(); + if (now - this.#lastRestartAt < this.#restartWindowMs) { + this.#restartCount++; + } else { + this.#restartCount = 1; + } + this.#lastRestartAt = now; + if (this.#restartCount > this.#maxRestartsPerWindow) { + this.#closed = true; // or expose a distinct "unavailable" state + void worker.terminate(); + return; + } + this.#worker = this.#createWorker(); void worker.terminate(); }Also applies to: 93-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-compiler.ts` around lines 54 - 59, Update the worker lifecycle around `#restartWorker` and `#createWorker` to prevent unbounded immediate restarts after repeated error, exit, or job-timeout failures. Add a bounded consecutive-failure guard with cooldown/backoff or a circuit-open state, reset it after a successful worker recovery, and ensure further compile requests receive the established unavailable/error result while the circuit is open instead of spawning workers continuously.packages/cli/src/studio-files.ts (1)
74-81: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Source permission bits are filtered through
umask, and the test only passes because of the mode chosen.open(path, "wx", mode)requests a creation mode that the processumaskthen masks, sodurableWriteStudioFilesilently narrows permissions on commit.
packages/cli/src/studio-files.ts#L74-L81:chmodthe open temp handle tooptions.mode & 0o7777beforesync()so the requested mode is applied unconditionally.packages/cli/src/studio-files.test.ts#L16-L39: the0o640assertion survives only because0o640 & ~0o022 === 0o640; once the source is fixed, add a group/other-writable mode such as0o664so the umask path is actually exercised.📍 Affects 2 files
packages/cli/src/studio-files.ts#L74-L81(this comment)packages/cli/src/studio-files.test.ts#L16-L39🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.ts` around lines 74 - 81, The temporary file created in durableWriteStudioFile must preserve the requested permission bits despite umask. In packages/cli/src/studio-files.ts#L74-L81, chmod the open temporary handle to options.mode & 0o7777 before sync(); in packages/cli/src/studio-files.test.ts#L16-L39, change the asserted mode to include group/other-writable bits such as 0o664 so the test exercises umask handling.packages/cli/src/studio-ui.ts (4)
570-586: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect the rich-draft handler and its baseDiskVersion/conflict handling rg -nP -A 60 'api/rich-draft.*PUT|"/__scribe/api/rich-draft"' packages/cli/src/studio.ts rg -nP -n 'recoveryBaseVersion|diskVersion' packages/cli/src/studio.ts | head -60Repository: aetosdios27/scribe
Length of output: 5490
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate studio-ui.ts and inspect the Rich Text submission/update session areas. fd -a 'studio-ui\.ts$' . file="$(fd 'studio-ui\.ts$' . | head -n 1)" echo "== outline ==" ast-grep outline "$file" --view compact || true echo "== relevant lines 520-595 ==" sed -n '520,595p' "$file" | cat -n echo "== relevant publicStateWithRevision/draft API ==" sed -n '1160,1300p' packages/cli/src/studio.ts | cat -n echo "== save/createDraft references that change diskVersion ==" rg -n "save\\(|createDraft\\(|diskVersion =|recoveryBaseVersion =|externalConflict|acceptRichCandidate|SCB_RICH" packages/cli/src -g '*.ts' | head -120 # Read-only behavioral probe: model the accept rich-draft conflict condition from studio.ts and Studio session updates. python3 - <<'PY' base = {"source":"old","baseSource":"old","baseDiskVersion":"v1","revision":"r1"} saved = {"source":"old","baseSource":"old","baseDiskVersion":"v1","revision":"r1"} # accepts keeps original disk version later = {"source":"old","baseSource":"old","baseDiskVersion":"v1","revision":"r1"} # same stale value server_state_after_save = {"recoveryBaseVersion":"v1","diskVersion":"v2","richProjection":"same"} # server: baseDiskVersion defaults to body.baseDiskVersion, then conflict when !== state.diskVersion for session_name, session in [("base","base"), ("saved_unpatched", saved), ("saved_patched", {"source":"old","baseSource":"old","baseDiskVersion":"v2","revision":"r1"})]: baseDiskVersion = session.get("baseDiskVersion") or server_state_after_save["recoveryBaseVersion"] conflict = server_state_after_save["recoveryBaseVersion"] != server_state_after_save["diskVersion"] print(session_name, {"baseDiskVersion":baseDiskVersion, "recovery_conflict_before": bool(server_state_after_save["recoveryConflict"]) if "recoveryConflict" in server_state_after_save else None, "submitted_conflict": bool(conflict)}) PYRepository: aetosdios27/scribe
Length of output: 17565
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== studio.ts save/createDraft/diskVersion sections ==" sed -n '330,390p' packages/cli/src/studio.ts | cat -n sed -n '720,780p' packages/cli/src/studio.ts | cat -n sed -n '825,888p' packages/cli/src/studio.ts | cat -n sed -n '1260,1298p' packages/cli/src/studio.ts | cat -n echo "== rich-draft API request/response ==" python3 - <<'PY' from pathlib import Path p = Path('packages/cli/src/studio-ui.ts') text = p.read_text() start = text.index('/__scribe/api/rich-draft') print(text[start:start+550]) PY echo "== JS object merge model ==" node - <<'JS' const session = { source: 'old', baseSource: 'old', baseDiskVersion: 'v1', revision: 'r1', projectionMarkdown: 'proj', islands: [] }; const body = { source: 'new', revision: 'r2', projectionMarkdown: 'proj', islands: [] }; const acceptedUnpatched = { ...session, revision: body.revision, projectionMarkdown: body.projectionMarkdown, islands: body.islands, baseSource: body.source }; const acceptedPatched = { ...session, revision: body.revision, projectionMarkdown: body.projectionMarkdown, islands: body.islands, baseSource: body.source, baseDiskVersion: body.diskVersion ?? session.baseDiskVersion }; const nextState = { diskVersion: 'v2', recoveryBaseVersion: 'v1' }; function submittedBaseDiskVersion(session, serverBase) { return typeof session.baseDiskVersion === "string" ? session.baseDiskVersion : serverBase; } console.log(JSON.stringify({ baseSourceAfterUnpatched: acceptedUnpatched.baseSource, baseDiskVersionAfterUnpatched: acceptedUnpatched.baseDiskVersion, baseSourceAfterPatched: acceptedPatched.baseSource, baseDiskVersionAfterPatched: acceptedPatched.baseDiskVersion, unpatchedSubmittedBaseAfterSave: submittedBaseDiskVersion(acceptedUnpatched, nextState.recoveryBaseVersion), patchedSubmittedBaseAfterSave: acceptedPatched.baseDiskVersion, unpatchedConflictIfUnchangedAfterSave: submittedBaseDiskVersion(acceptedUnpatched, nextState.recoveryBaseVersion) !== nextState.diskVersion, patchedConflictIfBodyNoDiskVersion: typeof body.diskVersion === 'undefined', }, null, 2)); JSRepository: aetosdios27/scribe
Length of output: 14158
Prevent Rich Text drafts from comparing against the stale
baseDiskVersion.
onAcceptedupdatesbaseSource, butRichEditorstill sends the originalsession.baseDiskVersionon later/__scribe/api/rich-draftrequests. That value is lower thanstate.diskVersionafter a save, sobaseDiskVersion !== context.state.diskVersionkeeps settingstate.conflictand can trigger stale-revision/conflict UI flow. Use a stable Rich Text base rather thansession.baseDiskVersion, or ensure the accepted session carries the correct current base value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 570 - 586, Update the RichEditor draft request flow and the onAccepted session update so accepted saves use the current Rich Text base disk version rather than the stale session.baseDiskVersion. Ensure subsequent /__scribe/api/rich-draft requests send a baseDiskVersion matching context.state.diskVersion after acceptance, while preserving the existing revision, source, and projection updates.
711-735: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Splitter's
aria-valuemin/aria-valuemaxdon't match the range the control actually produces.Pointer drag and
fitSelectedViewportclamp tominimum = Math.min(42, 320 / width * 100)…100 - minimum, soaria-valuenowfrequently lands outside the advertised 25–75 window, and keyboard arrows clamp to a third range (25–75), causing a visible jump after a drag. Derive all three from the same bounds.♻️ Suggested change
+ const minimumSplit = 25; <div className="studio-splitter" role="separator" aria-label="Resize editor and preview" aria-orientation="vertical" - aria-valuemin="25" - aria-valuemax="75" + aria-valuemin={minimumSplit} + aria-valuemax={100 - minimumSplit} aria-valuenow={Math.round(split)}and clamp
updateSplit/fitSelectedViewportwith the sameminimumSplit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 711 - 735, Unify the splitter bounds used by the rendered ARIA attributes, pointer dragging, fitSelectedViewport, and keyboard handling. Derive a shared minimumSplit from the available width using the existing 320px/42% rule, set aria-valuemin/max to minimumSplit and 100 - minimumSplit, and clamp updateSplit, fitSelectedViewport, and arrow-key adjustments to those same limits.
886-897: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No retry when the initial document load fails — the UI shows "Reconnecting" forever.
If
openDocument()returnsfalse(server not yet up, transient 5xx), theEventSourceis never created, so nothing ever flipsconnectedback totrueand no furtherrefresh()runs. The status bar advertises "Reconnecting" while the client has actually given up until a manual page reload.♻️ Suggested change
- void openDocument().then((opened) => { - if (!active || !opened) return; - events = new EventSource("/__scribe/api/events"); - events.onopen = () => setConnected(true); - events.onerror = () => setConnected(false); - events.onmessage = () => void refresh(); - }); + let retry; + const start = async () => { + if (!active) return; + if (!await openDocument()) { + retry = setTimeout(start, 2_000); + return; + } + events = new EventSource("/__scribe/api/events"); + events.onopen = () => setConnected(true); + events.onerror = () => setConnected(false); + events.onmessage = () => void refresh(); + }; + void start(); return () => { active = false; + clearTimeout(retry); events?.close(); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let retry; const start = async () => { if (!active) return; if (!await openDocument()) { retry = setTimeout(start, 2_000); return; } events = new EventSource("/__scribe/api/events"); events.onopen = () => setConnected(true); events.onerror = () => setConnected(false); events.onmessage = () => void refresh(); }; void start(); return () => { active = false; clearTimeout(retry); events?.close(); }; }, [apply]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 886 - 897, Update the effect containing openDocument so a false initial load schedules retries instead of returning permanently without creating EventSource. Re-run openDocument until it succeeds or the effect is deactivated, then establish EventSource and retain the existing refresh and connection handlers. Ensure cleanup clears any pending retry and closes the event source.
985-1003: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
flushMarkdownfailure is indistinguishable from success for callers.On a failed draft PUT it returns the previous
state, which is a truthy document object.save()(Line 1080) awaits it without checking and then issuesPUT /__scribe/api/save, persisting the server's older draft while the editor still holds unsent text — the user sees "Saved to …" after losing the last edits. Return a failure signal (e.g.undefined) and abort the save.🔧 Suggested change
if (!response.ok || typeof body.source !== "string") { toast.error(body.error || "Studio could not preserve this draft."); - return state; + return undefined; }and in
save():- } else await flushMarkdown(); + } else if (!await flushMarkdown()) { + setSavePhase("error"); + return; + }Note
enterRich(Line 1010) already treats a falsy result as a failure, so it benefits too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-ui.ts` around lines 985 - 1003, Update flushMarkdown to return a falsy failure signal instead of the previous state when the draft PUT fails or returns an invalid body. In save(), check the awaited flushMarkdown result and abort before issuing the save request when it is falsy; preserve the existing success flow and enterRich handling.packages/cli/src/studio.ts (3)
631-643: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
recoveryBaseVersionis committed before a fallibleapplyDraftin both draft handlers. Both mutation endpoints write the client-supplied base version into shared state and only then perform the work that can throw, so a failure leaves the recovery base pointing at a draft that was never applied./recover-discard(Lines 877-884) already demonstrates the correct save-and-restore pattern.
packages/cli/src/studio.ts#L631-L643: capture the previousrecoveryBaseVersionand restore it in acatcharound theapplyDraftcall.packages/cli/src/studio.ts#L687-L697: apply the same capture-and-restore around theapplyDraftcall in the Rich Text path.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
📍 Affects 1 file
packages/cli/src/studio.ts#L631-L643(this comment)packages/cli/src/studio.ts#L687-L697🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 631 - 643, In both draft mutation handlers, preserve the prior context.state.recoveryBaseVersion before assigning the client base version, and restore it in a catch if applyDraft throws; update the handlers at packages/cli/src/studio.ts lines 631-643 and 687-697, following the save-and-restore pattern used by /recover-discard. Keep successful applyDraft behavior unchanged.
806-812: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Conflict state set in the catch block is never broadcast.
Line 808 flips
state.conflictoutside the coordinator and returns 409, but noevents.publishfollows, so other connected tabs keep rendering a non-conflicted document until the next unrelated revision. Publish the current revision here (as the watcher error path at Line 395 already does).🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 806 - 812, The StudioFileConflictError branch in the request catch block must broadcast the updated conflict state before returning the 409 response. After setting context.state.conflict, publish the current revision using the existing events.publish pattern from the watcher error path, then preserve the current response payload and status.
1431-1459: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Eagerly created promises can reject before a handler is attached, producing an unhandled rejection.
Two spots:
httpClosed(Lines 1434-1438) is constructed immediately but only awaited in thefinallyat Line 1443 — after up to two 3-secondshutdownStepawaits. Ifhttp.close()invokes its callback with an error early (for exampleERR_SERVER_NOT_RUNNINGaftercloseAllConnections), the rejection is unhandled at the microtask checkpoint and Node's default policy terminates the process during shutdown.- In
shutdownStep, when the timeout wins the race the originaloperationpromise is abandoned; a later rejection from it is likewise unhandled.Attaching a no-op handler at creation keeps the awaited semantics intact while preventing both cases.
🛡️ Proposed fix
const httpClosed = !http.listening ? Promise.resolve() : new Promise<void>((resolveClose, rejectClose) => { http.close((error) => error === undefined ? resolveClose() : rejectClose(error)); }); + httpClosed.catch(() => undefined);async function shutdownStep(operation: Promise<unknown>, label: string): Promise<void> { + operation.catch(() => undefined); let timer: ReturnType<typeof setTimeout> | undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async function closeStudioServers(vite: ViteDevServer, http: HttpServer, compiler: StudioCompiler): Promise<void> { http.closeIdleConnections(); http.closeAllConnections(); const httpClosed = !http.listening ? Promise.resolve() : new Promise<void>((resolveClose, rejectClose) => { http.close((error) => error === undefined ? resolveClose() : rejectClose(error)); }); httpClosed.catch(() => undefined); try { await shutdownStep(compiler.close(), "compiler worker"); await shutdownStep(vite.close(), "Vite server"); } finally { await shutdownStep(httpClosed, "Studio HTTP server"); } } async function shutdownStep(operation: Promise<unknown>, label: string): Promise<void> { operation.catch(() => undefined); let timer: ReturnType<typeof setTimeout> | undefined; try { await Promise.race([ operation, new Promise<never>((_, reject) => { timer = setTimeout(() => reject(new Error(`Timed out closing the ${label}.`)), 3_000); }) ]); } finally { if (timer !== undefined) clearTimeout(timer); } }🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 1431 - 1459, Prevent unhandled rejections in closeStudioServers and shutdownStep by attaching a no-op rejection handler immediately when creating httpClosed and when receiving the operation promise, while preserving the existing awaited error propagation and timeout behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/cli/src/studio-files.ts (1)
101-109: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPost-rename verification failure reports a write that already happened.
The
renameon Line 101 has already committed, so ifcommitted.equals(bytes)fails the thrown message ("could not verify the bytes written") reads like the save was a no-op while the source file has in fact been replaced. Callers deciding whether to retry or keep the draft dirty need to know the target was mutated. Make the message explicit that the file was replaced but verification failed.💡 Proposed fix
const committed = await readFile(options.resolvedPath); if (!committed.equals(bytes)) { - throw new Error("Studio could not verify the bytes written to the source file."); + throw new Error( + `Studio replaced ${options.resolvedPath} but could not verify the bytes written. The file on disk may not match the saved draft.` + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.ts` around lines 101 - 109, Update the post-rename verification error in the studio file save flow to state that the source file was replaced but byte verification failed. Preserve the existing verification condition and error-throw behavior, changing only the message near rename and readFile.packages/cli/src/studio-compiler.ts (2)
150-161: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-object throws crash the worker.
If
compileScribeMdxrejects with a string,null, or any non-object (e.g. anERR_MODULE_NOT_FOUNDsurfaced oddly, or a thrown primitive),diagnostic.ruleIdon Line 156 throws aTypeErrorinside the async listener. That becomes an unhandled rejection in the worker thread, killing it and forcing a restart instead of returning a clean error diagnostic — and it counts toward the circuit breaker. Normalize the caught value first.🛡️ Proposed fix
} catch (error) { - const diagnostic = error; + const diagnostic = (typeof error === "object" && error !== null) ? error : {}; parentPort.postMessage({ id, diagnostics: [{ severity: "error", code: diagnostic.ruleId ?? "SCB0001", message: diagnostic.reason ?? diagnostic.message ?? String(error),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-compiler.ts` around lines 150 - 161, Normalize the caught value in the catch block surrounding compileScribeMdx before reading diagnostic.ruleId, reason, message, line, or column. Ensure string, null, and other primitive throws become a safe diagnostic object while preserving available Error or diagnostic fields, so parentPort.postMessage always returns one clean error diagnostic without throwing.
142-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap fatal vfile messages to Studio errors.
file.messagesincludes fatal messages, but every diagnostic is reported withseverity: "warning", so non-throwing fatal validation failures can be saved as broken content. Derive severity frommessage.fatal, e.g.message.fatal === true ? "error" : "warning".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-compiler.ts` around lines 142 - 148, Update the diagnostics mapping in the Studio compiler to derive severity from each message’s fatal flag: report messages with message.fatal === true as errors and all other messages as warnings. Preserve the existing code, message, line, and column mapping.packages/cli/src/studio.test.ts (1)
130-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
recoveryRoothere is not tracked for cleanup.The two new rollback tests register their recovery roots with
fixtureRoots(Lines 465 and 499), but this one does not, so its temp tree survives the run. Same one-line fix.♻️ Proposed fix
const recoveryRoot = await mkdtemp(join(tmpdir(), "scribe recovery failure ")); + fixtureRoots.add(recoveryRoot); await mkdir(join(recoveryRoot, "recovery", `${studioRecoveryKey(file.path)}.json`), { recursive: true });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.test.ts` around lines 130 - 143, Register recoveryRoot with fixtureRoots immediately after creating it in the “surfaces recovery read failures before opening the Studio” test, matching the cleanup setup used by the other rollback tests.packages/cli/src/studio.ts (2)
1452-1467: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSequential awaits in
closeStudioServersskip later cleanup steps on earlier failure.
compiler.close()andvite.close()are awaited sequentially inside onetry. IfshutdownStep(compiler.close(), ...)rejects (including via its own 3s timeout), the thrown error skipsawait shutdownStep(vite.close(), ...)entirely — only thefinallyblock's HTTP-server close still runs. The Vite dev server (file watchers, HMR websocket, dependency optimizer) is left running even though shutdown was explicitly requested. Since each step already has an independent timeout precisely to bound worst-case duration, the steps should also be attempted independently (e.g. viaPromise.allSettled) so one failure doesn't prevent the others from being attempted.🔧 Proposed fix
async function closeStudioServers(vite: ViteDevServer, http: HttpServer, compiler: StudioCompiler): Promise<void> { http.closeIdleConnections(); http.closeAllConnections(); const httpClosed = !http.listening ? Promise.resolve() : new Promise<void>((resolveClose, rejectClose) => { http.close((error) => error === undefined ? resolveClose() : rejectClose(error)); }); void httpClosed.catch(() => undefined); - try { - await shutdownStep(compiler.close(), "compiler worker"); - await shutdownStep(vite.close(), "Vite server"); - } finally { - await shutdownStep(httpClosed, "Studio HTTP server"); - } + const results = await Promise.allSettled([ + shutdownStep(compiler.close(), "compiler worker"), + shutdownStep(vite.close(), "Vite server"), + shutdownStep(httpClosed, "Studio HTTP server") + ]); + const failure = results.find((result): result is PromiseRejectedResult => result.status === "rejected"); + if (failure) throw failure.reason; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 1452 - 1467, Update closeStudioServers so compiler.close() and vite.close() are initiated and awaited independently, using Promise.allSettled or equivalent, ensuring a compiler shutdown failure or timeout does not skip the Vite shutdown. Preserve the existing shutdownStep labels, timeout behavior, and finally-block cleanup of httpClosed.
300-315: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRemove
optimizeDeps.noDiscoveryor move package dependencies intoinclude.
noDiscovery: truemeans Vite will only pre-bundle the dependencies listed inoptimizeDeps.include. The included runtime deps are not bare package imports, while the actual package dependencies imported by studio code, such asmonaco-editor, are omitted. This can leave CJS/ESM package imports unoptimized during the initial dev load; either list the bare package includes this config is intended to cover, or removenoDiscoveryso normal discovery runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 300 - 315, Update the optimizeDeps configuration in the studio Vite setup: either remove noDiscovery to restore automatic dependency discovery, or keep it only after adding every relevant bare package dependency imported by the studio code, including monaco-editor, to include. Preserve the existing intended pre-bundling behavior for the listed runtime dependencies.
🧹 Nitpick comments (5)
packages/cli/src/studio-files.test.ts (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp fixture directories are never removed.
sourceFilecreates a newmkdtemproot per test and nothing deletes them, so each run leaks directories in the OS temp dir. The sibling suitepackages/cli/src/studio.test.ts(Lines 19-28) tracks roots in a set and cleans them inafterEach/afterAll; mirroring that here keeps the suites consistent.♻️ Proposed cleanup
-import { chmod, lstat, mkdtemp, readFile, readdir, symlink, writeFile } from "node:fs/promises"; +import { chmod, lstat, mkdtemp, readFile, readdir, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { expect, it } from "vitest"; +import { afterEach, expect, it } from "vitest"; import { durableWriteStudioFile, readStudioFile, StudioFileConflictError } from "./studio-files.js"; +const roots = new Set<string>(); + +afterEach(async () => { + await Promise.all([...roots].map((root) => rm(root, { force: true, recursive: true }))); + roots.clear(); +}); + async function sourceFile(source = "# Original\n"): Promise<string> { const root = await mkdtemp(join(tmpdir(), "scribe durable ")); + roots.add(root); const path = join(root, "article.mdx"); await writeFile(path, source); return path; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-files.test.ts` around lines 9 - 14, Update the studio-files test fixture flow around sourceFile to track each mkdtemp root and remove the tracked directories after each test and after the suite completes, mirroring the cleanup pattern used by the sibling studio.test suite.packages/cli/src/studio-compiler.ts (1)
103-121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: reset the failure counter when the cooldown expires.
#consecutiveFailuresonly resets on a successful worker message (Line 56). After a cooldown lapses, the counter is still at/abovemaximumConsecutiveFailures, so a single subsequent fault immediately reopens the circuit without a fresh window of attempts. Zeroing it when the circuit is allowed to close makes the breaker behavior half-open rather than sticky.♻️ Half-open reset
`#availableWorker`(): Worker { if (this.#worker !== undefined) return this.#worker; if (Date.now() < this.#circuitOpenUntil) { throw new Error("Studio compiler is temporarily unavailable after repeated worker failures. Try again shortly."); } + this.#circuitOpenUntil = 0; + this.#consecutiveFailures = 0; this.#worker = this.#createWorker(); return this.#worker; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio-compiler.ts` around lines 103 - 121, Reset `#consecutiveFailures` in `#availableWorker` when `#circuitOpenUntil` has elapsed and the circuit is about to allow a new worker, before creating it. Preserve the existing temporary-unavailability check while starting the next cooldown window with a fresh failure count.packages/cli/src/studio.ts (3)
1071-1088: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a comment explaining the
_createMdxContent→MdxContentBodyrename.The rename trick (making the generated MDX body function look like a capitalized component so the React refresh plugin treats it as a fast-refresh boundary) is non-obvious from the code alone. A short comment would save the next reader from re-deriving the intent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 1071 - 1088, Add a concise comment near the `_createMdxContent` to `MdxContentBody` rename in `renameGeneratedMdxBody`, explaining that capitalizing the generated MDX body function makes the React refresh plugin recognize it as a fast-refresh boundary.
643-650: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated recovery-base-version guard pattern.
The "snapshot previous
recoveryBaseVersion, speculatively assign, restore onapplyDraftfailure" pattern is repeated verbatim at Lines 643-650 (draft PUT), Lines 701-713 (rich-draft PUT), and Lines 898-905 (recover-discard). Consider extracting a small helper, e.g.withRecoveryBaseVersion(context.state, baseDiskVersion, () => applyDraft(...)), to avoid triple maintenance if the rollback semantics ever change.♻️ Sketch
async function withRecoveryBaseVersion<T>( state: StudioState, nextBaseVersion: string, action: () => Promise<T> ): Promise<T> { const previous = state.recoveryBaseVersion; state.recoveryBaseVersion = nextBaseVersion; try { return await action(); } catch (error) { state.recoveryBaseVersion = previous; throw error; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 643 - 650, Extract the repeated recovery-base-version rollback logic into a shared withRecoveryBaseVersion helper near the existing studio utilities, preserving its generic return value and error propagation. Replace the inline snapshot/assignment/try-catch blocks in the draft PUT, rich-draft PUT, and recover-discard flows with this helper while keeping each existing action unchanged.
1146-1195: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTheme toggle remounts the entire article tree.
componentsis memoized on[theme], and itsWrapperis defined inside that memo. Every theme change therefore produces a brand-newWrapperfunction identity, so React unmounts/remounts the wholeArticletree (losing scroll position and any transient DOM state, and re-triggeringStudioBanner/StudioImageasync checks) instead of just re-rendering with the new theme.Consider keeping
Wrapperstable and passingthemevia a ref/context read at render time, or readingdata-themefrom a mutable ref, instead of recreating the component per theme change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/studio.ts` around lines 1146 - 1195, Keep the Wrapper component identity stable in PreviewApp instead of defining it inside the theme-dependent useMemo. Pass the current theme through a ref or context that Wrapper reads during rendering, while preserving the existing data-theme behavior and createScribeComponents integration; ensure theme changes re-render without remounting the Article tree.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/studio-files.ts`:
- Around line 75-79: Update the temporary-file creation and chmod calls in the
surrounding studio file write flow to mask options.mode with 0o777 instead of
0o7777. Ensure the temporary file created by open and subsequently updated by
temporary.chmod never preserves setuid, setgid, or sticky bits.
In `@packages/styles/foundation.css`:
- Around line 130-132: Document the wide-table selector contract change in a
changeset or migration note for direct `@scribe-sdk/styles` consumers. Explain
that hand-authored .scribe-table-scroll tables must add the compiler-emitted
data-scribe-table-layout="wide" attribute or provide their own CSS to preserve
column-count-based horizontal scrolling, covering both updated wide-table style
rules.
In `@scripts/audit-public-packages.mjs`:
- Around line 138-146: Update run to throw an error when result.status is
nonzero instead of calling process.exit, preserving the nonzero failure status
while allowing auditPublicPackages()’s finally cleanup to execute. Keep the
existing result.error handling unchanged.
---
Outside diff comments:
In `@packages/cli/src/studio-compiler.ts`:
- Around line 150-161: Normalize the caught value in the catch block surrounding
compileScribeMdx before reading diagnostic.ruleId, reason, message, line, or
column. Ensure string, null, and other primitive throws become a safe diagnostic
object while preserving available Error or diagnostic fields, so
parentPort.postMessage always returns one clean error diagnostic without
throwing.
- Around line 142-148: Update the diagnostics mapping in the Studio compiler to
derive severity from each message’s fatal flag: report messages with
message.fatal === true as errors and all other messages as warnings. Preserve
the existing code, message, line, and column mapping.
In `@packages/cli/src/studio-files.ts`:
- Around line 101-109: Update the post-rename verification error in the studio
file save flow to state that the source file was replaced but byte verification
failed. Preserve the existing verification condition and error-throw behavior,
changing only the message near rename and readFile.
In `@packages/cli/src/studio.test.ts`:
- Around line 130-143: Register recoveryRoot with fixtureRoots immediately after
creating it in the “surfaces recovery read failures before opening the Studio”
test, matching the cleanup setup used by the other rollback tests.
In `@packages/cli/src/studio.ts`:
- Around line 1452-1467: Update closeStudioServers so compiler.close() and
vite.close() are initiated and awaited independently, using Promise.allSettled
or equivalent, ensuring a compiler shutdown failure or timeout does not skip the
Vite shutdown. Preserve the existing shutdownStep labels, timeout behavior, and
finally-block cleanup of httpClosed.
- Around line 300-315: Update the optimizeDeps configuration in the studio Vite
setup: either remove noDiscovery to restore automatic dependency discovery, or
keep it only after adding every relevant bare package dependency imported by the
studio code, including monaco-editor, to include. Preserve the existing intended
pre-bundling behavior for the listed runtime dependencies.
---
Nitpick comments:
In `@packages/cli/src/studio-compiler.ts`:
- Around line 103-121: Reset `#consecutiveFailures` in `#availableWorker` when
`#circuitOpenUntil` has elapsed and the circuit is about to allow a new worker,
before creating it. Preserve the existing temporary-unavailability check while
starting the next cooldown window with a fresh failure count.
In `@packages/cli/src/studio-files.test.ts`:
- Around line 9-14: Update the studio-files test fixture flow around sourceFile
to track each mkdtemp root and remove the tracked directories after each test
and after the suite completes, mirroring the cleanup pattern used by the sibling
studio.test suite.
In `@packages/cli/src/studio.ts`:
- Around line 1071-1088: Add a concise comment near the `_createMdxContent` to
`MdxContentBody` rename in `renameGeneratedMdxBody`, explaining that
capitalizing the generated MDX body function makes the React refresh plugin
recognize it as a fast-refresh boundary.
- Around line 643-650: Extract the repeated recovery-base-version rollback logic
into a shared withRecoveryBaseVersion helper near the existing studio utilities,
preserving its generic return value and error propagation. Replace the inline
snapshot/assignment/try-catch blocks in the draft PUT, rich-draft PUT, and
recover-discard flows with this helper while keeping each existing action
unchanged.
- Around line 1146-1195: Keep the Wrapper component identity stable in
PreviewApp instead of defining it inside the theme-dependent useMemo. Pass the
current theme through a ref or context that Wrapper reads during rendering,
while preserving the existing data-theme behavior and createScribeComponents
integration; ensure theme changes re-render without remounting the Article tree.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7048290-4519-4e77-bc88-124bb12a4f49
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.changeset/clean-studios-compose.md.changeset/pre.jsonREADME.mdpackages/cli/CHANGELOG.mdpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/studio-compiler.test.tspackages/cli/src/studio-compiler.tspackages/cli/src/studio-files.test.tspackages/cli/src/studio-files.tspackages/cli/src/studio-ui.test.tspackages/cli/src/studio-ui.tspackages/cli/src/studio.test.tspackages/cli/src/studio.tspackages/mdx/CHANGELOG.mdpackages/mdx/README.mdpackages/mdx/package.jsonpackages/mdx/src/rehype.tspackages/mdx/src/table-normalization.test.tspackages/react/CHANGELOG.mdpackages/react/README.mdpackages/react/package.jsonpackages/styles/CHANGELOG.mdpackages/styles/README.mdpackages/styles/default.csspackages/styles/default.test.tspackages/styles/foundation.csspackages/styles/package.jsonscripts/audit-public-packages.d.mtsscripts/audit-public-packages.mjstests/fixtures/studio-article.mdxtests/release/changesets.test.tstests/release/ci.test.tstests/release/package-manifests.test.tstests/studio-browser/studio.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- .changeset/clean-studios-compose.md
- packages/react/package.json
- .changeset/pre.json
- packages/styles/default.css
- packages/cli/package.json
- packages/cli/CHANGELOG.md
- tests/fixtures/studio-article.mdx
- packages/styles/CHANGELOG.md
- packages/cli/src/studio-compiler.test.ts
- packages/mdx/src/rehype.ts
- tests/studio-browser/studio.spec.ts
- packages/cli/src/studio-ui.ts
Linear
Refs AET-15
Summary
scribeas the primary executable while retainingscbas a compatibility alias0.1.0-alpha.6and generate the fixed-group changelogsVerification already completed locally
0.1.0-alpha.6git diff --checkRelease boundary
This PR prepares the exact alpha.6 candidate. npm publication remains a separate manual action after the hosted matrix is green. The
alphadist-tag will move to alpha.6;latestmust remain unchanged.Summary by CodeRabbit
New Features
Bug Fixes
Documentation