Add exact-version document review workflow - #169
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesDocument-review contracts, access enforcement, server workflows, HTTP/MCP transports, task-page panels, threaded discussions, and visual-review coverage tooling are added or updated. Document APIs now use task-scoped authentication and client access boundaries. Document review workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant TaskPage
participant ReviewHTTP
participant ReviewServer
participant Reviewer
Manager->>TaskPage: open document review task
TaskPage->>ReviewServer: load review panel data
Manager->>ReviewHTTP: request review or apply proposal
ReviewHTTP->>ReviewServer: execute authenticated operation
Reviewer->>ReviewHTTP: submit verdict
ReviewHTTP->>ReviewServer: persist review response
Manager->>ReviewHTTP: decide document revision
ReviewHTTP->>ReviewServer: persist internal decision
Possibly related PRs
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ba42e5de2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PR review packetStart here
Review checklist
Changed files considered
Updated automatically when this PR's preview or visual review reruns. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
packages/web/src/components/tasks/document-review-reviewer-panel.test.tsx (1)
115-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a second submit with a different verdict.
This guards the idempotency-key reuse flagged in
document-review-reviewer-panel.tsx: after one submit, changing the feedback/verdict and submitting again should send a differentIdempotency-Key. Today nothing catches the silent dedupe.🤖 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/web/src/components/tasks/document-review-reviewer-panel.test.tsx` around lines 115 - 138, Add a second submission scenario to the test around the existing “submits change feedback and the verdict in one request” case: after the first request, change the feedback and select a different verdict, submit again, then assert two requests were made and their Idempotency-Key headers differ while verifying the second payload reflects the updated values.packages/web/src/components/tasks/document-review-manager-panel.test.tsx (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth new panel tests depend on the deprecated
react-dom/test-utilsSimulateAPI. It still works on react-dom 18.3.1 but is removed in React 19, so both suites break on upgrade.
packages/web/src/components/tasks/document-review-manager-panel.test.tsx#L6-L6: replaceSimulate.clickwith@testing-library/react(if available) ordispatchEvent(new MouseEvent("click", { bubbles: true }))insideact.packages/web/src/components/tasks/document-review-reviewer-panel.test.tsx#L6-L6: replaceSimulate.change/Simulate.clickthe same way, setting the textarea value before dispatchinginput/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/web/src/components/tasks/document-review-manager-panel.test.tsx` at line 6, Replace the deprecated Simulate API in both test suites: in packages/web/src/components/tasks/document-review-manager-panel.test.tsx at line 6, replace Simulate.click with the available testing-library interaction or a bubbled MouseEvent dispatched inside act; in packages/web/src/components/tasks/document-review-reviewer-panel.test.tsx at line 6, replace Simulate.change and Simulate.click similarly, setting the textarea value before dispatching the input/change event.packages/web/src/components/tasks/document-review-manager-panel.tsx (1)
395-436: 📐 Maintainability & Code Quality | 🔵 TrivialHand-rolled reviewer autocomplete: prefer a RetroUI primitive and add keyboard/ARIA support.
The dropdown is mouse-only: no
role="combobox"/listboxwiring, noaria-expanded/aria-activedescendant, no arrow-key or Escape handling, and no focus management, so keyboard and screen-reader users can't select a reviewer. Reuse the existing primitive (menu/combobox) fromsrc/components/retroui/rather than the raw input + absolutely positioned buttons.As per coding guidelines: "Use RetroUI primitives from
src/components/retroui/for buttons, inputs, checkboxes, dialogs, tables, menus, accordions, and other standard controls before hand-rolling custom components" and "Cropping, dragging, zooming, sliders, dialogs, tables, and other tricky interactions should rely on established primitives before hand-rolled behavior".#!/bin/bash # Find available RetroUI primitives suitable for a searchable select/menu. fd . -t f --full-path 'packages/web/src/components/retroui' rg -n 'export (function|const)' -g 'packages/web/src/components/retroui/**' | head -50🤖 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/web/src/components/tasks/document-review-manager-panel.tsx` around lines 395 - 436, Replace the hand-rolled reviewer autocomplete in the reviewer field with the appropriate searchable select/combobox or menu primitive from src/components/retroui/. Preserve personQuery, selectedPerson, people, and searching behavior while delegating keyboard navigation, selection, Escape handling, focus management, and ARIA combobox/listbox wiring to the primitive; remove the raw input and absolutely positioned button list.Source: Coding guidelines
packages/web/src/app/tasks/[id]/page.tsx (1)
647-656: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: collapse the duplicated narrowing into one derived value.
showDocumentReviewManageralready encodesmode === "MANAGER" && setup != null; the repeated checks here exist only for TS narrowing. A single narrowed object keeps the JSX condition to one expression.♻️ Suggested refactor
+ const managerView = + documentReviewPageData?.panel.mode === "MANAGER" && + documentReviewPageData.setup != null && + (documentReviewPageData.setup.documents.length > 0 || + documentReviewPageData.panel.proposals.length > 0 || + documentReviewPageData.panel.reviews.length > 0) + ? { + panel: documentReviewPageData.panel, + setup: documentReviewPageData.setup, + } + : null;- {showDocumentReviewManager && - documentReviewPageData?.panel.mode === "MANAGER" && - documentReviewPageData.setup ? ( - <DocumentReviewManagerPanel - panel={documentReviewPageData.panel} - setup={documentReviewPageData.setup} - /> - ) : documentReviewPageData?.panel.mode === "REVIEWER" ? ( + {managerView ? ( + <DocumentReviewManagerPanel + panel={managerView.panel} + setup={managerView.setup} + /> + ) : documentReviewPageData?.panel.mode === "REVIEWER" ? (🤖 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/web/src/app/tasks/`[id]/page.tsx around lines 647 - 656, Refactor the JSX around DocumentReviewManagerPanel and DocumentReviewReviewerPanel to derive one narrowed document-review value from showDocumentReviewManager and documentReviewPageData, rather than repeating the MANAGER mode and setup checks. Use that derived value for the manager rendering while preserving the existing reviewer branch and null fallback.packages/web/src/lib/tasks/document-review-binding.server.ts (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
import typefor these@optimitron/dbenums.
TaskApplicationPolicy,TaskClaimPolicy, andTaskExecutionModeare only used as type annotations inDocumentReviewBindingTask(Lines 13, 15, 17) — no member access occurs in this file.♻️ Proposed fix
-import { +import type { TaskApplicationPolicy, TaskClaimPolicy, TaskExecutionMode, } from "`@optimitron/db`";🤖 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/web/src/lib/tasks/document-review-binding.server.ts` around lines 1 - 5, Change the `@optimitron/db` import of TaskApplicationPolicy, TaskClaimPolicy, and TaskExecutionMode to a type-only import, preserving their use as type annotations in DocumentReviewBindingTask.Source: Coding guidelines
packages/web/src/app/api/documents/route.ts (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMalformed JSON bodies surface as 500 here.
await request.json()is unguarded, so a bad body lands in the catch block and returnsINTERNAL_ERROR/500 rather than a client error. The sibling handler inpackages/web/src/app/api/documents/[id]/route.ts(line 58) already guards with.catch(() => null); matching it would keep the two document endpoints consistent.♻️ Suggested change
- const body = (await request.json()) as Record<string, unknown>; + const body = ((await request.json().catch(() => null)) ?? + {}) as Record<string, unknown>;🤖 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/web/src/app/api/documents/route.ts` around lines 52 - 56, Update the request-body parsing in the document route before the idempotency-key lookup to catch JSON parse failures and use a null/empty fallback, matching the guarded parsing in the sibling [id] route. Ensure malformed JSON follows the existing client-error validation path instead of reaching the catch block as an internal server error.packages/web/src/lib/mcp-tools/documents.ts (1)
205-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVisibility is compared against string literals in both document tool handlers. The shared cause is that
ContentVisibilityis not imported here, unlikepackages/web/src/app/api/documents/route.ts(lines 67-70) which uses the enum for the same field.
packages/web/src/lib/mcp-tools/documents.ts#L205-L208: compareargs.visibilityagainstContentVisibility.PUBLIC/ContentVisibility.PRIVATEin thecreateDocumentbranch.packages/web/src/lib/mcp-tools/documents.ts#L237-L240: apply the same enum comparison in theupdateDocumentbranch.As per coding guidelines: "Use enums instead of magic strings in TypeScript code".
🤖 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/web/src/lib/mcp-tools/documents.ts` around lines 205 - 208, Import and use ContentVisibility in both the createDocument branch at packages/web/src/lib/mcp-tools/documents.ts lines 205-208 and the updateDocument branch at lines 237-240, replacing the PUBLIC and PRIVATE string comparisons with ContentVisibility.PUBLIC and ContentVisibility.PRIVATE while preserving the null fallback.Source: Coding guidelines
packages/web/src/lib/__tests__/task-visibility.server.test.ts (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"optimitron.review-request.v1"is re-typed in two test files instead of imported. The literal is owned byReviewRequestV1Schemainpackages/web/src/lib/tasks/document-review-contracts.ts; duplicating it means a contract rename leaves both tests green while the predicate and the forged-fixture shape drift. Export it as a named constant and import it, as both files already do forDOCUMENT_REVIEW_TASK_KEY_PREFIX.
packages/web/src/lib/__tests__/task-visibility.server.test.ts#L8-L8: delete the localDOCUMENT_REVIEW_REQUEST_SCHEMAconst and import the exported constant from@/lib/tasks/document-review-contracts.packages/web/src/lib/tasks/document-review.integration.test.ts#L361-L382: replace the inlineschema: "optimitron.review-request.v1"in the forged-reviewcontextJsonwith the imported constant.🤖 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/web/src/lib/__tests__/task-visibility.server.test.ts` at line 8, Export the schema identifier owned by ReviewRequestV1Schema in packages/web/src/lib/tasks/document-review-contracts.ts, then import and reuse it in packages/web/src/lib/__tests__/task-visibility.server.test.ts at lines 8-8 instead of the local constant, and in packages/web/src/lib/tasks/document-review.integration.test.ts at lines 361-382 instead of the inline forged-review contextJson literal.packages/web/src/lib/mcp-tools/document-reviews.ts (1)
212-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the structured error code so agents can tell a conflict from a validation failure.
DocumentReviewErrorcarriescode(DOCUMENT_REVIEW_CONFLICT/_NOT_FOUND/_INVALID), but everything collapses to a bare message here. An MCP client retrying a409idempotency conflict versus fixing invalid input needs that distinction — the HTTP transport already exposes it.♻️ Suggested change
} catch (error) { - return err( - error instanceof Error - ? error.message - : "Document review operation failed", - ); + const code = + error && typeof (error as { code?: unknown }).code === "string" + ? (error as { code: string }).code + : undefined; + return err( + [ + error instanceof Error + ? error.message + : "Document review operation failed", + code ? `(${code})` : null, + ] + .filter(Boolean) + .join(" "), + ); }A dedicated
err(message, code)overload returning{ code, error }in the JSON body would be cleaner if you prefer machine-readable output.🤖 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/web/src/lib/mcp-tools/document-reviews.ts` around lines 212 - 218, Update the catch handling in the document review operation to preserve DocumentReviewError.code when constructing the MCP error response, returning the structured code alongside the message via the supported err(message, code) shape. Keep the existing fallback message and behavior for non-DocumentReviewError exceptions.packages/web/src/lib/tasks/document-review.server.ts (2)
2030-2052: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPanel aggregation runs unbounded sequential per-artifact work inside an open transaction.
decisionArtifacts,applicationArtifacts, andproposalArtifactsare unbounded (notake), and each proposal preview issues two more revision reads sequentially. On a task with a long decision/proposal history this holds a DB transaction open across a growing number of round-trips on the task-page render path. Consider bounding the artifact queries with atakeand parallelizing (Promise.all) the per-artifact authentication/preview work, mirroring thereviewspath above.🤖 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/web/src/lib/tasks/document-review.server.ts` around lines 2030 - 2052, Bound the decisionArtifacts, applicationArtifacts, and proposalArtifacts queries with an appropriate take limit, then update the aggregation loops to run per-artifact readAuthenticDecisionArtifact, readAuthenticApplicationArtifact, and buildProposalPreview work via Promise.all while preserving filtering and result ordering/semantics. Mirror the bounded, parallel pattern used by the nearby reviews path.
603-628: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIdempotency lookup loads every attempt of this kind for the task, then filters in memory.
idempotencyKeyis already stored in the samemetadataJSON, so it can be matched in the query and keep the scan bounded as proposals/decisions accumulate on a long-lived authority task.♻️ Suggested narrowing
const attempts = await tx.taskExecutionAttempt.findMany({ where: { deletedAt: null, - metadata: { - equals: input.kind, - path: ["kind"], - }, + AND: [ + { metadata: { equals: input.kind, path: ["kind"] } }, + { metadata: { equals: input.idempotencyKey, path: ["idempotencyKey"] } }, + ], taskId: input.taskId, },The in-memory
requestHashmismatch check stays as-is.🤖 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/web/src/lib/tasks/document-review.server.ts` around lines 603 - 628, Update the taskExecutionAttempt.findMany query in the idempotency lookup to include the idempotencyKey condition in the metadata JSON filter, using input.idempotencyKey, so only matching attempts are fetched. Remove the subsequent in-memory matching filter while preserving the existing requestHash mismatch check unchanged.packages/web/src/app/api/tasks/[id]/document-reviews/http.ts (1)
47-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMap
DocumentReviewErrorand unsupported auth responses explicitly.
documentReviewErrorResponsestill relies on the current"Unauthorized"literal and any object with a numericstatusplus a string message, and it missesResponseauth failures fromrequireTaskRequestAuth. Prefererror instanceof DocumentReviewError, add the missingimportif needed, and handle unsupported auth responses instead of reflecting arbitrary error shapes.🤖 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/web/src/app/api/tasks/`[id]/document-reviews/http.ts around lines 47 - 83, Update documentReviewErrorResponse to use instanceof DocumentReviewError for document-review error mapping, importing the class if necessary, and remove the arbitrary status/message shape check. Replace the "Unauthorized" literal check with explicit handling for Response-based auth failures returned by requireTaskRequestAuth, mapping supported auth statuses to the standardized authentication response without reflecting arbitrary error objects; preserve the existing ZodError mapping.
🤖 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/web/src/components/tasks/document-review-reviewer-panel.tsx`:
- Line 51: Update the idempotency state in the reviewer panel from a standalone
key to a { fingerprint, key } record, matching the manager panel pattern. In
submit(), compute the normalized review-body fingerprint from verdict and
explanation, reuse the key only when the fingerprint matches, and generate and
store a new key whenever the payload changes.
In `@packages/web/src/components/tasks/task-comment-feed.test.tsx`:
- Around line 49-51: Remove the globalThis.React assignment from the
TaskCommentFeed test setup. Keep JSX and renderToStaticMarkup runtime
requirements isolated through the test/compiler configuration rather than
process-wide mutable state, while preserving the existing test behavior.
In `@packages/web/src/lib/__tests__/task-visibility.server.test.ts`:
- Around line 202-207: Add an explicit non-empty assertion for
collectMembershipRoleFilters(readable) before the existing role-value assertion
loop, ensuring the test fails when no membership role filters are collected
while preserving the OWNER/ADMIN validation.
In `@packages/web/src/lib/documents.server.ts`:
- Around line 855-863: Replace the awaited per-row
isDocumentWithinClientAccessBoundary call in the document-list loop with batched
boundary resolution: when taskId is provided, reuse the task boundary already
validated earlier; otherwise collect distinct row task IDs, load their
public/owner fields with one task.findMany, and pass each resolved task to
isTaskWithinClientAccessBoundary while preserving rows without task links and
existing filtering behavior.
In `@packages/web/src/lib/tasks/task-comments.server.ts`:
- Around line 103-172: Update the review-comment creation flow in
submitDocumentReview to pass visibility: TaskCommentVisibility.INTERNAL when
calling createTopLevelTaskCommentInTransaction, ensuring reviewer explanations
remain private while leaving the helper’s default behavior unchanged for other
callers.
---
Nitpick comments:
In `@packages/web/src/app/api/documents/route.ts`:
- Around line 52-56: Update the request-body parsing in the document route
before the idempotency-key lookup to catch JSON parse failures and use a
null/empty fallback, matching the guarded parsing in the sibling [id] route.
Ensure malformed JSON follows the existing client-error validation path instead
of reaching the catch block as an internal server error.
In `@packages/web/src/app/api/tasks/`[id]/document-reviews/http.ts:
- Around line 47-83: Update documentReviewErrorResponse to use instanceof
DocumentReviewError for document-review error mapping, importing the class if
necessary, and remove the arbitrary status/message shape check. Replace the
"Unauthorized" literal check with explicit handling for Response-based auth
failures returned by requireTaskRequestAuth, mapping supported auth statuses to
the standardized authentication response without reflecting arbitrary error
objects; preserve the existing ZodError mapping.
In `@packages/web/src/app/tasks/`[id]/page.tsx:
- Around line 647-656: Refactor the JSX around DocumentReviewManagerPanel and
DocumentReviewReviewerPanel to derive one narrowed document-review value from
showDocumentReviewManager and documentReviewPageData, rather than repeating the
MANAGER mode and setup checks. Use that derived value for the manager rendering
while preserving the existing reviewer branch and null fallback.
In `@packages/web/src/components/tasks/document-review-manager-panel.test.tsx`:
- Line 6: Replace the deprecated Simulate API in both test suites: in
packages/web/src/components/tasks/document-review-manager-panel.test.tsx at line
6, replace Simulate.click with the available testing-library interaction or a
bubbled MouseEvent dispatched inside act; in
packages/web/src/components/tasks/document-review-reviewer-panel.test.tsx at
line 6, replace Simulate.change and Simulate.click similarly, setting the
textarea value before dispatching the input/change event.
In `@packages/web/src/components/tasks/document-review-manager-panel.tsx`:
- Around line 395-436: Replace the hand-rolled reviewer autocomplete in the
reviewer field with the appropriate searchable select/combobox or menu primitive
from src/components/retroui/. Preserve personQuery, selectedPerson, people, and
searching behavior while delegating keyboard navigation, selection, Escape
handling, focus management, and ARIA combobox/listbox wiring to the primitive;
remove the raw input and absolutely positioned button list.
In `@packages/web/src/components/tasks/document-review-reviewer-panel.test.tsx`:
- Around line 115-138: Add a second submission scenario to the test around the
existing “submits change feedback and the verdict in one request” case: after
the first request, change the feedback and select a different verdict, submit
again, then assert two requests were made and their Idempotency-Key headers
differ while verifying the second payload reflects the updated values.
In `@packages/web/src/lib/__tests__/task-visibility.server.test.ts`:
- Line 8: Export the schema identifier owned by ReviewRequestV1Schema in
packages/web/src/lib/tasks/document-review-contracts.ts, then import and reuse
it in packages/web/src/lib/__tests__/task-visibility.server.test.ts at lines 8-8
instead of the local constant, and in
packages/web/src/lib/tasks/document-review.integration.test.ts at lines 361-382
instead of the inline forged-review contextJson literal.
In `@packages/web/src/lib/mcp-tools/document-reviews.ts`:
- Around line 212-218: Update the catch handling in the document review
operation to preserve DocumentReviewError.code when constructing the MCP error
response, returning the structured code alongside the message via the supported
err(message, code) shape. Keep the existing fallback message and behavior for
non-DocumentReviewError exceptions.
In `@packages/web/src/lib/mcp-tools/documents.ts`:
- Around line 205-208: Import and use ContentVisibility in both the
createDocument branch at packages/web/src/lib/mcp-tools/documents.ts lines
205-208 and the updateDocument branch at lines 237-240, replacing the PUBLIC and
PRIVATE string comparisons with ContentVisibility.PUBLIC and
ContentVisibility.PRIVATE while preserving the null fallback.
In `@packages/web/src/lib/tasks/document-review-binding.server.ts`:
- Around line 1-5: Change the `@optimitron/db` import of TaskApplicationPolicy,
TaskClaimPolicy, and TaskExecutionMode to a type-only import, preserving their
use as type annotations in DocumentReviewBindingTask.
In `@packages/web/src/lib/tasks/document-review.server.ts`:
- Around line 2030-2052: Bound the decisionArtifacts, applicationArtifacts, and
proposalArtifacts queries with an appropriate take limit, then update the
aggregation loops to run per-artifact readAuthenticDecisionArtifact,
readAuthenticApplicationArtifact, and buildProposalPreview work via Promise.all
while preserving filtering and result ordering/semantics. Mirror the bounded,
parallel pattern used by the nearby reviews path.
- Around line 603-628: Update the taskExecutionAttempt.findMany query in the
idempotency lookup to include the idempotencyKey condition in the metadata JSON
filter, using input.idempotencyKey, so only matching attempts are fetched.
Remove the subsequent in-memory matching filter while preserving the existing
requestHash mismatch check 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c797204-5ff1-4654-8513-242217bd6fd5
📒 Files selected for processing (36)
packages/web/src/app/api/documents/[id]/route.test.tspackages/web/src/app/api/documents/[id]/route.tspackages/web/src/app/api/documents/route.test.tspackages/web/src/app/api/documents/route.tspackages/web/src/app/api/tasks/[id]/document-reviews/decision/route.tspackages/web/src/app/api/tasks/[id]/document-reviews/http.tspackages/web/src/app/api/tasks/[id]/document-reviews/proposal/route.tspackages/web/src/app/api/tasks/[id]/document-reviews/response/route.tspackages/web/src/app/api/tasks/[id]/document-reviews/route.tspackages/web/src/app/tasks/[id]/page.tsxpackages/web/src/components/tasks/document-review-manager-panel.test.tsxpackages/web/src/components/tasks/document-review-manager-panel.tsxpackages/web/src/components/tasks/document-review-reviewer-panel.test.tsxpackages/web/src/components/tasks/document-review-reviewer-panel.tsxpackages/web/src/components/tasks/task-comment-feed.test.tsxpackages/web/src/components/tasks/task-comment-feed.tsxpackages/web/src/lib/__tests__/documents.server.test.tspackages/web/src/lib/__tests__/mcp-server.test.tspackages/web/src/lib/__tests__/mcp-tool-catalog.test.tspackages/web/src/lib/__tests__/task-visibility.server.test.tspackages/web/src/lib/api-routes.tspackages/web/src/lib/documents.server.tspackages/web/src/lib/mcp-server.tspackages/web/src/lib/mcp-tools/document-reviews.tspackages/web/src/lib/mcp-tools/documents.test.tspackages/web/src/lib/mcp-tools/documents.tspackages/web/src/lib/tasks/document-review-binding.server.test.tspackages/web/src/lib/tasks/document-review-binding.server.tspackages/web/src/lib/tasks/document-review-contracts.test.tspackages/web/src/lib/tasks/document-review-contracts.tspackages/web/src/lib/tasks/document-review-invalidation.server.tspackages/web/src/lib/tasks/document-review-ui.server.tspackages/web/src/lib/tasks/document-review.integration.test.tspackages/web/src/lib/tasks/document-review.server.tspackages/web/src/lib/tasks/task-comments.server.tspackages/web/src/lib/tasks/task-visibility.server.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/web/src/lib/tasks/document-review.server.ts (2)
184-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcode-free schema discriminant would be safer here.
"optimitron.review-request.v1"is duplicated from theReviewRequestV1Schemaliteral indocument-review-contracts.ts; if the schema version bumps, this boundary predicate silently stops matching personal-private review tasks (reviewers lose access rather than erroring). Export the schema id constant from the contracts module and reference it.As per coding guidelines, "Use enums instead of magic strings in TypeScript code".
🤖 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/web/src/lib/tasks/document-review.server.ts` around lines 184 - 194, Replace the hardcoded schema discriminant in the personal-private predicate within the boundary logic with the exported schema ID constant from ReviewRequestV1Schema’s contracts module. Export that constant from document-review-contracts.ts and import/reuse it here so version changes update both the schema and access predicate consistently.Source: Coding guidelines
852-899: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMissing
deletedAt: nullfilters make soft-deleted governance artifacts count as proposal authorship.Both
findManycalls here read artifacts/attempts without excluding soft-deleted rows, unlike the equivalent decision-history query indecideDocumentRevision(Line 2164-2177), which filtersdeletedAt: nullon both the artifact and its attempt. A soft-deleted proposal/application pair would still bar a reviewer atrequestDocumentReview(Line 1746) and invalidate provenance inhasValidReviewTaskProvenance. Fail-closed, so not exploitable, but the inconsistency is surprising and hard to reason about later.♻️ Align soft-delete filtering with the decision-history query
const applicationArtifacts = await tx.taskExecutionArtifact.findMany({ where: { + deletedAt: null, structuredResultJson: { equals: target.revisionId, path: ["resultingDocument", "revisionId"], }, taskExecutionAttempt: { + deletedAt: null, metadata: { equals: DOCUMENT_PROPOSAL_APPLICATION_ARTIFACT_KIND, path: ["kind"], }, taskId: authorityTaskId, }, }, select: artifactWithAttemptSelect, }); @@ const proposalArtifacts = await tx.taskExecutionArtifact.findMany({ where: { + deletedAt: null, id: { in: applications.map( ({ application }) => application.proposalArtifact.artifactId, ), }, }, select: artifactWithAttemptSelect, });🤖 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/web/src/lib/tasks/document-review.server.ts` around lines 852 - 899, Add deletedAt: null constraints to both artifact queries in the proposal-authorship lookup: filter the application artifact and its taskExecutionAttempt, and filter the proposal artifact and its attempt. Keep the existing matching and selection logic unchanged so soft-deleted governance artifacts are excluded consistently with decideDocumentRevision.packages/web/src/app/api/tasks/[id]/document-reviews/route.ts (1)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the
[id]contract fordocument-reviews.
GET /document-reviewstreats[id]asreviewTaskIdfor an assigned review, whilePOST /document-reviewsandAPI_ROUTES.tasks.documentReviews()treat it asauthorityTaskIdand POSTs to/proposal. Add a route/MCP comment or split the endpoint to avoid clients passing review-task IDs to create flows and authority-task IDs to load an assigned review.🤖 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/web/src/app/api/tasks/`[id]/document-reviews/route.ts around lines 9 - 19, Clarify the conflicting `[id]` semantics around the GET handler and related document-review routes: document that GET `context.params.id` is an assigned review task ID, while POST and `API_ROUTES.tasks.documentReviews()` use an authority task ID for proposal creation. Add an explicit route/MCP comment near the relevant handlers, or split the endpoints so clients cannot interchange these identifiers.
🤖 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/web/src/lib/tasks/document-review-invalidation.server.ts`:
- Around line 14-94: Update hasDocumentGovernanceHistory and
governanceArtifactReferenceWhere to exclude soft-deleted records by requiring
deletedAt: null on the review task, task execution artifact, and related
taskExecutionAttempt queries. Preserve the existing governance matching
conditions while ensuring soft-deleted history does not cause the relinking
guard to report prior governance history.
---
Nitpick comments:
In `@packages/web/src/app/api/tasks/`[id]/document-reviews/route.ts:
- Around line 9-19: Clarify the conflicting `[id]` semantics around the GET
handler and related document-review routes: document that GET
`context.params.id` is an assigned review task ID, while POST and
`API_ROUTES.tasks.documentReviews()` use an authority task ID for proposal
creation. Add an explicit route/MCP comment near the relevant handlers, or split
the endpoints so clients cannot interchange these identifiers.
In `@packages/web/src/lib/tasks/document-review.server.ts`:
- Around line 184-194: Replace the hardcoded schema discriminant in the
personal-private predicate within the boundary logic with the exported schema ID
constant from ReviewRequestV1Schema’s contracts module. Export that constant
from document-review-contracts.ts and import/reuse it here so version changes
update both the schema and access predicate consistently.
- Around line 852-899: Add deletedAt: null constraints to both artifact queries
in the proposal-authorship lookup: filter the application artifact and its
taskExecutionAttempt, and filter the proposal artifact and its attempt. Keep the
existing matching and selection logic unchanged so soft-deleted governance
artifacts are excluded consistently with decideDocumentRevision.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7c25d77-71bc-405e-8b62-0d7cf8794810
📒 Files selected for processing (22)
packages/web/src/app/api/people/search/route.test.tspackages/web/src/app/api/people/search/route.tspackages/web/src/app/api/tasks/[id]/document-reviews/http.test.tspackages/web/src/app/api/tasks/[id]/document-reviews/http.tspackages/web/src/app/api/tasks/[id]/document-reviews/route.tspackages/web/src/components/tasks/document-review-reviewer-panel.test.tsxpackages/web/src/components/tasks/document-review-reviewer-panel.tsxpackages/web/src/components/tasks/task-comment-feed.test.tsxpackages/web/src/components/tasks/task-comment-feed.tsxpackages/web/src/lib/__tests__/documents.server.test.tspackages/web/src/lib/__tests__/mcp-tool-catalog.test.tspackages/web/src/lib/__tests__/task-visibility.server.test.tspackages/web/src/lib/documents.server.tspackages/web/src/lib/mcp-tools/document-reviews.test.tspackages/web/src/lib/mcp-tools/document-reviews.tspackages/web/src/lib/tasks/document-review-contracts.test.tspackages/web/src/lib/tasks/document-review-contracts.tspackages/web/src/lib/tasks/document-review-invalidation.server.tspackages/web/src/lib/tasks/document-review.integration.test.tspackages/web/src/lib/tasks/document-review.server.tspackages/web/src/lib/tasks/task-merge.server.test.tspackages/web/src/lib/tasks/task-merge.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/components/tasks/task-comment-feed.tsx
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/web/e2e/utils/visual-routes.ts`:
- Around line 41-47: Update DOCUMENT_REVIEW_FIXTURE_MANIFEST_PATH to resolve
from the packages/web package root anchored to this file’s location, matching
the seeder and runner paths; do not base it on process.cwd(), so repo-root
Playwright invocations locate the existing manifest.
In `@packages/web/scripts/build-visual-review.mjs`:
- Around line 1046-1059: Update loadChangedFiles so JSON.parse(envJson) is
wrapped in error handling that converts malformed
VISUAL_REVIEW_CHANGED_FILES_JSON into the same actionable TypeError used for
invalid array contents, while preserving valid JSON parsing and existing shape
validation.
In `@packages/web/scripts/run-playwright.mjs`:
- Line 21: Update the requestedMode === "visual" branch in the Playwright runner
to call loadEnvFile with the repository-root .env path, using the existing
WEB_ROOT path symbol defined before it. Preserve quiet loading while ensuring
the root environment file supplies DATABASE_URL.
In `@packages/web/scripts/seed-visual-review-fixtures.ts`:
- Line 116: Replace the string literals assigned to status and verdict in the
fixture seed data with the corresponding generated Prisma status enum and the
document-review verdict enum exported from
`@/lib/tasks/document-review-contracts`. Update both referenced occurrences while
preserving the existing values and surrounding fixture structure.
- Around line 460-464: Reorder the setup calls in main so assertFixtureRunner()
executes before removeFixtureManifest(). Keep assertLocalFixtureDatabase() and
resetVisualReviewFixtures() in their existing order after the runner validation.
- Around line 54-63: Update the local-host validation around databaseUrl and
hostname to recognize URL.hostname’s bracketed IPv6 loopback form “[::1]” in
addition to the existing local hosts, so postgres URLs using ::1 are accepted
without weakening rejection of non-local database hosts.
In `@packages/web/scripts/visual-review-page.mjs`:
- Around line 1855-1871: Update the verdict-handling flow around saveVerdicts
and nextUnreviewedRouteName so a needs-work verdict keeps the current route
selected and leaves its note field available; only looks-right and skipped
should auto-advance to the next unreviewed route. Preserve the existing
completion behavior for verdicts that do advance.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a836a27e-f00e-4d26-9624-635da4f85547
📒 Files selected for processing (13)
packages/web/e2e/utils/visual-routes.tspackages/web/e2e/visual-regression.spec.tspackages/web/e2e/visual-review-page.spec.tspackages/web/package.jsonpackages/web/scripts/build-visual-review.mjspackages/web/scripts/run-playwright.mjspackages/web/scripts/seed-visual-review-fixtures.tspackages/web/scripts/visual-review-coverage.mjspackages/web/scripts/visual-review-coverage.test.mjspackages/web/scripts/visual-review-page.mjspackages/web/scripts/visual-review-page.smoke.mjspackages/web/src/components/tasks/document-review-reviewer-panel.tsxpackages/web/src/components/tasks/task-comment-feed.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/components/tasks/document-review-reviewer-panel.tsx
- packages/web/src/components/tasks/task-comment-feed.tsx
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/e2e/utils/visual-routes.ts (1)
298-318: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the manifest shape before dereferencing it.
The
as DocumentReviewFixtureManifestcast is compile-time only. Valid JSON such asnullcauses Line 311 to crash, while truthy non-string IDs can produce invalid/tasks/[object Object]routes. Add runtime object validation and require all three task IDs to be non-empty strings.🐛 Proposed validation
if ( + !manifest || + typeof manifest !== "object" || + Array.isArray(manifest) || manifest.version !== 1 || - !manifest.managerTaskId || - !manifest.activeReviewTaskId || - !manifest.staleReviewTaskId + typeof manifest.managerTaskId !== "string" || + typeof manifest.activeReviewTaskId !== "string" || + typeof manifest.staleReviewTaskId !== "string" || + !manifest.managerTaskId || + !manifest.activeReviewTaskId || + !manifest.staleReviewTaskId ) {🤖 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/web/e2e/utils/visual-routes.ts` around lines 298 - 318, Update the manifest validation after JSON parsing and before accessing manifest.version or task IDs: ensure the parsed value is a non-null object, has version 1, and has non-empty string values for managerTaskId, activeReviewTaskId, and staleReviewTaskId. Keep the existing invalid-manifest error path and avoid relying on the compile-time DocumentReviewFixtureManifest cast for runtime validation.
🤖 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.
Outside diff comments:
In `@packages/web/e2e/utils/visual-routes.ts`:
- Around line 298-318: Update the manifest validation after JSON parsing and
before accessing manifest.version or task IDs: ensure the parsed value is a
non-null object, has version 1, and has non-empty string values for
managerTaskId, activeReviewTaskId, and staleReviewTaskId. Keep the existing
invalid-manifest error path and avoid relying on the compile-time
DocumentReviewFixtureManifest cast for runtime validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 914271dc-e655-44cd-a8b6-857c8d338535
📒 Files selected for processing (4)
packages/web/e2e/utils/visual-routes.tspackages/web/scripts/build-visual-review.mjspackages/web/scripts/run-playwright.mjspackages/web/scripts/seed-visual-review-fixtures.ts
|
Addressed the outside-diff manifest validation finding in 88d85c3. Runtime validation now rejects null/non-object manifests plus blank or non-string task IDs; focused malformed/valid Playwright collection and the web test typecheck pass. |
Purpose
Give people a simple, reusable way to improve and approve important text without creating a legal-only subsystem or forcing humans to edit documents directly.
Core user stories:
The same flow can support founding documents, proposed laws, scientific protocols, policies, contracts, research reports, and other consequential text.
What changed
Safety boundaries
@optimitron/dbtype changes.REJECTinto approval.Deliberately deferred
This does not add lawyer research or outreach, invitation batches, reminders, contribution receipts, funding, referendum publication, Court-model rewrites, or a new wiki/review/board schema. Those can be added only after this smaller workflow proves the need.
This replaces #167 with a narrower kernel: 42 files and 7,816 additions here versus 145 files and 23,389 additions there.
Verification
pnpm --filter @optimitron/web typecheck:apppnpm --filter @optimitron/web typecheck:testsgit diff --checkSupersedes #167.
Summary by CodeRabbit