Repository navigation
Fix the docs outage, and let MCP actually file a document - #241
Conversation
The docs tree change added doc.sort_order, doc.search_vector, doc_collection.position and doc_access_request, and shipped doc-tree-order-catchup.sql to seed the new order. That script only backfills values, so a database that never had the columns applied answers every read of a doc with 42703 column "sort_order" does not exist, which takes out /docs and /api/sync together. Add the DDL half, idempotent and in one transaction like the other catchup scripts, plus doc.slug for a database old enough to miss it.
Applying schema is a manual step someone has to remember, and the docs tree change is what happens when it is forgotten: code shipped naming a column the database did not have. db:check-drift compares every table the Drizzle schema declares against information_schema and exits non zero on anything missing, so CI can refuse to ship code the target database cannot answer. It also lists the tables the schema no longer declares, because those are exactly what db:push would drop, and push is not safe against a database holding data: it runs drizzle-kit push --force. db:catchup applies one file from packages/db/catchup over the configured connection, so bringing a database up to date is a command rather than a paste into a SQL console.
A doc lives in a folder or in a project, never both. Moving already refused the combination; creating and updating quietly allowed it, which is how a doc ends up claiming a folder while the sidebar files it under its project. Filing a doc now detaches it from its project and attaching it to a project unfiles it, and naming both in one call is refused. listDocCollections carries docCount, counting only the live docs the caller may read, so a folder can be seen to be empty without listing every doc. listDocs takes unfiled, so the docs in no folder are findable rather than merely absent from every folder. Deleting a folder takes an optional collection to hand its docs to, and the docs are never deleted either way. A version snapshot compared the previous body in the app, so every autosave of a large doc dragged the last saved copy out of Postgres to decide whether anything had changed. Compare it in SQL instead.
The doc tools could create a folder and list folders, and nothing else: no tool could put a document into one. Passing collection to create_doc or update_doc was accepted and dropped, so a migration could file 148 documents and leave every folder empty without one error. create_doc and update_doc take collection, update_doc takes parent, and both resolve a folder by id then by name, refusing an ambiguous name rather than guessing. list_docs filters by collection and by unfiled. list_doc_collections carries docCount and position. Adds update_doc_collection, delete_doc_collection with reassignTo, move_doc, unarchive_doc and delete_doc. move_doc places a document against a named sibling the way dragging it does, by reading the destination siblings and passing both anchors, so after and before mean what they say rather than naming one edge. Resolving a document now falls back to the archived ones, otherwise unarchive_doc could never name the document it exists to restore. defineTool builds a strict object, so an unrecognised argument is a validation error on every tool rather than a silent no-op.
A doc opened in the rich text editor with a full formatting bar pinned above it, under a header, under a row of pills, so four bands of chrome sat between the breadcrumb and the first line of text, and the prose was held to 45rem however wide the window was. A doc now opens in markdown, which is what everything is written in, and the formatting bar starts hidden behind a toggle beside the mode switch. Both stick, per person, in local storage. The pill row above the title is gone from the editing surface: the breadcrumb in the header already says which folder and project a doc is in. The title, the reader padding and the space under the metadata are tighter, and the reader takes a width the reader chooses, defaulting to 68rem, with Comfortable, Wide and Full width in the doc menu. Uploading moves into use-doc-uploads so the editor is about editing.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds shared document preferences and upload handling, configurable reading widths, expanded document organization and lifecycle tools, stricter placement validation, an idempotent schema catch-up migration, and PostgreSQL drift-checking utilities. ChangesWeb document experience
Document organization tools
Database operations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCP as MCP document tools
participant Core as Core document service
participant DB as PostgreSQL
participant Sync as Sync publisher
MCP->>Core: resolve document and planned placement
Core->>DB: read placement and sibling state
DB-->>Core: document and collection data
Core->>DB: apply move or collection operation
DB-->>Core: updated document actions
Core->>Sync: publish resulting actions
Sync-->>MCP: return operation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
…lter The description promised newest first while the query orders by the sidebar order, or by relevance once a query is given. Asking for one collection and for the unfiled documents in the same call answered with nothing, because no document is both. Say so instead.
A move that named a parent and a neighbour looked for that neighbour in the collection the arguments asked for, but a doc nested under a parent inherits the parent's collection and project instead. The sibling set was the wrong one, so a valid move was refused with a neighbour that does not sit where the document is moving to. Ask core where the doc will land, through the same resolution the move itself runs, rather than deriving a second answer here that can drift from it.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/features/docs/editor/editor-toolbar.tsx (1)
152-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHide all rich-text formatting controls when
collapsedis true.Line 152 hides only undo, redo, and text-style controls. Bold, italic, lists, links, and other formatting controls still render below it.
DocEditorpassestoolbarCollapsed={!toolbar}and labels the action "Hide formatting". The rich editor therefore does not apply the saved toolbar preference. Gate the remaining formatting controls on!collapsed, while keeping the mode switch and upload status available.🤖 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 `@apps/web/src/features/docs/editor/editor-toolbar.tsx` around lines 152 - 205, Update the editor toolbar rendering so every rich-text formatting control, including bold, italic, lists, links, and related controls, is gated by !collapsed just like the existing undo, redo, and text-style block. Keep the mode switch and upload status rendered when collapsed, and preserve the current compact behavior.
🤖 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 `@apps/web/src/features/docs/doc-editor.tsx`:
- Around line 137-140: Update the tab option className in the mode selector to
use the shared tab interaction helper instead of the raw transition-colors and
hover:text-muted classes, while preserving the existing active-state styling and
layout classes.
In `@apps/web/src/features/docs/doc-header.tsx`:
- Around line 120-139: Update ReadingWidthItems to use the existing Radix
dropdown radio-group and radio-group-item primitives for the mutually exclusive
reading widths. Bind the group value to width, update selection through
setWidth, and expose each option’s checked state while preserving the current
labels, test IDs, and styling.
In `@apps/web/src/features/docs/use-doc-preferences.ts`:
- Around line 79-90: Add store behavior tests alongside the existing
parsePreferences tests, covering setter persistence, storage-event updates to
subscribers, and resetDocPreferences refreshing the in-memory snapshot. Exercise
the public preference store APIs and verify persisted storage, listener
notifications, and updated snapshots so persistence and cross-tab
synchronization regressions are detected.
- Around line 27-40: Replace the manual validation and cast in parsePreferences
with a shared docPreferencesSchema exported from `@orbit/shared`, using safeParse
for localStorage input. Preserve DEFAULT_DOC_PREFERENCES for invalid top-level
data and retain per-field fallback behavior by merging parsed values with the
defaults when validation fails partially.
In `@packages/core/src/content/doc-service.ts`:
- Around line 1480-1507: Update deleteDocCollection and its emptyCollection call
to enforce document-level write authorization for every document being
reassigned or removed, using assertDocWritable or the existing equivalent before
applying changes. Preserve the collection-level doc:write check and ensure
unauthorized private documents cannot be moved through collection deletion.
In `@packages/db/catchup/doc-tree-schema-catchup.sql`:
- Around line 21-24: Move the `begin;` statement before the `ALTER TYPE
public.notification_reason ... ADD VALUE` statements so both enum additions
execute within the transaction. Keep the existing enum ordering and `if not
exists` behavior unchanged; document a non-atomic path only if pre-16 PostgreSQL
deployments must be supported.
In `@packages/db/src/check-drift.ts`:
- Around line 125-133: At packages/db/src/check-drift.ts:125-133, update the
import.meta.main CLI boundary to validate DATABASE_URL with the shared Zod
schema before calling liveColumns, preserving the existing missing-input exit
behavior. At packages/db/src/apply-catchup.ts:13-38, validate both the database
URL and catch-up script identifier with the shared schemas, then resolve the
validated identifier only to approved internal packages/db/catchup files before
readFile and sql.unsafe; do not accept arbitrary readable paths.
- Around line 26-60: Extend expectedTables and driftBetween to compare complete
schema metadata, not only table and column names. Include constraints, indexes,
defaults, generated expressions, and required enum values from the declarations
in content.ts, and obtain the corresponding live metadata for comparison. Report
missing or mismatched objects so catch-up migrations such as doc_access_request
and doc.search_vector are fully validated.
In `@packages/mcp-server/src/tools/docs.ts`:
- Around line 27-29: Move the shared MCP reference schemas represented by docRef
and collectionRef into `@orbit/shared`, export them there, and update the new tool
parameters in the server package to import and reuse those schemas instead of
defining local z.string validations. Preserve their current non-empty validation
and descriptions.
- Around line 79-84: Update packages/mcp-server/src/tools/docs.ts lines 79-84 in
resolveDoc to use an unbounded, lifecycle-aware exact resolver that matches
readable document IDs or titles; support live-only resolution for placement
operations and explicit archived resolution only for lifecycle operations.
Update packages/mcp-server/src/tools/docs.ts lines 143-150 to query destination
siblings directly in sort order rather than filtering a globally limited list,
ensuring archived documents are not used as parents or ordering neighbours.
- Around line 107-114: Update neighbourOf to reject calls where both after and
before are defined by raising the existing validationFailed error; preserve
resolving a single anchor and returning null when neither is provided.
In `@packages/mcp-server/tests/tools/docs.test.ts`:
- Around line 30-50: Update collectionId, makeDoc, listCollections, and listDocs
to validate TestClient.result payloads with the appropriate Zod response schemas
from `@orbit/shared` before accessing or returning their properties. Replace the
direct type assertions with schema parsing for each MCP tool response,
preserving the existing returned IDs, documents, and collections.
- Around line 188-194: Extend CollectionShape and the “renames and re-icons a
collection” test to include the icon field, then assert the listed collection’s
icon is “rocket” alongside the existing name assertion. Ensure the test fails if
the icon update is not persisted.
---
Outside diff comments:
In `@apps/web/src/features/docs/editor/editor-toolbar.tsx`:
- Around line 152-205: Update the editor toolbar rendering so every rich-text
formatting control, including bold, italic, lists, links, and related controls,
is gated by !collapsed just like the existing undo, redo, and text-style block.
Keep the mode switch and upload status rendered when collapsed, and preserve the
current compact behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88143f6e-ed2a-42eb-abf3-87ed1b2a8f89
📒 Files selected for processing (22)
apps/web/src/features/docs/doc-editor.tsxapps/web/src/features/docs/doc-header.tsxapps/web/src/features/docs/doc-reader.tsxapps/web/src/features/docs/doc-surface.tsxapps/web/src/features/docs/editor/editor-toolbar.tsxapps/web/src/features/docs/editor/rich-text-editor.tsxapps/web/src/features/docs/use-doc-preferences.tsapps/web/src/features/docs/use-doc-uploads.tsapps/web/tests/features/docs/doc-anchor-editor.test.tsxapps/web/tests/features/docs/doc-preferences.test.tsapps/web/tests/features/docs/doc-surface-anchors.test.tsxpackage.jsonpackages/core/src/content/doc-service.tspackages/db/catchup/doc-tree-schema-catchup.sqlpackages/db/package.jsonpackages/db/src/apply-catchup.tspackages/db/src/check-drift.tspackages/mcp-server/src/resolve.tspackages/mcp-server/src/tools/docs.tspackages/mcp-server/src/tools/support.tspackages/mcp-server/tests/tools/docs.test.tspackages/shared/src/validators/doc.ts
The outline was only rendered beside the rich editor, so defaulting to markdown took the table of contents off every document, which the end to end suite caught. The outline is computed from the markdown either way, so render it in both modes. In markdown it navigates by scrolling the source to the heading's own line, through headingLineNumbers, which skips a hash inside a fenced code block and counts a setext heading on the line its text sits on, so the indexes line up one for one with the rendered outline. Active heading tracking stays a rich mode affordance: it reads the position of rendered headings, and markdown mode has none. The spec now covers both modes, since the default it asserted changed.
…ce the outline is on Resolving a doc ref and finding a destination's siblings both read listDocs, which stops at DOC_LIST_LIMIT. Past that window a document could not be named and a neighbour could be missing from the anchors, which would have surfaced as a move refused for no visible reason. findDocsByRef and listDocSiblings ask for exactly what is wanted, with no limit. Only lifecycle tools fall back to the archived ones, so an archived doc can no longer be a parent or an ordering neighbour. move_doc refuses a call naming both the sibling to land after and the one to land before, rather than honouring one and dropping the other. The outline held its scroll container in a ref, so it measured whatever was mounted on its first run and never noticed a replacement: switching editor mode swapped the container and active heading tracking stopped. It takes the element itself now, so React re-runs the measurement when the surface changes. Also: the mode switch uses the shared tab interaction token rather than hand rolled hover classes, apply-catchup resolves only files inside packages/db/catchup, check-drift says out loud that it compares names and not constraints, indexes or generated expressions, and the preference store has tests for persistence and cross tab updates.
CodeQL reads StorageEvent through an extern that predates the init dictionary, so passing one is reported as a superfluous argument. A storage event with no key is what a browser sends when storage was cleared wholesale, and the store treats it the same way: re-read everything. Same path, no alert.
…nnot read CodeQL bundles an extern where StorageEvent takes no arguments at all, from before it was constructible, so every form of new StorageEvent is reported. The comment policy allows no suppression directive for it. Dispatch a plain Event carrying a null key instead, which is the shape the listener actually reads and what a browser sends when storage was cleared wholesale.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/web/src/features/docs/outline.ts`:
- Around line 144-152: Update the fence-closing logic around FENCE and marker in
apps/web/src/features/docs/outline.ts lines 144-152 to close only when the
candidate line contains the matching fence marker followed solely by permitted
trailing whitespace; marker text such as “not-a-closer” must remain code
content. Add a regression case in apps/web/tests/features/docs/outline.test.ts
lines 49-57 proving headings stay hidden after an invalid closer and become
visible only after a valid closer.
In `@apps/web/tests/features/docs/use-editor-outline.test.tsx`:
- Around line 127-134: Update the Swapping test around useEditorOutline to
verify the replacement surface is actually observed: render headings with
distinct offsets in both rich and markdown surfaces, dispatch a scroll event on
the markdown replacement container after rerendering, and assert that activeId
reflects the markdown surface’s heading. Replace the measuring-only assertion
with this behavior-focused check while preserving the existing surface
replacement assertions.
In `@packages/db/src/apply-catchup.ts`:
- Around line 2-14: Update the CATCHUP_DIR initialization used by catchupPath to
convert the directory URL with fileURLToPath(new URL('../catchup/',
import.meta.url)) instead of reading URL.pathname, and import fileURLToPath from
node:url. Preserve the existing path validation and resolution behavior.
In `@packages/db/tests/apply-catchup.test.ts`:
- Around line 4-37: Add execution-focused tests for applyCatchup alongside the
existing catchupPath and targetName tests. Cover successful SQL loading and
execution, rejected queries, and verify the PostgreSQL client’s sql.end cleanup
runs through the finally path; mock the client and assert
sql.unsafe(body).simple() is invoked with the loaded script.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ebe3049-ee46-44dc-b585-874790c6dade
📒 Files selected for processing (17)
apps/web/e2e/doc-outline.spec.tsapps/web/src/features/docs/doc-editor.tsxapps/web/src/features/docs/doc-surface.tsxapps/web/src/features/docs/editor/markdown-code-editor.tsxapps/web/src/features/docs/editor/rich-text-editor.tsxapps/web/src/features/docs/outline.tsapps/web/src/features/docs/use-editor-outline.tsapps/web/tests/features/docs/doc-preferences.test.tsapps/web/tests/features/docs/long-document-cost.test.tsxapps/web/tests/features/docs/outline.test.tsapps/web/tests/features/docs/use-editor-outline.test.tsxpackages/core/src/content/doc-service.tspackages/db/src/apply-catchup.tspackages/db/src/check-drift.tspackages/db/tests/apply-catchup.test.tspackages/mcp-server/src/tools/docs.tspackages/mcp-server/tests/tools/docs.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/features/docs/editor/rich-text-editor.tsx
- apps/web/src/features/docs/doc-surface.tsx
- packages/db/src/check-drift.ts
- packages/mcp-server/src/tools/docs.ts
- apps/web/src/features/docs/doc-editor.tsx
headingLineNumbers closed a fenced block on any line starting with a matching marker, so a line like ```not-a-closer inside a block ended it, counted the headings that followed as real, and drifted the indexes away from the rendered outline the list is displayed from. A closing fence carries no info string: require the rest of the line to be blank. The surface swap test asserted only that React handed over the new element, so it passed whether or not the outline measured it. It now places headings on both surfaces, scrolls the replacement, and asserts the active heading comes from it. applyCatchup had no test that ran it. It now applies the doc tree script to a scratch database twice, checks sort_order, slug and a generated search_vector arrive and survive the second run, indexes a doc body through that vector, and refuses a path outside the catchup directory.
Why
/docswas answering 500 in production. The cause was schema drift, not code: the deployed build queriesdoc.sort_order, and the production database does not have that column.The docs tree change (#165) added
doc.sort_order,doc.search_vector,doc_collection.positionanddoc_access_request, and shippeddoc-tree-order-catchup.sqlto seed the new order. That script only backfills values, it assumes the columns exist. The DDL half was never written, so it was never applied. Every other schema commit in the last three weeks has a matching catchup script; this one was the gap.It was not only the docs page. In three hours of production logs: 29 failures on
GET /api/sync, so realtime catch-up was down for the whole workspace.Live drift, read from production:
What this changes
The outage.
packages/db/catchup/doc-tree-schema-catchup.sqladds the missing DDL, idempotent and in one transaction like the other catchup scripts. Verified against a database rebuilt from baseline0000: reproduced the failure, applied the script, both failing production queries then succeeded, and a second run was clean.So it cannot happen quietly again.
bun run db:check-driftcompares every table the Drizzle schema declares againstinformation_schemaand exits non zero when the database is behind, so CI can refuse to ship code the target database cannot answer. It also lists tables the schema no longer declares, because those are whatdb:pushwould drop, and push is not safe here: it runsdrizzle-kit push --force, and the fourstandup*tables are no longer in the schema but still hold 31 rows in production.bun run db:catchup <file>applies one catchup file over the configured connection.Filing a document from MCP. Twelve doc tools existed and none could put a document into a collection. Passing
collectionwas accepted and dropped, which is how 148 documents were imported while every folder stayed empty, with no error anywhere.create_docandupdate_doctakecollection;update_doctakesparentlist_docsfilters bycollectionand byunfiledlist_doc_collectionscarriesdocCountandpositionupdate_doc_collection,delete_doc_collection(withreassignTo),move_doc,unarchive_doc,delete_docA collection resolves by id, then by name, and an ambiguous name is a clear error rather than a guess.
One home per document. Orbit's rule is that a doc lives in a folder or in a project, never both, and the sidebar groups purely on
projectId. Moving already refused the combination; creating and updating quietly allowed it. That is why filing alone would not have been enough: the imported documents carry a project, so they would have stayed under "Project docs" with the folders still empty. Filing a document now detaches it from its project, attaching it to a project unfiles it, and naming both in one call is refused. Production currently holds 7 documents in that invalid state.Unknown arguments error.
defineToolbuilds a strict object, so an unrecognised argument is a validation error on all 71 tools rather than a silent no-op. This is the bug that made the migration fail silently.The docs UI. A doc opened in rich text with a full formatting bar pinned above it, under a header, under a row of pills: four bands of chrome between the breadcrumb and the first line, with the prose held to 45rem however wide the window was.
Mode, toolbar and width stick per person in local storage.
Large documents. A version snapshot compared the previous body in the app, so every autosave of a large document dragged the last saved copy out of Postgres just to decide whether anything had changed. It compares in SQL now. The limit is unchanged at 500,000 characters; the generated search vector is bounded at 200,000 and measures 165KB against Postgres's 1MB cap even for pathological content, so it was never the constraint.
Verification
bun run lint,check-comments,check-bytes,check-bun-imports,check-deps,typecheck: cleanpackages/mcp-server: 160 pass, including the GitHub tools merged frommainunder the new strict argumentsapps/web: 1585 passpackages/core: 725 pass on a clean lane (two timed out on a shared lane under contention with another agent's run, and pass in isolation)Applying this
The catchup must be applied before this deploys, or the drift check will fail and
/docsstays down:Deliberately not here
descriptionon a collection, which the spec asks for: it needs another schema change, and schema changes shipping ahead of the database is exactly what broke production. Worth doing once the drift check is in CI.reorder_docs;move_docwithafter/beforeis already the reorder primitive.Review
Greptile, ordering uses unresolved destination home (P1) — real, and fixed in 6751f05. A move naming both a parent and a neighbour looked for that neighbour in the collection the arguments asked for, but a nested doc inherits its parent's home, so the sibling set was wrong and a valid move was refused.
plannedPlacementnow asks core where the doc will land through the same resolution the move runs. Regression test added; verified it fails without the fix.Greptile, catchup violates comment policy (P1) — disagreed, with a reply on the thread.
check-commentsinspects source, not SQL, the CI check passes with the comments present, and every existing script inpackages/db/catchupcarries the same header.Two issues found in my own review before the bots:
list_docsclaimed an ordering it did not use, andcollectionplusunfiledin one call answered with nothing instead of saying why.Greptile Summary
The PR restores the missing document-tree database schema, adds drift and catchup tooling, expands MCP document filing and collection operations, and revises the docs editing and reading experience.
Confidence Score: 3/5
The PR is not yet safe to merge because document ordering can still be silently misapplied during concurrent moves and the catchup script remains out of compliance with the repository’s mounted source-comment rule.
The parent-home resolution fix addresses the original ordering mismatch, but anchor IDs are still computed before the mutation transaction and can disappear from the re-resolved sibling scope, causing the requested position to fall back to the end of the list. The new catchup SQL also retains comments prohibited by the applicable repository guide.
Files Needing Attention: packages/mcp-server/src/tools/docs.ts, packages/core/src/content/doc-service.ts, packages/db/catchup/doc-tree-schema-catchup.sql
Important Files Changed
Sequence Diagram
Reviews (11): Last reviewed commit: "fix(docs): a fence closes only on a line..." | Re-trigger Greptile