95 undo mark complete for checklists#116
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 3dda446 | Commit Preview URL | Dec 20 2025, 10:40 PM |
WalkthroughThis pull request introduces several enhancements across the application stack: improved tooltip positioning with flip and dynamic boundary detection, replacement of FloatingPanel with Tooltip in UI components, WebSocket upgrade request handling in CORS and security middleware, production observability configuration, PDF upload refactoring to use FormData, a new Node.js-based database reset script with cleanup and migration orchestration, LOC reporting automation, build analysis tooling, and various documentation and configuration updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsx (1)
700-708: Keyboard accessibility: info icon is not focusable.The inner
<div>has focus ring styles but lackstabindex="0"orrole="button", making it unreachable via keyboard navigation. Users relying on keyboard-only interaction won't be able to trigger the tooltip.Proposed fix
- <div class='inline-flex items-center justify-center rounded-full p-1.5 opacity-70 hover:opacity-100 focus:opacity-100 focus:ring-2 focus:ring-blue-500 focus:outline-none'> + <div + tabindex="0" + role="button" + aria-label="Question information" + class='inline-flex items-center justify-center rounded-full p-1.5 opacity-70 hover:opacity-100 focus:opacity-100 focus:ring-2 focus:ring-blue-500 focus:outline-none' + >
🧹 Nitpick comments (2)
packages/ui/src/zag/Tooltip.jsx (2)
15-15: Consider removing or documenting the commented-out shift option.If
shift: trueis intentionally disabled, a brief comment explaining why would help future maintainers. Otherwise, remove the dead code.
16-30: The sticky navbar selector is fragile and tightly coupled to class naming.The selector
nav[class*="sticky"]relies on the navbar having "sticky" in its class attribute. If the navbar class changes or uses a different naming convention, this lookup will fail silently and fall back to viewport.Consider using a more explicit approach such as a data attribute (
[data-navbar]) or an ID selector for robustness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/ui/src/zag/Tooltip.jsx(1 hunks)packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsx(2 hunks)packages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/ui/src/zag/Tooltip.jsxpackages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/ui/src/zag/Tooltip.jsxpackages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/ui/src/zag/Tooltip.jsxpackages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/components/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/components/**/*.{jsx,tsx}: Use responsive design principles for UI components
Use Zag.js for UI components and design system
Zag components exist inpackages/web/src/components/zag/*and should be reused; check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state
If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Components should be lean and focused and should not implement business logic; move business logic into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/components/**/*.{jsx,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx}: Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Group related components in subdirectories with an index.js barrel export
Use Zag.js for UI components and design system
Zag component exist inpackages/web/src/components/zag/*and should be reused. Check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Components should be lean and focused. They should not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a God component coordinating multiple large concerns
Files:
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsxpackages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx
🧬 Code graph analysis (1)
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsx (1)
packages/ui/src/zag/Tooltip.jsx (1)
Tooltip(7-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (2)
packages/web/src/components/checklist-ui/compare/NotesCompareSection.jsx (1)
55-55: LGTM - Formatting consistency improvements.These are purely cosmetic reformatting changes that consolidate multi-line expressions into single lines. The logic remains unchanged.
Also applies to: 107-109
packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsx (1)
5-5: Good simplification - FloatingPanel replaced with Tooltip.The migration to
Tooltipfor displaying question info is a cleaner approach that aligns with the enhanced Tooltip positioning logic (flip + boundary awareness).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/workers/package.json (1)
10-10: Consider reusing the existing build script.The deploy script now builds the shared package inline, but line 14 already defines a
buildscript that does the same thing.Optional refactor to reduce duplication
- "deploy": "pnpm --filter shared build && wrangler deploy --env production", + "deploy": "pnpm run build && wrangler deploy --env production",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eslint.config.js(1 hunks)packages/workers/package.json(1 hunks)packages/workers/src/middleware/cors.js(3 hunks)packages/workers/src/middleware/securityHeaders.js(1 hunks)packages/workers/wrangler.jsonc(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
eslint.config.jspackages/workers/package.jsonpackages/workers/src/middleware/securityHeaders.jspackages/workers/wrangler.jsoncpackages/workers/src/middleware/cors.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
eslint.config.jspackages/workers/src/middleware/securityHeaders.jspackages/workers/src/middleware/cors.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/middleware/securityHeaders.jspackages/workers/src/middleware/cors.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/securityHeaders.jspackages/workers/src/middleware/cors.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/middleware/securityHeaders.jspackages/workers/src/middleware/cors.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (5)
eslint.config.js (1)
33-33: LGTM! Appropriate addition of theperformancebrowser global.The
performanceAPI is a standard browser global used for performance measurements (e.g.,performance.now(),performance.mark()). This addition correctly prevents ESLint from flagging legitimate uses of the performance API as undefined variables.packages/workers/src/middleware/securityHeaders.js (1)
14-18: LGTM! Correct WebSocket upgrade handling.The guard correctly skips security headers for WebSocket upgrade responses (status 101). This is necessary because WebSocket connections cannot have standard HTTP headers applied after the protocol upgrade.
packages/workers/src/middleware/cors.js (2)
14-46: LGTM! Proper WebSocket upgrade bypass for CORS.The refactoring correctly:
- Extracts the CORS handler for clarity
- Wraps it to bypass CORS processing for WebSocket upgrade requests
- Checks the request
Upgradeheader before CORS logic runsThis approach complements the security headers middleware, which skips headers after detecting a 101 response status. Together, they properly handle WebSocket upgrades at both the request and response stages.
25-25: Content-Length header is required for file upload validation.The
Content-Lengthheader is explicitly read in the pdfs and avatars upload routes for early rejection of oversized files. This header allows the application to validate payload size before processing the request body, improving both security and performance. The addition is intentional and necessary.Likely an incorrect or invalid review comment.
packages/workers/wrangler.jsonc (1)
88-97: Consider environment-specific sampling rates for production use at scale.A
head_sampling_rateof 1 (100%) follows Cloudflare's recommended best practices for observability and provides complete visibility. However, for high-traffic applications, lower sampling rates help reduce log volume and manage costs. Production environments may benefit from lower rates (e.g., 0.1) while maintaining 100% sampling in staging/development for debugging.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web/src/api/pdf-api.js(1 hunks)packages/workers/src/routes/pdfs.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/routes/pdfs.jspackages/web/src/api/pdf-api.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/workers/src/routes/pdfs.jspackages/web/src/api/pdf-api.js
packages/workers/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/workers/**/*.{js,ts}: Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/routes/pdfs.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/routes/pdfs.jspackages/web/src/api/pdf-api.js
packages/workers/src/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/workers/src/**/*.{js,ts}: Use Zod for schema and input validation on the backend
Use Drizzle ORM for database interactions and migrations
Use Better-Auth for authentication and user management
Files:
packages/workers/src/routes/pdfs.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/api/pdf-api.js
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/api/pdf-api.js
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/api/pdf-api.js
🧬 Code graph analysis (2)
packages/workers/src/routes/pdfs.js (1)
packages/workers/src/index.js (1)
projectId(308-308)
packages/web/src/api/pdf-api.js (1)
packages/workers/src/routes/pdfs.js (5)
formData(145-145)file(146-146)fileName(140-140)fileName(258-258)fileName(327-327)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (1)
packages/web/src/api/pdf-api.js (1)
43-61: LGTM! Consistent FormData approach improves browser compatibility.The refactor to always use FormData is a good improvement:
- Provides consistent upload behavior for both File and ArrayBuffer inputs
- Eliminates manual header management (browser handles Content-Type boundary automatically)
- Better cross-browser compatibility, especially for Safari
- Aligns perfectly with the backend's multipart/form-data handling (lines 143-146 in
packages/workers/src/routes/pdfs.js)The ArrayBuffer → Blob → File conversion is correct and necessary for FormData compatibility.
| // Note: Frontend already checks for duplicate filenames before uploading, | ||
| // so we skip the R2 head() call to reduce latency. Race conditions are | ||
| // unlikely and would just result in an overwrite. | ||
| const key = `projects/${projectId}/studies/${studyId}/${fileName}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PDF upload-related files in backend
echo "=== Searching for pdfs.js and related upload handling ==="
fd -e js -e ts "pdf" packages/workers/src/routes/ 2>/dev/null | head -20
# Display pdfs.js content
echo -e "\n=== Examining pdfs.js upload route ==="
if [ -f "packages/workers/src/routes/pdfs.js" ]; then
wc -l "packages/workers/src/routes/pdfs.js"
echo "Lines 210-230:"
sed -n '210,230p' "packages/workers/src/routes/pdfs.js"
fi
# Search for any listPdfs or file existence checks in backend
echo -e "\n=== Searching for duplicate prevention logic in backend ==="
rg -n "listPdfs|head\(|exists" packages/workers/src/ --type=ts --type=js -B 2 -A 2 | head -50Repository: InfinityBowman/corates
Length of output: 4896
Reconsider removing the R2 existence check for PDF uploads.
The removal of the R2 head() call introduces real data loss risks that frontend validation alone cannot prevent:
-
Race conditions with concurrent uploads: R2's last-writer-wins model means the last upload to the same key overwrites all previous versions. Two simultaneous requests with identical filenames will silently overwrite data without error.
-
Frontend validation is not sufficient protection: Frontend checks cannot prevent programmatic API calls, direct client-side bypasses, or simultaneous uploads that occur before validation completes. R2 does not support object versioning, so overwrites cannot be recovered.
-
No safety net exists: Unlike S3, R2 provides no recovery mechanism. Overwritten PDFs are permanently lost.
Recommended mitigations:
- Restore the
head()check beforeput()to detect existing files (minimal latency impact) - Use unique filenames (UUID or timestamp suffix) instead of user-provided names if overwrites are unacceptable
- If accepting overwrites, explicitly document this behavior in API docs so users understand the risk
- Consider adding an optional
overwrite=trueparameter to make overwrites explicit rather than silent
🤖 Prompt for AI Agents
In packages/workers/src/routes/pdfs.js around lines 216 to 219, the R2 existence
check (head()) was removed which allows silent overwrites and data loss for
concurrent or bypassed uploads; restore a pre-put R2.head(key) check and return
a 409 (or similar) when a file exists unless the request explicitly permits
overwrite (e.g., an overwrite=true query/body flag), or alternatively generate
and persist unique filenames (append a UUID/timestamp) before put() if
overwrites are unacceptable; update API docs to document overwrite behavior and
ensure the handler honors the overwrite flag to skip the head check when true.
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 (1)
packages/web/src/stores/projectActionsStore/studies.js (1)
241-279: Add error handling for async deleteStudy cleanupThe caller at
packages/web/src/components/project-ui/all-studies-tab/study-card/StudyCardHeader.jsx:58invokesprojectActionsStore.study.delete(study().id)without awaiting the returned Promise. Since deleteStudy is now async and performs multi-stage cleanup (R2 storage, IndexedDB cache, then Y.js deletion), the Promise must be handled.Either await the function with error handling, or explicitly handle it as fire-and-forget with
.catch()to ensure cleanup failures are logged and don't silently fail during component lifecycle changes.
🧹 Nitpick comments (5)
scripts/loc-report.mjs (2)
12-17: Consider consistent import style.Line 12 uses the
node:protocol prefix (node:child_process), while lines 13-17 use bare module names (fs,path,os,url). Consider using thenode:prefix consistently for all Node.js built-in modules.Proposed fix
import { spawnSync } from 'node:child_process'; -import { writeFileSync, unlinkSync, mkdtempSync } from 'fs'; -import { join } from 'path'; -import { tmpdir } from 'os'; -import { fileURLToPath } from 'url'; -import { dirname } from 'path'; +import { writeFileSync, unlinkSync, mkdtempSync } from 'node:fs'; +import { join, dirname } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fileURLToPath } from 'node:url';
94-100: Temp directory is not cleaned up.The temp directory created by
mkdtempSyncon line 83 is not removed during cleanup - only the file inside it is deleted. Consider usingrmdirSyncorrmSyncto clean up the directory as well.Proposed fix
+import { writeFileSync, unlinkSync, mkdtempSync, rmdirSync } from 'node:fs';} finally { try { unlinkSync(tmpFile); + rmdirSync(tmpDir); } catch { // Ignore cleanup errors } }packages/workers/scripts/reset-db-prod.mjs (3)
19-39: Consider extracting shared utility.This
runCommandfunction is nearly identical to the one inscripts/loc-report.mjs. For maintainability, consider extracting it to a shared utility module (e.g.,scripts/lib/exec.mjs).
75-85: Unused R2 bucket functions.
deleteR2BucketandcreateR2Bucketare defined but their usage is commented out (lines 117-137). Consider removing these functions if R2 bucket management is intentionally excluded, or add a comment explaining why they're kept for future use.
92-92:asyncis unnecessary.The
mainfunction is declaredasyncbut contains noawaitexpressions. All operations use synchronousspawnSync. Consider removingasyncor converting to asyncspawnif you want non-blocking execution.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(1 hunks)packages/web/src/stores/projectActionsStore/pdfs.js(2 hunks)packages/web/src/stores/projectActionsStore/studies.js(2 hunks)packages/workers/package.json(2 hunks)packages/workers/scripts/reset-db-prod.mjs(1 hunks)packages/workers/scripts/reset-db-prod.sh(0 hunks)scripts/loc-report.mjs(1 hunks)scripts/loc-report.sh(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/loc-report.sh
- packages/workers/scripts/reset-db-prod.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workers/package.json
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
scripts/loc-report.mjspackages/workers/scripts/reset-db-prod.mjspackages/web/src/stores/projectActionsStore/studies.jspackage.jsonpackages/web/src/stores/projectActionsStore/pdfs.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
packages/web/src/stores/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Solid's
createStorefor complex state or state objects for better performance and reactivity
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
🧠 Learnings (2)
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/scripts/reset-db-prod.mjs
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/scripts/reset-db-prod.mjs
🧬 Code graph analysis (2)
packages/workers/scripts/reset-db-prod.mjs (1)
scripts/loc-report.mjs (10)
__dirname(19-19)ROOT(20-20)args(116-116)result(23-23)result(35-40)result(58-58)result(88-90)stderr(43-43)stdout(44-44)message(45-45)
packages/web/src/stores/projectActionsStore/pdfs.js (3)
packages/web/src/api/pdf-api.js (1)
deletePdf(113-127)packages/web/src/stores/projectActionsStore/index.js (1)
packages/web/src/primitives/pdfCache.js (1)
removeCachedPdf(146-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (4)
package.json (1)
33-33: LGTM!The script path update correctly points to the new Node.js-based LOC report script, aligning with the tooling migration.
scripts/loc-report.mjs (1)
111-175: LGTM!The main function logic is well-structured with proper argument handling, file filtering, package grouping, and error handling.
packages/workers/scripts/reset-db-prod.mjs (1)
41-56: LGTM!The D1 execution wrapper correctly builds the wrangler command with appropriate flags for remote production execution.
packages/web/src/stores/projectActionsStore/studies.js (1)
6-6:clearStudyCacheis properly exported from@primitives/pdfCache.js(line 170) and the import in studies.js is valid. No runtime error will occur.
| console.log(''); | ||
| console.log('=========================================='); | ||
| console.log(' Database reset, R2 cleared, and workers deployed!'); | ||
| console.log('=========================================='); |
There was a problem hiding this comment.
Misleading success message.
The message "R2 cleared" is misleading since the R2 bucket clearing code (Step 2) is commented out. Update the message to reflect the actual operations performed.
Proposed fix
console.log('');
console.log('==========================================');
- console.log(' Database reset, R2 cleared, and workers deployed!');
+ console.log(' Database reset and workers deployed!');
console.log('==========================================');🤖 Prompt for AI Agents
In packages/workers/scripts/reset-db-prod.mjs around lines 152 to 155, the
printed success message incorrectly claims "R2 cleared" even though the R2
bucket clearing step is commented out; update the final console output to
accurately reflect the performed actions (e.g., remove or replace "R2 cleared"
with "R2 unchanged" or mention only the operations actually executed such as
"Database reset and workers deployed"), keeping the surrounding formatting
intact.
| function checkCommand(command) { | ||
| const result = spawnSync('which', [command], { encoding: 'utf8' }); | ||
| if (result.status !== 0) { | ||
| if (command === 'cloc') { | ||
| console.error(`cloc not found in PATH. Install it: https://github.com/AlDanial/cloc`); | ||
| } else { | ||
| console.error(`${command} not found in PATH`); | ||
| } | ||
| process.exit(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
which command is not available on Windows.
The which command is Unix-specific and will fail on Windows. If cross-platform support is needed, consider using where on Windows or a library like which from npm.
Cross-platform alternative
function checkCommand(command) {
- const result = spawnSync('which', [command], { encoding: 'utf8' });
+ const isWindows = process.platform === 'win32';
+ const whichCmd = isWindows ? 'where' : 'which';
+ const result = spawnSync(whichCmd, [command], { encoding: 'utf8', shell: isWindows });
if (result.status !== 0) {🤖 Prompt for AI Agents
In scripts/loc-report.mjs around lines 22 to 32, the script calls the Unix-only
which command to detect binaries and will fail on Windows; update the
checkCommand function to be cross-platform by using process.platform === 'win32'
to run 'where' instead of 'which' on Windows (or prefer importing the npm which
package and use its sync API), try the platform-appropriate command and treat a
non-zero exit as missing, and keep the existing cloc-specific error text and
exit code; ensure encoding and status checks remain the same and handle any
spawnSync exceptions consistently.
… 95-undo-mark-complete-for-checklists
….com/InfinityBowman/corates into 95-undo-mark-complete-for-checklists
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/stores/projectActionsStore/pdfs.js (1)
202-243: Improve error handling for PDF deletion to prevent orphaned metadata and notify users of failures.The current implementation has two related issues:
Unhandled promise rejection in UI: The
handleDeletecall in StudyPdfSection.jsx doesn't await or catch the deletion promise, so errors silently fail without user notification.Real orphaned metadata risk: When R2 deletion succeeds but Y.js removal fails, the PDF is deleted from storage and cache but remains in the Y.js document, creating an inconsistency. While Y.js operations are typically atomic, better error recovery is needed.
Consider:
- Wrap the deletion call in the UI with try-catch and show error toasts to users
- Add exponential backoff retry logic for Y.js removal since network/sync issues can be transient
- Document the orphaned metadata scenario as a known edge case requiring manual cleanup if it occurs
🧹 Nitpick comments (2)
docs/architecture/diagrams/05-frontend-routes.md (2)
46-52: Inconsistent table formatting across route documentation sections.The Public Routes, Project Routes, and Local Routes tables have been reformatted to the pipe-delimited markdown table style. However, the Protected Routes table (lines 58–63) remains in the original format, creating inconsistency. For clarity and maintainability, all route tables should use the same formatting convention.
🔎 Proposed fix to reformat Protected Routes table
### Protected Routes (ProtectedGuard) Requires authentication. Redirects to signin if not logged in. -| Route | Component | Purpose | -| ------------------- | -------------- | ----------------------- | -| `/profile` | ProfilePage | User profile management | -| `/settings` | SettingsPage | App settings | -| `/settings/billing` | BillingPage | Subscription management | -| `/admin` | AdminDashboard | Admin-only features | +| Route | Component | Purpose | +| ------------------- | -------------- | ------------------------- | +| `/profile` | ProfilePage | User profile management | +| `/settings` | SettingsPage | App settings | +| `/settings/billing` | BillingPage | Subscription management | +| `/admin` | AdminDashboard | Admin-only features |Also applies to: 58-63, 67-71, 77-79
70-71: Clarify the full path syntax in the Project Routes table.Line 70 uses shorthand notation (
/.../checklists/:checklistId), while line 69 shows the explicit full path. For documentation clarity, consider expanding the shorthand on line 70 to show the complete route path consistently, or document why shorthand is used. This will help readers understand the full URL structure at a glance.🔎 Proposed fix to use explicit path syntax
-| `/.../checklists/:checklistId` | ChecklistYjsWrapper | Checklist assessment | +| `/projects/:projectId/studies/:studyId/checklists/:checklistId` | ChecklistYjsWrapper | Checklist assessment |
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.cursor/mcp.json(1 hunks).github/SECURITY.md(1 hunks)docs/architecture/diagrams/01-package-architecture.md(1 hunks)docs/architecture/diagrams/04-data-model.md(1 hunks)docs/architecture/diagrams/05-frontend-routes.md(2 hunks)docs/architecture/diagrams/06-api-routes.md(2 hunks)packages/web/.gitignore(1 hunks)packages/web/package.json(2 hunks)packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx(1 hunks)packages/web/src/stores/projectActionsStore/pdfs.js(2 hunks)packages/web/src/stores/projectActionsStore/studies.js(2 hunks)packages/web/vite.config.js(2 hunks)packages/workers/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/architecture/diagrams/04-data-model.md
- .cursor/mcp.json
- docs/architecture/diagrams/01-package-architecture.md
🧰 Additional context used
📓 Path-based instructions (11)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/web/vite.config.jsdocs/architecture/diagrams/05-frontend-routes.mdpackages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.jsdocs/architecture/diagrams/06-api-routes.mdpackages/web/package.jsonpackages/workers/package.jsonpackages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/vite.config.jspackages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.jspackages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/web/vite.config.jspackages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.jspackages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Import stores directly where needed instead of passing values through multiple components
When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
You may create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.jspackages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/**/*.{jsx,tsx,js,ts,css}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.jspackages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code (Safari is usually problematic)
Keep files small, focused, and modular. If a file exceeds a high number of lines, consider refactoring by extracting sub-modules into a folder with index.jsx and helper components, moving complex logic into separate utility files or primitives, or splitting large forms into section components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
UsecreateMemofor derived values to ensure they update reactively
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.jspackages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/stores/**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Solid's
createStorefor complex state or state objects for better performance and reactivity
Files:
packages/web/src/stores/projectActionsStore/studies.jspackages/web/src/stores/projectActionsStore/pdfs.js
packages/web/src/components/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/components/**/*.{jsx,tsx}: Use responsive design principles for UI components
Use Zag.js for UI components and design system
Zag components exist inpackages/web/src/components/zag/*and should be reused; check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state
If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Components should be lean and focused and should not implement business logic; move business logic into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
Files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/components/**/*.{jsx,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx}: Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Group related components in subdirectories with an index.js barrel export
Use Zag.js for UI components and design system
Zag component exist inpackages/web/src/components/zag/*and should be reused. Check the README.md in that folder for a list of existing components before adding new components and when debugging
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly from the props object or wrap them in a function to ensure they are always up-to-date
Components should be lean and focused. They should not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a God component coordinating multiple large concerns
Files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
🧠 Learnings (8)
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/**/*.{js,jsx,ts,tsx} : Prefer using config files rather than hardcoding values
Applied to files:
packages/web/vite.config.js
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/package.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Drizzle ORM for database interactions and migrations
Applied to files:
packages/workers/package.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/workers/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/package.json
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/workers/src/**/*.{js,ts} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/package.json
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : Import stores directly where needed instead of passing values through multiple components
Applied to files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
📚 Learning: 2025-12-19T12:38:18.491Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-19T12:38:18.491Z
Learning: Applies to packages/web/src/components/**/*.{jsx,tsx} : Zag components exist in `packages/web/src/components/zag/*` and should be reused; check the README.md in that folder for a list of existing components before adding new components and when debugging
Applied to files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
📚 Learning: 2025-12-19T14:49:49.717Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-19T14:49:49.717Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use `createMemo` for derived values to ensure they update reactively
Applied to files:
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx
🧬 Code graph analysis (2)
packages/web/src/stores/projectActionsStore/studies.js (2)
packages/web/src/api/pdf-api.js (1)
deletePdf(113-127)packages/web/src/primitives/pdfCache.js (1)
clearStudyCache(170-198)
packages/web/src/stores/projectActionsStore/pdfs.js (2)
packages/web/src/api/pdf-api.js (1)
deletePdf(113-127)packages/web/src/primitives/pdfCache.js (1)
removeCachedPdf(146-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (12)
docs/architecture/diagrams/06-api-routes.md (2)
92-96: Clear and consistent billing endpoint documentation.The Billing section properly documents the new Stripe integration endpoints with appropriate descriptions. Formatting and structure align well with existing sections.
116-118: Database endpoints properly documented as development-only.The added database endpoint documentation includes an appropriate caveat that these are development/diagnostic only, which is important for clarity about production usage. Format is consistent with the rest of the document.
docs/architecture/diagrams/05-frontend-routes.md (1)
29-29: Integrate the new reconciliation route into the Mermaid flowchart.The new
reconcileroute is defined in the Project Routes subgraph (line 29) and correctly connects fromchecklistview(line 37), which is appropriate for the workflow. This addition aligns with the PR objective to support checklist reconciliation.Also applies to: 37-37
packages/web/src/components/checklist-ui/compare/ChecklistReconciliation.jsx (1)
18-18: LGTM! Static import improves consistency.The switch from dynamic to static import simplifies the code and aligns with the rest of the file's import pattern. Since
createChecklistis used in core functionality (handleReset), this is an appropriate optimization that improves bundle optimization and code clarity.packages/workers/package.json (2)
22-22: LGTM!The migration from a shell script to a Node.js script improves cross-platform compatibility and aligns with other utility scripts in this package.
10-10: Both--filter sharedand--filter @corates/sharedresolve to the same package (@corates/shared) in this workspace, as there is no naming ambiguity. Both filter forms are functionally equivalent and work correctly; no resolution issue exists here.packages/web/.gitignore (1)
14-14: LGTM!The addition of
bundle-analysis.htmlto the gitignore is appropriate for the new analyze workflow introduced in this PR..github/SECURITY.md (1)
16-20: LGTM!The expanded security policy scope appropriately reflects the PDF handling and storage workflows touched by this PR.
packages/web/package.json (2)
15-16: LGTM!The new analyze script properly integrates with the mode-aware Vite configuration to enable build analysis and visualization.
40-43: LGTM!The added devDependencies appropriately support the new build analysis workflow.
packages/web/vite.config.js (2)
6-6: LGTM!The function-based configuration enables mode-aware builds, aligning with the new analyze workflow. This follows the coding guideline to prefer config files over hardcoding values.
30-36: LGTM!The mode-aware build options are correctly structured:
terserminification in analyze mode provides better sourcemaps for debuggingesbuildminification in other modes remains fast- Sourcemaps are only generated when needed for analysis
- Comment stripping maintains clean output
| * Cleans up all associated PDFs from R2 and IndexedDB before deleting from Y.js | ||
| */ | ||
| function deleteStudy(studyId) { | ||
| async function deleteStudy(studyId) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of deleteStudy to verify they handle async properly
# Search for calls to delete method on study actions
rg -n -B3 -A3 '\.delete\s*\(\s*studyId|studies\.delete|projectActions.*\.studies\.delete' --type=js --type=jsx --type=ts --type=tsx
# Search for deleteStudy direct calls
rg -n -B3 -A3 'deleteStudy\s*\(' --type=js --type=jsx --type=ts --type=tsxRepository: InfinityBowman/corates
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Find all callers of deleteStudy with glob patterns
# Search for deleteStudy direct calls
rg -n -B3 -A3 'deleteStudy\s*\(' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx'
# Also search for .delete( pattern which might be the method call
rg -n -B5 -A5 'studies\.delete\s*\(' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx'Repository: InfinityBowman/corates
Length of output: 3462
🏁 Script executed:
#!/bin/bash
# Check the store implementation more carefully
cat -n packages/web/src/stores/projectActionsStore/studies.js | sed -n '262,300p'
# Check the useProject wrapper
cat -n packages/web/src/primitives/useProject/studies.js | sed -n '108,125p'
# Check the useProject index export
cat -n packages/web/src/primitives/useProject/index.js | sed -n '320,326p'Repository: InfinityBowman/corates
Length of output: 2953
🏁 Script executed:
#!/bin/bash
# Find direct imports and calls of the store's deleteStudy
rg -n 'deleteStudy' packages/web/src/stores/projectActionsStore/index.js -A5 -B5
# Find all files that import from projectActionsStore/studies
rg -n 'from.*projectActionsStore' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx'
# Check if deleteStudy is exported from the store
cat -n packages/web/src/stores/projectActionsStore/index.js | grep -A20 -B5 'studies\|deleteStudy'Repository: InfinityBowman/corates
Length of output: 2666
🏁 Script executed:
#!/bin/bash
# Find actual usages of deleteStudy in components
rg -n 'deleteStudy' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' -B3 -A3
# Check the complete export structure of projectActionsStore
cat -n packages/web/src/stores/projectActionsStore/index.js | tail -100Repository: InfinityBowman/corates
Length of output: 9099
🏁 Script executed:
#!/bin/bash
# Find actual calls to projectActionsStore.study.delete or .delete
rg -n 'projectActionsStore\.study\.delete|\.study\.delete\s*\(' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' -B3 -A3
# Also check for await usage on deleteStudy calls
rg -n 'await.*deleteStudy|await.*\.delete\(' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx'Repository: InfinityBowman/corates
Length of output: 2704
🏁 Script executed:
#!/bin/bash
# Examine the StudyCardHeader context more closely
cat -n packages/web/src/components/project-ui/all-studies-tab/study-card/StudyCardHeader.jsx | sed -n '50,80p'
# Check if there's a confirmation dialog or async handling
cat -n packages/web/src/components/project-ui/all-studies-tab/study-card/StudyCardHeader.jsx | sed -n '1,100p'Repository: InfinityBowman/corates
Length of output: 5021
🏁 Script executed:
#!/bin/bash
# Check what ops.deleteStudy actually is - likely from useProject connection ops
cat -n packages/web/src/primitives/useProject/studies.js | sed -n '100,120p'
# Search for where studyOps is created/populated
rg -n 'studyOps\s*[=:]|studyOps\s*\{' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' -B5 -A10Repository: InfinityBowman/corates
Length of output: 3639
🏁 Script executed:
#!/bin/bash
# Check createStudyOperations to understand what ops.deleteStudy is
rg -n 'function createStudyOperations|export.*createStudyOperations' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' -A30
# Check if there are any await statements in the store's deleteStudy
cat -n packages/web/src/stores/projectActionsStore/studies.js | sed -n '262,300p'Repository: InfinityBowman/corates
Length of output: 4750
Callers of deleteStudy must await this function.
The async signature is necessary because the function performs critical cleanup operations (R2 PDF deletion and IndexedDB cache clearing) before deleting the study. Currently, callers in StudyCardHeader.jsx (line 58) and the test don't await this function, which means cleanup operations won't complete before the component continues execution. This can result in orphaned PDFs in R2 or stale cache data.
Update callers to properly await the async operation.
🤖 Prompt for AI Agents
In packages/web/src/stores/projectActionsStore/studies.js around line 262,
deleteStudy is async because it performs R2 PDF deletion and IndexedDB cache
clearing, but its callers do not await it; update the callers (notably
StudyCardHeader.jsx at line 58 and the related test) to await the deleteStudy
call (or return the promise) and make their enclosing handlers async if
necessary, and wrap the await in try/catch to handle/report errors so cleanup
completes before proceeding.
| if (pdfs.length > 0) { | ||
| const deletePromises = pdfs.map(pdf => { | ||
| if (!pdf?.fileName) return Promise.resolve(); | ||
| return deletePdf(projectId, studyId, pdf.fileName).catch(err => { | ||
| // Log but don't block - continue with other PDFs | ||
| console.warn(`Failed to delete PDF ${pdf.fileName} from R2:`, err); | ||
| }); | ||
| }); | ||
| await Promise.all(deletePromises); | ||
| } | ||
|
|
||
| // Clear all PDFs for this study from IndexedDB | ||
| await clearStudyCache(projectId, studyId).catch(err => { | ||
| // Log but don't block - continue with Y.js deletion | ||
| console.warn('Failed to clear study PDF cache from IndexedDB:', err); | ||
| }); | ||
|
|
||
| // Finally, delete the study from Y.js | ||
| ops.deleteStudy(studyId); |
There was a problem hiding this comment.
Inconsistent error handling may cause data inconsistencies.
The PDF cleanup in this function (lines 277-285) differs significantly from the deletion flow in pdfs.js:
In pdfs.js (deletePdfFromStudy):
- Strict consistency: throws if R2 deletion fails
- Conditional Y.js removal: only if R2 succeeds
- Clear error propagation
In this function:
- Best-effort: logs warnings and continues on R2 failures (line 281-283)
- Always deletes from Y.js regardless of cleanup success (line 295)
This creates potential inconsistencies:
- If R2 deletion fails for some PDFs but Y.js deletion succeeds, PDFs remain orphaned in R2 storage
- Users lose references to these PDFs in Y.js, but files continue consuming storage
- No way to recover or retry the cleanup
Consider:
- Collecting failures and throwing if any R2 deletions fail, or
- Using the same deletion strategy as
pdfs.jsby calling the existingdeletePdfFromStudymethod instead of reimplementing, or - At minimum, counting failures and logging a clear warning about potential orphaned files
Proposed fix: Reuse existing deletion flow
try {
// Get study data to access PDFs before deletion
const study = projectStore.getStudy(projectId, studyId);
const pdfs = study?.pdfs || [];
- // Delete each PDF from R2 storage
+ // Delete each PDF using the existing deletion flow from pdfs.js
+ // This ensures consistent error handling and proper cleanup
if (pdfs.length > 0) {
- const deletePromises = pdfs.map(pdf => {
- if (!pdf?.fileName) return Promise.resolve();
- return deletePdf(projectId, studyId, pdf.fileName).catch(err => {
- // Log but don't block - continue with other PDFs
- console.warn(`Failed to delete PDF ${pdf.fileName} from R2:`, err);
- });
- });
- await Promise.all(deletePromises);
+ // Note: This assumes you pass the pdf deletion function to this module
+ // or import it from a shared location
+ for (const pdf of pdfs) {
+ if (!pdf?.fileName) continue;
+ try {
+ // Use R2 deletion only (cache and Y.js will be handled below)
+ await deletePdf(projectId, studyId, pdf.fileName);
+ } catch (err) {
+ console.warn(`Failed to delete PDF ${pdf.fileName}:`, err);
+ // Continue with other PDFs
+ }
+ }
}
// Clear all PDFs for this study from IndexedDB
await clearStudyCache(projectId, studyId).catch(err => {
// Log but don't block - continue with Y.js deletion
console.warn('Failed to clear study PDF cache from IndexedDB:', err);
});
// Finally, delete the study from Y.js
+ // This will remove all PDF references in Y.js as well
ops.deleteStudy(studyId);
} catch (err) {Committable suggestion skipped: line range outside the PR's diff.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.