Allow multiple pdf upload and tagging#79
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds multi-PDF management across UI and data layers: new PDF components and modals, tag and citation metadata, cache-aware upload/download/delete flows with orphan cleanup, Y.js sync updates (pdfId, tag, citation fields), All-Studies UI rewrite, and Zag machine initialization refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI (Checklist / AllStudies)
participant Handlers as PDF Handlers
participant API as Project API (pdfs.js)
participant YDoc as Y.js Store
participant Cache as Local Cache
participant Server
rect rgb(200,220,255)
Note over User,Server: Upload + Add to Study
User->>UI: selects/uploads PDF
UI->>Handlers: handleUploadPdf(studyId, file)
Handlers->>Server: uploadPdf(file)
Server-->>Handlers: uploaded file metadata
Handlers->>Cache: cachePdf(data)
Handlers->>API: addPdfToStudy(studyId, pdfInfo, tag)
API->>YDoc: create pdfId, store metadata (id,key,fileName,tag,citation...)
YDoc-->>API: confirm
API-->>Handlers: pdfId
Handlers->>UI: update list/selection
end
rect rgb(220,255,220)
Note over User,Server: View / Download (cache-first)
User->>UI: view/download PDF
UI->>Handlers: handleViewPdf / handleDownloadPdf
alt cache hit
Handlers->>Cache: getCachedPdf(key)
Cache-->>Handlers: pdf data
else cache miss
Handlers->>Server: downloadPdf(key)
Server-->>Handlers: pdf data
Handlers->>Cache: cachePdf(data)
end
Handlers->>UI: render or trigger browser download
end
rect rgb(255,240,200)
Note over User,Server: Tag / Metadata Update
User->>UI: change tag or edit metadata
UI->>Handlers: handleTagChange / handleUpdatePdfMetadata
Handlers->>API: updatePdfTag / updatePdfMetadata
API->>YDoc: clear conflicting tags (if needed) and update fields
YDoc-->>API: confirm
API-->>Handlers: success
Handlers->>UI: refresh display
end
rect rgb(255,220,220)
Note over User,Server: Delete + Cleanup
User->>UI: confirm delete
UI->>Handlers: handleDeletePdf(studyId,pdf)
Handlers->>Server: deletePdf(key)
Server-->>Handlers: confirm
Handlers->>API: removePdfFromStudy(studyId,pdfId)
API->>YDoc: delete pdf entry
YDoc-->>API: confirm
Handlers->>Cache: clear cached pdf
Handlers->>UI: update selection/list
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | bf26c69 | Commit Preview URL | Dec 18 2025, 05:00 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
.github/copilot-instructions.md (1)
11-11: Minor grammar correction."client side" should be hyphenated as "client-side" when used as a compound adjective.
🔎 Apply this diff to fix the grammar:
-Do not worry about migrations either client side or backend unless specifically instructed to do so in the prompt. This project is not in production and has no users. +Do not worry about migrations either client-side or backend unless specifically instructed to do so in the prompt. This project is not in production and has no users.packages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsx (1)
22-27: Consider usingcreateMemofor reactive derived values.The
disabledValuesfunction computes values based on props. Per SolidJS best practices and coding guidelines, derived values from props should usecreateMemoto ensure proper reactive updates.🔎 Apply this diff to use createMemo:
+import { createMemo } from 'solid-js'; import { Select } from '@corates/ui'; const TAG_OPTIONS = [ { label: 'Primary Report', value: 'primary' }, { label: 'Protocol', value: 'protocol' }, { label: 'Secondary', value: 'secondary' }, ]; export default function PdfTagSelect(props) { - const disabledValues = () => { + const disabledValues = createMemo(() => { const disabled = []; if (props.disablePrimary) disabled.push('primary'); if (props.disableProtocol) disabled.push('protocol'); return disabled; - }; + });As per coding guidelines: "When you need to compute a value based on props or state, use
createMemoto ensure it updates reactively."packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (2)
42-74: Consider usingcreateMemofor derived values.The
tagMenuItemsfunction computes menu items based on props. Per SolidJS best practices, derived values from props should usecreateMemoto ensure proper reactive updates and avoid unnecessary recalculations.🔎 Apply this diff to use createMemo:
-import { Show, createSignal } from 'solid-js'; +import { Show, createSignal, createMemo } from 'solid-js'; import { FiTrash2, FiEye, FiDownload } from 'solid-icons/fi'; import { CgFileDocument } from 'solid-icons/cg'; import { Menu, Dialog } from '@corates/ui'; import PdfTagBadge from './PdfTagBadge.jsx'; export default function PdfListItem(props) { const [showDeleteConfirm, setShowDeleteConfirm] = createSignal(false); - const tagMenuItems = () => { + const tagMenuItems = createMemo(() => { const items = []; // Only show tag options that make sense if (props.pdf.tag !== 'primary') { items.push({ id: 'set-primary', label: props.hasPrimary && props.pdf.tag !== 'primary' ? 'Set as Primary (replaces current)' : 'Set as Primary', }); } if (props.pdf.tag !== 'protocol') { items.push({ id: 'set-protocol', label: props.hasProtocol && props.pdf.tag !== 'protocol' ? 'Set as Protocol (replaces current)' : 'Set as Protocol', }); } if (props.pdf.tag !== 'secondary') { items.push({ id: 'set-secondary', label: 'Set as Secondary', }); } return items; - }; + });As per coding guidelines: "When you need to compute a value based on props or state, use
createMemoto ensure it updates reactively."
30-40: Optional: Consider extracting formatting utilities.The
formatFileSizeandformatDatehelper functions could be extracted to a shared utility file (e.g.,@/utils/formatters.js) for reuse across components. However, since the component is still focused and manageable in size, this refactor can be deferred.packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (2)
25-34: Consider extracting the sorting logic to a shared utility.This sorting logic is duplicated in
PdfList.jsx(lines 26-36). To follow the DRY principle, consider extracting it to a shared utility function.🔎 Suggested approach
Create a utility in a shared location:
// e.g., in utils/pdfUtils.js or primitives/ export const PDF_TAG_ORDER = { primary: 0, protocol: 1, secondary: 2 }; export function sortPdfsByTag(pdfs) { return [...pdfs].sort((a, b) => { const tagA = PDF_TAG_ORDER[a.tag] ?? 2; const tagB = PDF_TAG_ORDER[b.tag] ?? 2; return tagA - tagB; }); }
68-79: Consider reusingPdfTagBadgecomponent for consistency.The inline badge rendering duplicates the
PdfTagBadgecomponent's functionality. While the compact menu item styling may justify this, consider whetherPdfTagBadgecould accept asizeorcompactprop to enable reuse here.packages/web/src/components/checklist-ui/pdf/PdfList.jsx (1)
16-25: Prop count exceeds guideline (8 props vs. 5 max).Per coding guidelines, components should receive at most 1–5 props. Consider grouping related callbacks into a single
handlersobject prop or moving PDF operations into an external store/context that children can access directly.🔎 Suggested approach
Option 1 - Group callbacks:
-// props.onView, onDownload, onDelete, onTagChange, onUpload +// props.handlers: { onView, onDownload, onDelete, onTagChange, onUpload }Option 2 - Use a PDF operations context/store that
PdfListandPdfListItemcan access directly, reducing prop drilling of handlers.packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
297-304: Redundant visibility check.The
Show when={studyPdfs().length > 1}duplicatesPdfSelector's internalshouldShowlogic. While harmless, you could simplify by always renderingPdfSelectorand letting it handle its own visibility.🔎 Suggested simplification
- {/* PDF Selector - only shown when multiple PDFs exist */} - <Show when={studyPdfs().length > 1}> - <PdfSelector - pdfs={studyPdfs()} - selectedPdfId={selectedPdfId()} - onSelect={handlePdfSelect} - /> - </Show> + <PdfSelector + pdfs={studyPdfs()} + selectedPdfId={selectedPdfId()} + onSelect={handlePdfSelect} + />packages/web/src/primitives/useProject/pdfs.js (2)
63-91: Consider adding tag validation.The function accepts any string as
tagwithout validation. While callers likely pass valid values, adding validation could prevent subtle bugs.🔎 Optional: Add tag validation
+const VALID_TAGS = ['primary', 'protocol', 'secondary']; + function addPdfToStudy(studyId, pdfInfo, tag = 'secondary') { const ydoc = getYDoc(); if (!ydoc || !isSynced()) return null; const studiesMap = ydoc.getMap('reviews'); const studyYMap = studiesMap.get(studyId); if (!studyYMap) return null; + const validatedTag = VALID_TAGS.includes(tag) ? tag : 'secondary'; const pdfsMap = getPdfsMap(studyId, true); // If setting as primary or protocol, clear existing tag first - if (tag === 'primary' || tag === 'protocol') { - clearTag(studyId, tag); + if (validatedTag === 'primary' || validatedTag === 'protocol') { + clearTag(studyId, validatedTag); }
137-159: Minor redundancy in ydoc checks.The
updatePdfTagfunction performsydoc/isSynced()checks that are already done insidegetPdfsMap. This is safe but slightly redundant.🔎 Optional: Simplify by reusing getPdfsMap checks
function updatePdfTag(studyId, pdfId, tag) { - const ydoc = getYDoc(); - if (!ydoc || !isSynced()) return; - const pdfsMap = getPdfsMap(studyId); if (!pdfsMap) return; const pdfYMap = pdfsMap.get(pdfId); if (!pdfYMap) return; // If setting as primary or protocol, clear existing tag first if (tag === 'primary' || tag === 'protocol') { clearTag(studyId, tag); } pdfYMap.set('tag', tag); + const ydoc = getYDoc(); const studiesMap = ydoc.getMap('reviews'); const studyYMap = studiesMap.get(studyId); if (studyYMap) { studyYMap.set('updatedAt', Date.now()); } }packages/web/src/primitives/useProjectPdfHandlers.js (3)
48-77: Consider caching downloaded PDFs.When fetching from the server (line 60), the downloaded data isn't cached for future use. This differs from
handleUploadPdfwhich caches uploaded files. Consider caching to improve subsequent access performance.🔎 Optional: Cache downloaded PDFs
if (!data) { // Fetch from server data = await downloadPdf(projectId, studyId, pdf.fileName); + // Cache for future access + cachePdf(projectId, studyId, pdf.fileName, data).catch(err => + console.warn('Failed to cache downloaded PDF:', err), + ); }
149-172: Minor DRY opportunity.The
effectiveTagdetermination logic (lines 155-158) is duplicated fromhandleUploadPdf(lines 88-91). Consider extracting to a helper if this pattern grows.The duplication is minor (4 lines) and may be acceptable given the localized scope.
181-184: Redundant arrow function wrappers.The convenience methods are already functions from
projectActions, so wrapping them in additional arrow functions is unnecessary.🔎 Simplify convenience method exports
return { handleViewPdf, handleDownloadPdf, handleUploadPdf, handleDeletePdf, handleTagChange, handleGoogleDriveImportSuccess, // Convenience methods - setPdfAsPrimary: (studyId, pdfId) => setPdfAsPrimary(studyId, pdfId), - setPdfAsProtocol: (studyId, pdfId) => setPdfAsProtocol(studyId, pdfId), + setPdfAsPrimary, + setPdfAsProtocol, };
📜 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 (17)
.github/copilot-instructions.md(1 hunks)docs/plans/all-studies-tab-redesign.plan.md(1 hunks)docs/plans/multiple-pdfs-per-study.plan.md(1 hunks)packages/web/package.json(1 hunks)packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(6 hunks)packages/web/src/components/checklist-ui/ChecklistsDashboard.jsx(0 hunks)packages/web/src/components/checklist-ui/pdf/PdfList.jsx(1 hunks)packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx(1 hunks)packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx(1 hunks)packages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsx(1 hunks)packages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsx(1 hunks)packages/web/src/components/checklist-ui/pdf/index.js(1 hunks)packages/web/src/primitives/useProject/index.js(1 hunks)packages/web/src/primitives/useProject/pdfs.js(3 hunks)packages/web/src/primitives/useProject/sync.js(1 hunks)packages/web/src/primitives/useProjectPdfHandlers.js(2 hunks)packages/web/src/stores/projectStore.js(2 hunks)
💤 Files with no reviewable changes (1)
- packages/web/src/components/checklist-ui/ChecklistsDashboard.jsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. Extract sub-modules into a folder (e.g.,ComponentName/withindex.jsxand helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsxpackages/web/src/stores/projectStore.jspackages/web/src/primitives/useProject/index.jspackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/checklist-ui/pdf/index.jspackages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsxpackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/primitives/useProject/pdfs.jspackages/web/src/components/checklist-ui/pdf/PdfSelector.jsxpackages/web/src/components/checklist-ui/pdf/PdfList.jsxpackages/web/src/primitives/useProject/sync.js
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Keep files small, focused, and modular. If a file exceeds a high number of lines, extract sub-modules into a folder with index.jsx and helper components, or move complex logic into separate utility files or primitives
Each file should handle one coherent responsibility
Group related components in subdirectories with an index.js barrel export
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
Import stores directly where needed instead of passing values through multiple components
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 (when scoped to a feature)
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 (e.g., () => props.name) to ensure they are always up-to-date
Use createMemo for derived values based on props or state to ensure reactive updates
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{js,jsx,ts,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
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 ...
Files:
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsxpackages/web/src/stores/projectStore.jspackages/web/src/primitives/useProject/index.jspackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/checklist-ui/pdf/index.jspackages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsxpackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/primitives/useProject/pdfs.jspackages/web/src/components/checklist-ui/pdf/PdfSelector.jsxpackages/web/src/components/checklist-ui/pdf/PdfList.jsxpackages/web/src/primitives/useProject/sync.js
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
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
Files:
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsxpackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/checklist-ui/pdf/index.jspackages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsxpackages/web/src/components/checklist-ui/pdf/PdfSelector.jsxpackages/web/src/components/checklist-ui/pdf/PdfList.jsx
**/*.{js,jsx,ts,tsx,json,yaml,yml,toml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using config files rather than hardcoding values
Files:
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsxpackages/web/src/stores/projectStore.jspackages/web/src/primitives/useProject/index.jspackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/components/checklist-ui/pdf/index.jspackages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsxpackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/primitives/useProject/pdfs.jspackages/web/src/components/checklist-ui/pdf/PdfSelector.jsxpackages/web/package.jsonpackages/web/src/components/checklist-ui/pdf/PdfList.jsxpackages/web/src/primitives/useProject/sync.js
**/{index,index.js,index.jsx,index.ts,index.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/primitives/useProject/index.jspackages/web/src/components/checklist-ui/pdf/index.js
docs/plans/**/*.{md,txt}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place plans in the docs/plans/ directory
Files:
docs/plans/multiple-pdfs-per-study.plan.mddocs/plans/all-studies-tab-redesign.plan.md
🧠 Learnings (9)
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: This project is split into multiple packages under the packages/ directory: landing/marketing site, main app (web), and backend services (workers). Each package may have its own dependencies and configurations
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: This project uses Cloudflare Workers only for backend services and frontend deployments. Cloudflare Pages is not used
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : For UI icons, use the `solid-icons` library or SVGs only. Do not use emojis
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Cloudflare Pages is not used in this project; only Cloudflare Workers is used for backend services and frontend deployments
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Do not use emojis in code, comments, or documentation
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/workers/migrations/*.sql : All migrations should go in a single file: packages/workers/migrations/0001_init.sql. Do NOT create separate migration files. Edit the existing 0001_init.sql file directly when adding new tables or schema changes
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : Use Zag.js for UI components and design system. Zag components exist in `packages/web/src/components/zag/*` and should be reused
Applied to files:
packages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsx
📚 Learning: 2025-12-16T18:27:52.769Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.769Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Group related components in subdirectories with an index.js barrel export
Applied to files:
packages/web/src/components/checklist-ui/pdf/index.js
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to **/{index,index.js,index.jsx,index.ts,index.tsx} : Group related components in subdirectories with an `index.js` barrel export
Applied to files:
packages/web/src/components/checklist-ui/pdf/index.js
🧬 Code graph analysis (6)
packages/web/src/components/checklist-ui/pdf/PdfTagSelect.jsx (1)
packages/ui/src/zag/Select.jsx (1)
Select(22-152)
packages/web/src/stores/projectStore.js (2)
packages/web/src/primitives/useProject/sync.js (1)
study(59-88)packages/workers/src/routes/pdfs.js (1)
pdfs(76-81)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (2)
packages/web/src/api/pdf-api.js (2)
uploadPdf(40-70)deletePdf(113-127)packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (1)
PdfSelector(15-99)
packages/web/src/primitives/useProjectPdfHandlers.js (3)
packages/web/src/primitives/useProject/index.js (1)
studies(43-43)packages/web/src/primitives/pdfCache.js (3)
getCachedPdf(82-102)cachePdf(111-138)removeCachedPdf(146-163)packages/web/src/api/pdf-api.js (3)
downloadPdf(79-93)uploadPdf(40-70)deletePdf(113-127)
packages/web/src/components/checklist-ui/pdf/PdfList.jsx (2)
packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (1)
sortedPdfs(25-34)packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (1)
PdfListItem(18-184)
packages/web/src/primitives/useProject/sync.js (2)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
pdfData(20-20)packages/web/src/components/checklist-ui/compare/ReconciliationWrapper.jsx (1)
pdfData(44-44)
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~11-~11: Use a hyphen to join words.
Context: ...not worry about migrations either client side or backend unless specifically inst...
(QB_NEW_EN_HYPHEN)
docs/plans/all-studies-tab-redesign.plan.md
[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...k actions (select multiple studies)? 3. Should PDF upload have a tag selector in the f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~184-~184: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e a tag selector in the file picker? 4. Should we show a PDF preview thumbnail?
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/plans/multiple-pdfs-per-study.plan.md
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/plans/all-studies-tab-redesign.plan.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (29)
packages/web/src/primitives/useProject/sync.js (1)
111-122: LGTM - Good backward compatibility handling.The updated PDF iteration logic properly handles both the new id-based structure and legacy fileName-based structure with sensible fallbacks. The default tag value of 'secondary' aligns well with the tagging system introduced across the PR.
packages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsx (1)
14-35: LGTM - Clean presentation component.The component correctly follows SolidJS best practices:
- Props are accessed directly without destructuring
- Uses
Showfor conditional rendering- Uses
solid-iconslibrary as per guidelines- Lean and focused on presentation only
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (1)
91-184: LGTM - Well-structured component with good UX patterns.The component demonstrates several good practices:
- Uses Zag.js components (Menu, Dialog) as per guidelines
- Uses
solid-iconslibrary for icons (no emojis)- Props accessed directly without destructuring
- Local state managed with
createSignal- Delete confirmation dialog provides good UX
- Conditional menu items with clear labeling for tag replacements
packages/web/src/stores/projectStore.js (2)
224-265: LGTM - Well-designed PDF selectors with backward compatibility.The new PDF selectors are well-structured:
- Consistent with existing store patterns
- Clear naming conventions
getSecondaryPdfscorrectly handles backward compatibility by including PDFs without tags- Proper use of optional chaining and safe defaults
- Each selector has a focused, single responsibility
483-489: LGTM - Proper export of new selectors.The new PDF selectors are correctly exported and follow the existing organization pattern in the store.
packages/web/src/primitives/useProject/index.js (1)
191-194: LGTM - Consistent integration of new PDF operations.The new PDF operations are properly integrated following established patterns:
- Consistent delegation to
pdfOpswith optional chaining- Spread args pattern matches existing operations
- Clear naming that indicates their purpose
removePdfByFileNameprovides useful backward compatibilitypackages/web/package.json (1)
22-22: Verify that the nanoid package version is current and secure.nanoid version 5.1.6 is the latest stable release, and Snyk confirms that 5.1.6 is the latest non-vulnerable version. The package demonstrates a positive release cadence with community engagement. No security advisories apply to this version.
docs/plans/multiple-pdfs-per-study.plan.md (1)
1-353: LGTM - Well-structured implementation plan.The plan is comprehensive with clear phases, file mappings, and implementation progress tracking. It follows the guideline to place plans in
docs/plans/.Minor: The static analysis tool flagged three fenced code blocks (lines 26, 53, 182) that could specify a language (e.g.,
yamlortextfor the Y.js structure diagrams,textfor the file structure). This is optional for documentation.packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (1)
15-98: LGTM - Clean, focused component.The component follows SolidJS best practices:
- Props accessed directly without destructuring
createMemoused appropriately for derived values- Uses
solid-iconslibrary as per guidelines- Reasonable prop count (3 props)
packages/web/src/components/checklist-ui/pdf/PdfList.jsx (2)
81-94: LGTM - CorrecthasPrimary/hasProtocollogic.The conditional logic
hasPrimary={hasPrimary() && pdf.tag !== 'primary'}correctly conveys toPdfListItemwhether setting this PDF as primary would replace an existing primary. Same for protocol.
26-36: Good secondary sort byuploadedAt.The additional descending sort by
uploadedAtwithin tag groups provides stable, user-friendly ordering. Note: the tag-based sorting logic is also inPdfSelector.jsx- consider the shared utility suggestion from earlier.docs/plans/all-studies-tab-redesign.plan.md (2)
1-184: Well-organized redesign plan.The plan clearly outlines the component structure, implementation steps, and UI/UX considerations. The expandable card pattern and modal separation are sensible architectural choices.
17-39: Emojis in documentation violate coding guidelines.Per the coding guidelines: "Do not use emojis in code, comments, or documentation." Lines 30, 33, and 36 use emojis (📄, ⭐, 📋) in the ASCII diagram. Consider using text labels or icon descriptions instead.
🔎 Suggested fix
-│ │ 📄 smith-2024-main.pdf ⭐ Primary [View] [⋮] │ │ +│ │ [doc] smith-2024-main.pdf [*] Primary [View] [⋮] │ │ -│ │ 📄 smith-2024-protocol.pdf 📋 Protocol [View] [⋮] │ │ +│ │ [doc] smith-2024-protocol.pdf [P] Protocol [View] [⋮] │ │ -│ │ 📄 supplementary.pdf [View] [⋮] │ │ +│ │ [doc] supplementary.pdf [View] [⋮] │ │⛔ Skipped due to learnings
Learnt from: CR Repo: InfinityBowman/corates PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-16T18:28:31.657Z Learning: Applies to **/*.{js,jsx,ts,tsx} : Do not use emojis in code, comments, or documentationLearnt from: CR Repo: InfinityBowman/corates PR: 0 File: .cursorrules:0-0 Timestamp: 2025-12-16T18:27:52.769Z Learning: Applies to **/*.{js,jsx,ts,tsx} : Do not use emojis in code, comments, or documentationpackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (3)
173-192: LGTM - Clean deletion flow with proper state reset.The deletion handler correctly:
- Deletes from cloud storage
- Removes from Y.js metadata
- Clears local cache
- Resets selection state
The auto-select effect will then pick the new default PDF automatically.
15-361: LGTM - Well-integrated multi-PDF support.The component cleanly integrates multi-PDF functionality:
- Proper use of
createMemofor derived PDF state- Appropriate loading/caching flow
- Correct tag assignment on upload (primary for first, secondary for subsequent)
- Clean integration with
PdfSelector
77-83: Remove the infinite loop concern—the guard condition prevents re-triggering.The effect on lines 77-83 is safe: once
selectedPdfIdis set, the condition!selectedPdfId()evaluates to false on subsequent runs, preventing the setter from being called again. There is no infinite loop risk. IfdefaultPdf()changes later, the effect reruns but the guard still prevents unexpected re-selection.If you prefer explicit dependency tracking for code clarity, consider importing and using
on()from solid-js to track onlydefaultPdf(), avoiding reactivity toselectedPdfId():createEffect(on(defaultPdf, (pdf) => { if (pdf && !selectedPdfId()) { setSelectedPdfId(pdf.id); } }));This is optional for clarity only.
Likely an incorrect or invalid review comment.
packages/web/src/primitives/useProject/pdfs.js (7)
1-11: LGTM!Good documentation of PDF tags and proper import of dependencies.
21-39: LGTM!The
getPdfsMaphelper properly handles all edge cases with appropriate null checks and conditional creation.
41-54: LGTM!The
clearTagfunction correctly enforces the single primary/protocol constraint by resetting conflicting tags to 'secondary'.
93-112: LGTM!The
removePdfFromStudyfunction correctly handles removal by pdfId with appropriate null checks and timestamp updates.
114-129: LGTM!Good backward compatibility method. The early return after first match is appropriate behavior for filename-based removal.
161-177: LGTM!Clean convenience methods that provide a clear API for common operations.
179-186: LGTM!Complete export list matching the documented public API.
packages/web/src/primitives/useProjectPdfHandlers.js (5)
1-27: LGTM!Clear documentation of supported tags and proper imports. The destructuring of
projectActionsis acceptable here since it's a plain object parameter, not SolidJS props.
29-46: LGTM!Good cache-first approach for viewing PDFs. The 60-second delay before revoking the blob URL gives adequate time for the browser tab to load the PDF.
86-117: LGTM!Good implementation with auto-primary tagging for the first PDF. The optional chaining on
study?.pdfs?.lengthsafely handles edge cases, and caching the upload improves subsequent access.
119-137: LGTM!Correct order of operations: server delete first, then local state update. This ensures consistency if the server call fails.
139-147: LGTM!Clean wrapper that delegates to the underlying operation.
packages/web/src/components/checklist-ui/pdf/index.js (1)
1-13: LGTM!Clean barrel export following the coding guidelines. This properly groups related PDF components with an
index.jsexport for simplified imports. Based on learnings, this aligns with the project's convention for organizing related components.
| const pdfId = addPdfToStudy( | ||
| params.studyId, | ||
| { | ||
| key: result.key, | ||
| fileName: result.fileName, | ||
| size: result.size, | ||
| uploadedBy: user()?.id, | ||
| uploadedAt: Date.now(), | ||
| }, | ||
| tag, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for addPdfToStudy function definition
rg "addPdfToStudy" --type js --type jsx --type ts --type tsx -A 15Repository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Search for addPdfToStudy function definition across all files
rg "addPdfToStudy" -A 15 --type-add 'web:*.{js,jsx,ts,tsx}'Repository: InfinityBowman/corates
Length of output: 22704
🏁 Script executed:
#!/bin/bash
# Get the complete addPdfToStudy function implementation
sed -n '/function addPdfToStudy/,/^ }/p' packages/web/src/primitives/useProject/pdfs.js | head -50Repository: InfinityBowman/corates
Length of output: 991
🏁 Script executed:
#!/bin/bash
# Check how pdfId is used after assignment in ChecklistYjsWrapper.jsx
sed -n '150,175p' packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxRepository: InfinityBowman/corates
Length of output: 781
Add null validation for addPdfToStudy return value before setting state.
The function addPdfToStudy returns the PDF id on success, but can return null if the document is not synced or the study doesn't exist. Line 163 calls setSelectedPdfId(pdfId) without validating that pdfId is not null, which could silently set the state to null and cause the UI to behave unexpectedly. Add a check to ensure pdfId is valid before updating state:
if (pdfId) {
setSelectedPdfId(pdfId);
}🤖 Prompt for AI Agents
In packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx around lines
150 to 160, the code assigns the return value of addPdfToStudy to pdfId and
immediately calls setSelectedPdfId(pdfId) without checking for null;
addPdfToStudy can return null when the document isn't synced or the study is
missing, so validate the return before updating state by checking that pdfId is
truthy (e.g., if (pdfId) { setSelectedPdfId(pdfId); }) and only set state when a
valid id is returned.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/ui/src/zag/Menu.jsx (1)
99-99: Consider extracting button styles to a config.The button has hardcoded Tailwind classes for padding, colors, and hover states. As per coding guidelines, consider extracting these to a configuration object if this button style needs to be reused or customized across different Menu instances.
Example refactor to make button styling configurable:
+const defaultButtonClass = 'inline-flex items-center gap-1 p-1.5 text-gray-400 hover:text-gray-600 hover:bg-gray-100 rounded transition-colors'; + export function Menu(props) { const [local, machineProps] = splitProps(props, [ 'trigger', 'items', 'inDialog', 'hideIndicator', 'class', + 'buttonClass', ]);Then use:
class={local.buttonClass || defaultButtonClass}packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (2)
159-182: Remove redundant padding wrapper.The
Dialogcomponent already wraps its children in a<div class='p-4'>. The additional<div class='p-4'>here creates double padding. Consider removing the wrapper or using a different spacing approach.🔎 Suggested fix:
{/* Delete confirmation dialog */} <Dialog open={showDeleteConfirm()} onOpenChange={setShowDeleteConfirm} title='Delete PDF'> - <div class='p-4'> - <p class='text-gray-600 mb-4'> + <div> + <p class='text-gray-600 mb-6'> Are you sure you want to delete <strong>{props.pdf.fileName}</strong>? This action cannot be undone. </p> <div class='flex justify-end gap-3'>
30-40: Consider extracting utility functions.
formatFileSizeandformatDateare common formatting utilities that could be reused across components. Consider moving them to a shared utility file if similar functions exist elsewhere in the codebase.packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (2)
13-27: Consider reducing the number of props via context or a primitive.This component accepts 14 props, which exceeds the guideline of 1-5 props for local configuration. The component appears to be a coordination layer that passes many callbacks through to child components.
According to the coding guidelines: "If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context."
Consider refactoring to:
- Move PDF-related handlers (onViewPdf, onDownloadPdf, onUploadPdf, onDeletePdf, onTagChange, onOpenGoogleDrive) into a context or primitive
- Move study-related handlers (onEditMetadata, onAssignReviewers, onDeleteStudy) into a context or primitive
- Use the existing ProjectContext or create a StudyCardContext to provide these handlers
This would reduce prop drilling and make the component more maintainable.
Based on coding guidelines requiring components to receive at most 1-5 props for local configuration and to avoid prop drilling of application state.
36-40: Simplify the Collapsible onOpenChange handler.The condition
if (open !== expanded())before callingonToggleExpandedmay be redundant. The Collapsible component's onOpenChange should only fire when the state actually changes, making this check unnecessary.🔎 Simplify to:
<Collapsible open={expanded()} - onOpenChange={open => { - if (open !== expanded()) { - props.onToggleExpanded?.(); - } - }} + onOpenChange={() => props.onToggleExpanded?.()} trigger={api => (packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (1)
15-23: Consider consolidating PDF handlers into a single prop.This component accepts 8 props, which exceeds the guideline of 1-5 props. While this is a leaf component (less critical than a coordinator component), consider consolidating the PDF-related callbacks into a single handlers object or using a context/primitive.
For example:
// Instead of multiple callback props: props.onViewPdf, props.onDownloadPdf, props.onUploadPdf, etc. // Use a single handlers object: props.pdfHandlers = { onView, onDownload, onUpload, onDelete, onTagChange, onOpenGoogleDrive }This would reduce the prop count and align better with the coding guidelines.
Based on coding guidelines requiring components to receive at most 1-5 props.
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (1)
17-24: Consider reducing props via context.This component accepts 8 props, exceeding the guideline of 1-5 props. Similar to StudyCard and StudyPdfSection, consider consolidating the action callbacks (onEditMetadata, onAssignReviewers, onDelete) into a single handlers object or providing them via context.
Based on coding guidelines requiring components to receive at most 1-5 props.
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (2)
107-125: Inconsistent optional chaining on PDF handlers.The PDF handlers use optional chaining inconsistently:
- Lines 108, 116: No optional chaining (
handlers.pdfHandlers.handleViewPdf,handlers.pdfHandlers.handleUploadPdf)- Lines 112, 120, 124: With optional chaining (
handlers.pdfHandlers.handleDownloadPdf?.,handlers.pdfHandlers.handleDeletePdf?.,handlers.pdfHandlers.handleTagChange?.)If all these handlers are expected to exist in
pdfHandlers, use consistent non-optional access. If some are optional, add comments explaining why.🔎 Make consistent by removing optional chaining (if all handlers are guaranteed):
const handleDownloadPdf = (studyId, pdf) => { - handlers.pdfHandlers.handleDownloadPdf?.(studyId, pdf); + handlers.pdfHandlers.handleDownloadPdf(studyId, pdf); }; const handleUploadPdf = async (studyId, file) => { await handlers.pdfHandlers.handleUploadPdf(studyId, file); }; const handleDeletePdf = (studyId, pdf) => { - handlers.pdfHandlers.handleDeletePdf?.(studyId, pdf); + handlers.pdfHandlers.handleDeletePdf(studyId, pdf); }; const handleTagChange = (studyId, pdfId, newTag) => { - handlers.pdfHandlers.handleTagChange?.(studyId, pdfId, newTag); + handlers.pdfHandlers.handleTagChange(studyId, pdfId, newTag); };
183-197: StudyCard receives many props - consider context pattern.Similar to the comment on StudyCard.jsx itself, the StudyCard component receives 13+ props here. While this works, it creates tight coupling between AllStudiesTab and StudyCard.
Consider creating a
StudyCardContextthat provides the handlers, reducing the number of props passed at each level and making the component more maintainable. This would align with the coding guideline to avoid passing shared state through props.Based on coding guidelines requiring use of stores/context instead of passing shared state through multiple component props.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/ui/src/zag/Menu.jsx(3 hunks)packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx(1 hunks)packages/web/src/components/project-ui/ProjectView.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsx(6 hunks)packages/web/src/components/project-ui/all-studies/index.js(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/index.js(1 hunks)packages/web/src/components/project-ui/tabs/AllStudiesTab.jsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/web/src/components/project-ui/tabs/AllStudiesTab.jsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. Extract sub-modules into a folder (e.g.,ComponentName/withindex.jsxand helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.jspackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/ui/src/zag/Menu.jsxpackages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Keep files small, focused, and modular. If a file exceeds a high number of lines, extract sub-modules into a folder with index.jsx and helper components, or move complex logic into separate utility files or primitives
Each file should handle one coherent responsibility
Group related components in subdirectories with an index.js barrel export
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
Import stores directly where needed instead of passing values through multiple components
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 (when scoped to a feature)
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 (e.g., () => props.name) to ensure they are always up-to-date
Use createMemo for derived values based on props or state to ensure reactive updates
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{js,jsx,ts,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
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 ...
Files:
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.jspackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
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
Files:
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.jspackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx
**/*.{js,jsx,ts,tsx,json,yaml,yml,toml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using config files rather than hardcoding values
Files:
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.jspackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/components/project-ui/ProjectView.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/ui/src/zag/Menu.jsxpackages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx
**/{index,index.js,index.jsx,index.ts,index.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.js
🧠 Learnings (4)
📚 Learning: 2025-12-16T18:27:52.769Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.769Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Group related components in subdirectories with an index.js barrel export
Applied to files:
packages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.js
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to **/{index,index.js,index.jsx,index.ts,index.tsx} : Group related components in subdirectories with an `index.js` barrel export
Applied to files:
packages/web/src/components/project-ui/all-studies/index.jspackages/web/src/components/project-ui/all-studies/study-card/index.js
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use Solid's createStore for complex state or state objects for better performance and reactivity
Applied to files:
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Import stores directly where needed instead of passing values through multiple components
Applied to files:
packages/web/src/components/project-ui/ProjectView.jsx
🧬 Code graph analysis (7)
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (4)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (1)
study(29-29)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (1)
study(26-26)packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (2)
handleDelete(86-89)PdfListItem(18-185)packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (1)
handleTagChange(123-125)
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (3)
packages/web/src/components/checklist-ui/pdf/PdfTagBadge.jsx (1)
PdfTagBadge(14-35)packages/ui/src/zag/Menu.jsx (1)
Menu(31-125)packages/ui/src/zag/Dialog.jsx (1)
Dialog(18-92)
packages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsx (3)
packages/web/src/components/project-ui/ReviewerAssignment.jsx (1)
getMemberName(44-48)packages/ui/src/zag/Dialog.jsx (1)
Dialog(18-92)packages/ui/src/zag/Select.jsx (1)
Select(22-152)
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (1)
packages/web/src/lib/formStatePersistence.js (5)
getRestoreParamsFromUrl(204-214)getFormState(90-132)clearFormState(140-165)clearRestoreParamsFromUrl(219-225)saveFormState(49-82)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (4)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (1)
study(29-29)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (1)
study(28-28)packages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsx (1)
name(24-24)packages/ui/src/zag/Menu.jsx (1)
Menu(31-125)
packages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsx (2)
packages/ui/src/zag/Dialog.jsx (1)
Dialog(18-92)packages/ui/src/zag/Collapsible.jsx (1)
Collapsible(15-70)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (4)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (2)
study(26-26)StudyCardHeader(17-139)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
study(28-28)StudyPdfSection(15-170)packages/ui/src/zag/Collapsible.jsx (1)
Collapsible(15-70)packages/ui/src/zag/Menu.jsx (1)
api(48-48)
⏰ 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 (21)
packages/ui/src/zag/Menu.jsx (3)
19-19: Documentation looks good.The
hideIndicatorprop is documented clearly and follows the existing documentation pattern.
32-38: Props destructuring is correct.The
hideIndicatorprop is properly included in the local props array, which is appropriate since it's used directly in the component's JSX.
97-117: Conditional indicator rendering implemented correctly; styling is appropriate and follows best practices.The
hideIndicatorfeature is cleanly implemented using SolidJS'sShowcomponent with thewhenprop, which properly hides the chevron when the prop is true. The button styling uses Tailwind'sdata-[state=open]modifier for state-based styling, which is the recommended approach for Zag components and aligns with current Tailwind best practices.Verified that existing Menu usages (PdfListItem.jsx and PdfSelector.jsx) work correctly with the new styling—the button styles are applied consistently across all instances without regressions. The SVG chevron implementation complies with the coding guidelines (no emoji usage).
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (1)
91-109: LGTM!The component structure is clean with proper separation of file info, tag badge, and action buttons. Props are accessed reactively without destructuring, which is correct for SolidJS.
packages/web/src/components/project-ui/all-studies/AssignReviewersModal.jsx (3)
25-39: LGTM - Good store usage pattern.The component correctly imports
projectStoredirectly and accesses members viaprojectStore.getMembers()rather than prop-drilling. ThememberItemsmemo properly includes an "Unassigned" option and handles member name fallbacks. Based on learnings, this follows the preferred pattern of importing stores directly.
42-48: LGTM!The form reset effect correctly tracks both
props.studyandprops.openas dependencies, ensuring the form is reinitialized when the modal opens with a different study.
85-104: Good UX: Cross-validation prevents duplicate reviewer selection.The
disabledValuesprop on each Select prevents selecting the same person as both Reviewer 1 and Reviewer 2, which is appropriate validation for this use case.packages/web/src/components/project-ui/all-studies/EditStudyMetadataModal.jsx (3)
52-63: LGTM - Robust publication year validation.The validation handles edge cases well: trims whitespace, checks for finite numbers, truncates decimals, and enforces a reasonable year range (1900-2100). This prevents invalid data from being saved.
65-73: Minor: Consider preserving empty string vs undefined semantics.Currently, empty trimmed strings become
undefined. If the backend distinguishes between "field was cleared" (empty string) and "field was never set" (undefined/null), this could cause issues. Verify this aligns with backend expectations.
85-91: LGTM!Clean Dialog setup with appropriate props forwarding. The
size='lg'correctly maps tomax-w-lgin the Dialog component.packages/web/src/components/project-ui/all-studies/study-card/index.js (1)
1-4: LGTM - Clean barrel export.This follows the coding guidelines for grouping related components in subdirectories with an
index.jsbarrel export. The named exports provide a clear public API for the study-card module.packages/web/src/components/project-ui/ProjectView.jsx (1)
31-31: LGTM - Module reorganization import update.The import path correctly reflects the restructuring of
AllStudiesTabfromtabs/to theall-studies/directory, with no remaining references to the old import path in the codebase.packages/web/src/components/project-ui/all-studies/index.js (1)
1-5: LGTM! Clean barrel export structure.This index file follows the coding guideline to "Group related components in subdirectories with an index.js barrel export." The re-export pattern is clean and appropriate.
Based on learnings: applies to **/{index,index.js,index.jsx,index.ts,index.tsx} - Group related components in subdirectories with an
index.jsbarrel export.packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (3)
25-62: LGTM! Solid state management and error handling.The upload handling is well-implemented:
- Proper use of
createSignalfor the uploading state- MIME type validation before upload
- Comprehensive error handling with user-friendly toast notifications
- Cleanup in the finally block to reset the file input
32-43: LGTM! Excellent use of createMemo for derived state.The computed values (
sortedPdfs,hasPrimary,hasProtocol) properly usecreateMemoto ensure reactive updates. The sort logic correctly prioritizes PDFs by tag (primary → protocol → secondary) and then by upload timestamp.As per coding guidelines: "Use createMemo for derived values based on props or state to ensure reactive updates."
160-161: Prop logic is correct and well-implemented.The expressions
hasPrimary() && pdf.tag !== 'primary'andhasProtocol() && pdf.tag !== 'protocol'correctly pass to PdfListItem a boolean indicating whether another PDF already occupies that tag. PdfListItem uses these props to display appropriate "(replaces current)" messages, which aligns with the documented behavior. No issues found.packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (2)
29-55: LGTM! Clean helper functions with proper fallbacks.The helper functions (
assignedReviewers,hasReviewers,displayName,citationLine) are well-structured with appropriate fallback logic. The displayName function provides a good user experience by falling back to "Author (Year)" or "Untitled Study" when the study name is not available.
100-102: Verify chevron rotation synchronization.The chevron uses
props.expandedto determine rotation state. Ensure this stays in sync with the Collapsible's internal state. From the StudyCard implementation,props.expandedreceivesapi.openfrom the Collapsible, which should keep it synchronized.However, there's a potential race condition: if
onToggleis called (line 96) andprops.expandedhasn't updated yet, the chevron might show the wrong state momentarily.Consider testing the expand/collapse interaction to confirm smooth visual transitions without flickering.
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (3)
22-23: LGTM! Proper use of context to avoid prop drilling.Using
useProjectContextto access shared state (projectId, handlers, getAssigneeName) is the correct approach and aligns with the coding guideline: "Do NOT prop-drill application state. Import stores directly where needed instead of passing values through multiple components."Based on coding guidelines requiring use of external stores or context rather than prop drilling.
39-53: LGTM! Solid OAuth redirect state restoration.The mount-time form state restoration is well-implemented:
- Checks for matching restore params
- Properly handles errors
- Cleans up both IndexedDB state and URL params after restoration
- Uses try-catch for robust error handling
This enables a smooth user experience when returning from OAuth flows.
31-31: LGTM! Appropriate use of Set for expanded studies.Using a
Setto track expanded study IDs is an efficient choice for this use case, allowing O(1) lookups and easy add/delete operations.As per coding guidelines: "Use Solid's createStore for complex state or state objects for better performance and reactivity." In this case, a Set within a signal is simpler and appropriate for tracking a collection of IDs.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/zag/Combobox.jsx (1)
52-69: Fix property override:openOnClickprop is being ignored.The hardcoded
openOnClick: trueon line 55 overrides any user-providedopenOnClickvalue frommachinePropsbecause it comes after the spread. According to the documentation (line 25),openOnClickshould be a configurable prop.However, the getter pattern for
collection(lines 56-58) is correctly implemented to ensure reactive access.🔎 Apply this diff to respect the user's `openOnClick` prop:
const service = useMachine(combobox.machine, () => ({ - ...machineProps, id: createUniqueId(), openOnClick: true, + ...machineProps, get collection() { return collection(); },This places defaults first so
machinePropscan override them.
♻️ Duplicate comments (1)
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (1)
147-155: Menuplacementprop issue already flagged.This is the same issue identified in the past review comment - the Menu component's placement prop is not functional due to hardcoded positioning.
🧹 Nitpick comments (12)
packages/ui/src/zag/RadioGroup.jsx (1)
30-30: Prefer reading orientation from the machine API for consistency.The
isVerticalfunction reads directly fromlocal.orientation(props), while the relevant code snippet fromToggleGroup.jsxshows the pattern of reading fromapi().orientation(machine state). Since orientation is passed to the machine on line 25, the UI should read it back from the machine API to maintain a single source of truth, consistent with the Zag.js pattern used in other components.🔎 Apply this diff for consistency with ToggleGroup pattern:
- const isVertical = () => (local.orientation || 'vertical') === 'vertical'; + const isVertical = () => api().orientation === 'vertical';packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (2)
17-24: Consider reducing prop count or moving shared state to context/store.This component receives 6 props, which exceeds the recommended 1-5 props per coding guidelines. Consider:
- Moving
getAssigneeNameto context (it's a project-level utility)- Grouping callbacks into a single
actionsobject propBased on coding guidelines, components should receive at most 1-5 props for local configuration.
69-81: UsecreateMemofor derived values to avoid redundant computation.
citationLine()is invoked twice in the JSX (lines 125-126), recomputing on each call. Wrap it withcreateMemofor reactive caching.Suggested fix
+import { createMemo } from 'solid-js'; ... - const citationLine = () => { + const citationLine = createMemo(() => { const pdf = primaryPdf(); const parts = []; const author = pdf?.firstAuthor || study().firstAuthor; const year = pdf?.publicationYear || study().publicationYear; const journal = pdf?.journal || study().journal; if (author) parts.push(author); if (year) parts.push(`(${year})`); if (journal) parts.push(`- ${journal}`); return parts.join(' '); - }; + });packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (2)
18-28: Prop count (9) significantly exceeds guideline limit.This component receives 9 props, well beyond the 1-5 prop limit. Consider refactoring:
- Extract
hasPrimary/hasProtocolinto a context or derive them from astudyPdfsprop- Group action callbacks (
onView,onDownload,onDelete,onTagChange,onEditMetadata) into a singleactionsobject- Pass
readOnlyvia context if it's consistent across the PDF listBased on coding guidelines and retrieved learnings.
31-41: Extract utility functions for reuse.
formatFileSizeandformatDateare generic helpers that could be reused across the codebase. Consider moving them to a shared utility file (e.g.,@lib/formatters.js).packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
15-25: Prop count (9) exceeds guideline limit.Similar to PdfListItem, this component receives 9 props. Consider:
- Passing handlers as a single
pdfActionsobject- Using context for
readOnlystate- Deriving study ID from a context rather than accessing
study().idrepeatedlyBased on coding guidelines.
46-51: Consider adding file extension check as fallback validation.MIME type detection can be unreliable across browsers. Adding a file extension check provides a more robust validation.
Suggested enhancement
const handleFileSelect = async e => { const file = e.target.files?.[0]; - if (!file || file.type !== 'application/pdf') { + const isPdf = file?.type === 'application/pdf' || file?.name?.toLowerCase().endsWith('.pdf'); + if (!file || !isPdf) { showToast.error('Invalid File', 'Please select a PDF file'); return; }packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (2)
34-40: Consider usingcreateStorefor grouped modal state.Multiple related signals for modal state could be consolidated using
createStorefor cleaner state management.Suggested approach
import { createStore } from 'solid-js/store'; const [modalState, setModalState] = createStore({ metadata: { open: false, study: null }, reviewers: { open: false, study: null }, pdfMetadata: { open: false, pdf: null, studyId: null }, }); // Usage: setModalState('metadata', { open: true, study: someStudy });Based on retrieved learnings: "Use Solid's createStore for complex state or state objects for better performance and reactivity."
206-221: StudyCard receives 15 props - consider refactoring to reduce coupling.This prop-drilling pattern violates the 1-5 prop guideline. Consider:
- Passing a single
handlersobject containing all callbacks- Using context for PDF and study handlers that are consistent across all cards
- Moving
getAssigneeNameto context (project-level utility)Suggested approach
// In AllStudiesTab or ProjectContext: const studyCardActions = { onToggleExpanded: toggleStudyExpanded, onEditMetadata: handleOpenMetadataModal, onAssignReviewers: handleOpenReviewersModal, onDeleteStudy: handlers.studyHandlers.handleDeleteStudy, pdf: { onView: handleViewPdf, onDownload: handleDownloadPdf, onUpload: handleUploadPdf, onDelete: handleDeletePdf, onTagChange: handleTagChange, onEditMetadata: handleOpenPdfMetadataModal, onOpenGoogleDrive: handleOpenGoogleDrive, }, }; // Then in JSX: <StudyCard study={study} expanded={isStudyExpanded(study.id)} actions={studyCardActions} />Based on coding guidelines.
packages/web/src/primitives/useProject/pdfs.js (1)
145-167: Consider validating tag values.The function accepts any string as
tagwithout validating it against the allowed values ('primary','protocol','secondary'). Invalid tags could be set, potentially causing issues in UI components that expect specific tag values.Suggested validation:
+const VALID_TAGS = ['primary', 'protocol', 'secondary']; + function updatePdfTag(studyId, pdfId, tag) { const ydoc = getYDoc(); if (!ydoc || !isSynced()) return; + + if (!VALID_TAGS.includes(tag)) { + console.warn(`Invalid PDF tag: ${tag}`); + return; + } const pdfsMap = getPdfsMap(studyId);packages/web/src/primitives/useProjectPdfHandlers.js (2)
57-83: Consider caching downloaded PDFs.When fetching from the server (line 66), the downloaded data isn't cached for future use. This means subsequent downloads of the same PDF will always hit the server.
handleUploadPdfcaches after upload, so similar caching here would be consistent.Suggested improvement:
if (!data) { // Fetch from server data = await downloadPdf(projectId, studyId, pdf.fileName); + // Cache for future use + cachePdf(projectId, studyId, pdf.fileName, data).catch(err => + console.warn('Failed to cache downloaded PDF:', err), + ); }
168-188: Consider returning pdfId for consistency.Unlike
handleUploadPdfwhich returns the newpdfId, this function doesn't return it. If callers need to reference the newly added PDF immediately after import, they'd have no way to identify it.Suggested change:
const handleGoogleDriveImportSuccess = (studyId, file, tag = 'secondary') => { if (!studyId || !file) return; // ... - addPdfToStudy( + return addPdfToStudy( studyId, // ... ); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/ui/src/zag/Accordion.jsx(2 hunks)packages/ui/src/zag/Clipboard.jsx(3 hunks)packages/ui/src/zag/Combobox.jsx(3 hunks)packages/ui/src/zag/Menu.jsx(4 hunks)packages/ui/src/zag/Progress.jsx(2 hunks)packages/ui/src/zag/RadioGroup.jsx(2 hunks)packages/ui/src/zag/ToggleGroup.jsx(3 hunks)packages/ui/src/zag/Tour.jsx(1 hunks)packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx(1 hunks)packages/web/src/primitives/useProject/index.js(1 hunks)packages/web/src/primitives/useProject/pdfs.js(3 hunks)packages/web/src/primitives/useProject/sync.js(1 hunks)packages/web/src/primitives/useProjectPdfHandlers.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx
- packages/web/src/primitives/useProject/sync.js
- packages/ui/src/zag/Menu.jsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. Extract sub-modules into a folder (e.g.,ComponentName/withindex.jsxand helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/ui/src/zag/Progress.jsxpackages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Clipboard.jsxpackages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/ui/src/zag/Combobox.jsxpackages/ui/src/zag/Tour.jsxpackages/web/src/primitives/useProject/index.jspackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/primitives/useProject/pdfs.jspackages/ui/src/zag/RadioGroup.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/ui/src/zag/ToggleGroup.jsx
**/*.{js,jsx,ts,tsx,json,yaml,yml,toml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using config files rather than hardcoding values
Files:
packages/ui/src/zag/Progress.jsxpackages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Clipboard.jsxpackages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/ui/src/zag/Combobox.jsxpackages/ui/src/zag/Tour.jsxpackages/web/src/primitives/useProject/index.jspackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/primitives/useProject/pdfs.jspackages/ui/src/zag/RadioGroup.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/ui/src/zag/ToggleGroup.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Keep files small, focused, and modular. If a file exceeds a high number of lines, extract sub-modules into a folder with index.jsx and helper components, or move complex logic into separate utility files or primitives
Each file should handle one coherent responsibility
Group related components in subdirectories with an index.js barrel export
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
Import stores directly where needed instead of passing values through multiple components
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 (when scoped to a feature)
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 (e.g., () => props.name) to ensure they are always up-to-date
Use createMemo for derived values based on props or state to ensure reactive updates
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{js,jsx,ts,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
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 ...
Files:
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/primitives/useProject/index.jspackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/primitives/useProject/pdfs.jspackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
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
Files:
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsxpackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx
**/{index,index.js,index.jsx,index.ts,index.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Group related components in subdirectories with an
index.jsbarrel export
Files:
packages/web/src/primitives/useProject/index.js
🧠 Learnings (9)
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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 (e.g., () => props.name) to ensure they are always up-to-date
Applied to files:
packages/ui/src/zag/Accordion.jsxpackages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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
Applied to files:
packages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : Use Zag.js for UI components and design system. Zag components exist in `packages/web/src/components/zag/*` and should be reused
Applied to files:
packages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/ToggleGroup.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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
Applied to files:
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context (when scoped to a feature)
Applied to files:
packages/web/src/components/checklist-ui/pdf/PdfListItem.jsxpackages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : When you have complex state or state objects, use Solid's `createStore` for better performance and reactivity
Applied to files:
packages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use Solid's createStore for complex state or state objects for better performance and reactivity
Applied to files:
packages/ui/src/zag/Tour.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Applied to files:
packages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Applied to files:
packages/ui/src/zag/Tour.jsx
🧬 Code graph analysis (9)
packages/ui/src/zag/Progress.jsx (13)
packages/ui/src/zag/Accordion.jsx (1)
service(22-26)packages/ui/src/zag/Clipboard.jsx (2)
service(23-27)service(90-94)packages/ui/src/zag/Menu.jsx (1)
service(41-46)packages/ui/src/zag/RadioGroup.jsx (1)
service(22-26)packages/ui/src/zag/ToggleGroup.jsx (1)
service(25-32)packages/ui/src/zag/Tour.jsx (1)
service(59-65)packages/ui/src/zag/Editable.jsx (1)
service(106-128)packages/ui/src/zag/Avatar.jsx (1)
service(11-14)packages/ui/src/zag/Collapsible.jsx (1)
service(22-30)packages/ui/src/zag/FloatingPanel.jsx (1)
service(39-58)packages/ui/src/zag/NumberInput.jsx (1)
service(48-48)packages/ui/src/zag/Dialog.jsx (2)
service(25-31)service(117-130)packages/ui/src/zag/Toast.jsx (1)
machineProps(22-26)
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx (1)
packages/ui/src/zag/Dialog.jsx (1)
Dialog(18-92)
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (5)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (1)
study(30-30)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (1)
study(25-25)packages/web/src/primitives/useProject/sync.js (1)
study(59-88)packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (2)
handleDelete(87-90)PdfListItem(18-198)packages/web/src/primitives/useProjectPdfHandlers.js (1)
handleTagChange(151-153)
packages/web/src/primitives/useProjectPdfHandlers.js (2)
packages/web/src/primitives/pdfCache.js (3)
getCachedPdf(82-102)cachePdf(111-138)removeCachedPdf(146-163)packages/web/src/api/pdf-api.js (3)
downloadPdf(79-93)uploadPdf(40-70)deletePdf(113-127)
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (8)
packages/web/src/primitives/useProject/index.js (1)
studies(43-43)packages/web/src/primitives/useProjectPdfHandlers.js (2)
studies(24-24)handleTagChange(151-153)packages/web/src/components/project-ui/ProjectView.jsx (1)
studies(51-51)packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (2)
study(30-30)StudyCard(13-69)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (1)
study(25-25)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
study(29-29)handleTagChange(81-83)packages/web/src/primitives/useProject/sync.js (1)
study(59-88)packages/web/src/components/project-ui/google-drive/GoogleDrivePickerModal.jsx (1)
GoogleDrivePickerModal(19-79)
packages/web/src/primitives/useProject/pdfs.js (2)
packages/web/src/primitives/useProject/index.js (2)
ydoc(29-29)getYDoc(48-48)packages/web/src/primitives/useProject/sync.js (1)
pdfsMap(109-109)
packages/ui/src/zag/RadioGroup.jsx (1)
packages/ui/src/zag/ToggleGroup.jsx (4)
local(23-23)service(25-32)api(34-34)isVertical(47-47)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (4)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (1)
study(30-30)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
study(29-29)pdfs(30-30)packages/web/src/primitives/useProject/sync.js (1)
study(59-88)packages/ui/src/zag/Menu.jsx (1)
Menu(31-125)
packages/ui/src/zag/ToggleGroup.jsx (4)
packages/ui/src/zag/Accordion.jsx (2)
service(22-26)api(28-28)packages/ui/src/zag/Clipboard.jsx (4)
service(23-27)service(90-94)api(29-29)api(96-96)packages/ui/src/zag/RadioGroup.jsx (3)
service(22-26)isVertical(30-30)api(28-28)packages/ui/src/zag/Dialog.jsx (3)
service(25-31)service(117-130)api(33-33)
⏰ 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 (22)
packages/ui/src/zag/RadioGroup.jsx (3)
3-3: LGTM: Import correctly updated.The addition of
splitPropsto the imports is necessary and properly used on line 20.
20-20: LGTM: splitProps correctly updated.Adding
'orientation'to the local props array is correct and consistent with the pattern of extracting props that need local handling before being passed to the machine.
22-26: LGTM: useMachine initialization follows established pattern.The migration to a functional initializer with
useMachineis consistent with the parallel changes in other UI components (ToggleGroup, etc.) and properly passes the orientation configuration to the machine with the correct default value of'vertical'.packages/ui/src/zag/Progress.jsx (1)
3-3: LGTM: Unused import removed.The removal of
mergePropsis correct since the refactored code no longer uses it.packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (1)
148-154: Menuplacementprop may not function as expected.The
placement='bottom-end'prop is passed, but based on a prior review ofpackages/ui/src/zag/Menu.jsx, the positioning is hardcoded to'bottom-start'. This prop will have no effect until the Menu component is fixed to properly extract and apply the placement value.packages/web/src/components/checklist-ui/pdf/PdfListItem.jsx (1)
171-195: LGTM - Delete confirmation dialog is well-implemented.Good UX pattern with clear confirmation messaging and properly disabled state during operations.
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx (1)
29-39: LGTM - Form reset effect is correctly implemented.The effect properly resets form fields when the modal opens or PDF changes, ensuring consistent form state.
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
32-44: LGTM - Good use ofcreateMemofor derived state.The sorting logic and derived flags (
hasPrimary,hasProtocol) are properly memoized, ensuring efficient reactive updates.
156-170: LGTM - Correct logic forhasPrimary/hasProtocolflags.The conditional logic correctly indicates whether another PDF in the study has the primary/protocol tag, enabling proper UI hints in PdfListItem.
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (2)
42-57: LGTM - OAuth redirect state restoration is well-implemented.The mount hook properly checks for restore params, retrieves saved state, clears it after use, and handles errors gracefully.
227-263: LGTM - Modal integrations are properly wired.All modals receive appropriate props and handlers. The Google Drive picker correctly targets specific studies via
googleDriveTargetStudyId.packages/web/src/primitives/useProject/index.js (1)
191-195: LGTM!The new PDF operation passthroughs follow the established pattern consistently, using optional chaining for null safety before delegating to
pdfOps.packages/web/src/primitives/useProject/pdfs.js (5)
25-39: LGTM!The
getPdfsMaphelper correctly handles null safety checks and the optional creation pattern.
45-54: LGTM!The
clearTaghelper correctly enforces the single primary/protocol constraint by resetting conflicting tags to 'secondary'.
106-120: LGTM!The removal logic correctly deletes by
pdfIdand updates the study timestamp.
175-202: LGTM with a note on documentation consistency.The metadata update logic is correct, properly handling deletion of empty values. Note that
handleUpdatePdfMetadatainuseProjectPdfHandlers.js(line 159) documentsabstract?as a metadata field, but it's not included in thefieldsarray here. Ifabstractis intended to be supported, add it to line 186.
209-220: LGTM!Clean convenience methods that improve API ergonomics.
packages/web/src/primitives/useProjectPdfHandlers.js (5)
26-33: LGTM!The destructuring of
projectActionsis appropriate here since these are stable function references, not reactive values. The expanded API surface aligns with the new PDF operations.
92-123: LGTM!The auto-primary logic for first PDF is a good UX improvement. Note: As mentioned in the
pdfs.jsreview, consider handling the case whereaddPdfToStudyreturnsnullto avoid orphaned R2 files.
130-143: LGTM!The operation order (server delete → Y.js removal → cache clear) is appropriate. Server-side deletion should happen first to ensure the file is actually removed before updating local state.
151-163: LGTM!Clean handler wrappers. Note the
abstract?mentioned in the JSDoc (line 159) isn't currently supported in the underlyingupdatePdfMetadataimplementation.
199-200: LGTM!The convenience method wrappers are clear. Minor note: these could be simplified to direct references (
setPdfAsPrimary, setPdfAsProtocol,) but the current form works fine.
| const service = useMachine(progress.machine, () => ({ | ||
| ...machineProps, | ||
| id: createUniqueId(), | ||
| min: 0, | ||
| max: 100, | ||
| }); | ||
|
|
||
| const service = useMachine(progress.machine, context); | ||
| })); |
There was a problem hiding this comment.
Fix property override order to respect user-provided min/max values.
The current spread order (...machineProps first, then min: 0, max: 100) causes the hardcoded values to override any user-provided min and max props, contradicting the component documentation that lists these as customizable with defaults.
Apply this diff to fix the override order:
- const service = useMachine(progress.machine, () => ({
- ...machineProps,
- id: createUniqueId(),
- min: 0,
- max: 100,
- }));
+ const service = useMachine(progress.machine, () => ({
+ id: createUniqueId(),
+ min: 0,
+ max: 100,
+ ...machineProps,
+ }));This ensures defaults are set first, then machineProps can override them while keeping id generation controlled.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const service = useMachine(progress.machine, () => ({ | |
| ...machineProps, | |
| id: createUniqueId(), | |
| min: 0, | |
| max: 100, | |
| }); | |
| const service = useMachine(progress.machine, context); | |
| })); | |
| const service = useMachine(progress.machine, () => ({ | |
| id: createUniqueId(), | |
| min: 0, | |
| max: 100, | |
| ...machineProps, | |
| })); |
🤖 Prompt for AI Agents
In packages/ui/src/zag/Progress.jsx around lines 29 to 34, the spread order puts
...machineProps before the hardcoded min: 0 and max: 100 so user-provided
min/max are being overridden; change the object construction so the defaults
(min: 0, max: 100) come first, then spread ...machineProps so user values can
override defaults, and keep id: createUniqueId() applied explicitly (e.g.,
defaults -> ...machineProps -> id) to preserve id generation control.
| const service = useMachine(toggle.machine, () => ({ | ||
| ...machineProps, | ||
| id: createUniqueId(), | ||
| orientation: 'horizontal', | ||
| loop: true, | ||
| rovingFocus: true, | ||
| deselectable: true, | ||
| }); | ||
|
|
||
| const service = useMachine(toggle.machine, context); | ||
| })); |
There was a problem hiding this comment.
Hardcoded defaults override machineProps, breaking documented API.
The initialization pattern spreads machineProps first, then sets hardcoded values for orientation, loop, rovingFocus, and deselectable. Since these come after the spread, they override any values passed via props, making these documented props non-functional.
For example, if a user passes orientation="vertical", it goes into machineProps but is immediately overridden by orientation: 'horizontal' on line 28.
Based on learnings, RadioGroup.jsx demonstrates the correct pattern: split these props into local, then use conditional defaults like local.orientation || 'horizontal'.
🔎 Apply this diff to respect props while providing defaults:
export function ToggleGroup(props) {
- const [local, machineProps] = splitProps(props, ['items', 'size', 'class']);
+ const [local, machineProps] = splitProps(props, [
+ 'items',
+ 'size',
+ 'class',
+ 'orientation',
+ 'loop',
+ 'rovingFocus',
+ 'deselectable',
+ ]);
const service = useMachine(toggle.machine, () => ({
...machineProps,
id: createUniqueId(),
- orientation: 'horizontal',
- loop: true,
- rovingFocus: true,
- deselectable: true,
+ orientation: local.orientation || 'horizontal',
+ loop: local.loop ?? true,
+ rovingFocus: local.rovingFocus ?? true,
+ deselectable: local.deselectable ?? true,
}));Note: Using ?? (nullish coalescing) for boolean props to handle explicit false values correctly, and || for orientation since both undefined and empty string should fall back to the default.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const service = useMachine(toggle.machine, () => ({ | |
| ...machineProps, | |
| id: createUniqueId(), | |
| orientation: 'horizontal', | |
| loop: true, | |
| rovingFocus: true, | |
| deselectable: true, | |
| }); | |
| const service = useMachine(toggle.machine, context); | |
| })); | |
| export function ToggleGroup(props) { | |
| const [local, machineProps] = splitProps(props, [ | |
| 'items', | |
| 'size', | |
| 'class', | |
| 'orientation', | |
| 'loop', | |
| 'rovingFocus', | |
| 'deselectable', | |
| ]); | |
| const service = useMachine(toggle.machine, () => ({ | |
| ...machineProps, | |
| id: createUniqueId(), | |
| orientation: local.orientation || 'horizontal', | |
| loop: local.loop ?? true, | |
| rovingFocus: local.rovingFocus ?? true, | |
| deselectable: local.deselectable ?? true, | |
| })); |
🤖 Prompt for AI Agents
In packages/ui/src/zag/ToggleGroup.jsx around lines 25 to 32, the current
useMachine call spreads machineProps first and then unconditionally sets
orientation, loop, rovingFocus, and deselectable which overrides any values
passed in; change to extract local props from machineProps (or component props)
and pass defaults conditionally so passed-in values win: set id:
createUniqueId(), orientation: local.orientation || 'horizontal', loop:
local.loop ?? true, rovingFocus: local.rovingFocus ?? true, deselectable:
local.deselectable ?? true, and spread the remaining machineProps so documented
props are respected while preserving defaults.
| const service = useMachine(tour.machine, () => ({ | ||
| ...machineProps, | ||
| id: createUniqueId(), | ||
| closeOnInteractOutside: true, | ||
| closeOnEscape: true, | ||
| keyboardNavigation: true, | ||
| }); | ||
|
|
||
| const service = useMachine(tour.machine, context); | ||
| })); |
There was a problem hiding this comment.
Fix property override order and move ID generation outside the factory.
The current implementation has two critical issues:
-
Property override order is backwards: Spreading
machinePropsfirst, then setting defaults means user-provided props are ignored. Users cannot overridecloseOnInteractOutside,closeOnEscape, orkeyboardNavigationdespite the JSDoc (lines 36-38) documenting them as configurable with defaults. -
Unstable ID generation: Calling
createUniqueId()inside the factory function passed touseMachinemay cause stability issues if the factory is re-evaluated. Solid hooks likecreateUniqueId()should be called in the component body.
🔎 Apply this diff to fix both issues:
+ const id = createUniqueId();
+
- const service = useMachine(tour.machine, () => ({
- ...machineProps,
- id: createUniqueId(),
+ const service = useMachine(tour.machine, () => ({
+ id,
closeOnInteractOutside: true,
closeOnEscape: true,
keyboardNavigation: true,
+ ...machineProps,
}));This ensures:
- The ID is stable and created once during component initialization
- Defaults are applied first, then user props override them as documented
🤖 Prompt for AI Agents
In packages/ui/src/zag/Tour.jsx around lines 59 to 65, the factory passed to
useMachine both generates a unique ID inside the factory (unstable) and spreads
machineProps before defaults (so user props are overridden); fix by calling
createUniqueId() once in the component body to produce a stable id, then in the
factory spread the defaults first and then machineProps so user-provided options
override defaults (ensure id is set from the precomputed value).
| {/* Study count */} | ||
| <div class='flex items-center justify-between'> | ||
| <p class='text-sm text-gray-500'> | ||
| {studies().length} {studies().length === 1 ? 'study' : 'studies'} in this project | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
Study count displays during loading state.
The study count section renders regardless of hasData() state, which could show "0 studies" while data is loading. Consider wrapping it in the hasData() check or showing a skeleton.
Suggested fix
+ <Show when={hasData()}>
{/* Study count */}
<div class='flex items-center justify-between'>
<p class='text-sm text-gray-500'>
{studies().length} {studies().length === 1 ? 'study' : 'studies'} in this project
</p>
</div>
+ </Show>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* Study count */} | |
| <div class='flex items-center justify-between'> | |
| <p class='text-sm text-gray-500'> | |
| {studies().length} {studies().length === 1 ? 'study' : 'studies'} in this project | |
| </p> | |
| </div> | |
| <Show when={hasData()}> | |
| {/* Study count */} | |
| <div class='flex items-center justify-between'> | |
| <p class='text-sm text-gray-500'> | |
| {studies().length} {studies().length === 1 ? 'study' : 'studies'} in this project | |
| </p> | |
| </div> | |
| </Show> |
🤖 Prompt for AI Agents
In packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx around
lines 184 to 189, the study count block renders even when data is still loading
and can display "0 studies"; wrap the entire count block in the existing
hasData() check (or an equivalent loading guard) so it only renders when
hasData() is true, or alternatively render a loading skeleton/placeholder while
!hasData(); ensure you use the same JSX className conventions as the file and
preserve the current content for the true state.
| // Validate publication year | ||
| let parsedPublicationYear; | ||
| if (publicationYear()) { | ||
| const raw = publicationYear().toString().trim(); | ||
| const num = Number(raw); | ||
| if (Number.isFinite(num)) { | ||
| const trunc = Math.trunc(num); | ||
| if (Number.isInteger(trunc) && trunc >= 1900 && trunc <= 2100) { | ||
| parsedPublicationYear = trunc; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Invalid publication year is silently discarded without user feedback.
When a user enters an invalid year (outside 1900-2100 or non-numeric), the value is silently set to undefined. Consider showing a validation error or toast to inform the user.
Suggested approach
// Add validation state
const [yearError, setYearError] = createSignal('');
// In handleSave, before building metadata:
if (publicationYear()) {
const raw = publicationYear().toString().trim();
const num = Number(raw);
if (!Number.isFinite(num) || num < 1900 || num > 2100) {
setYearError('Year must be between 1900 and 2100');
setSaving(false);
return;
}
parsedPublicationYear = Math.trunc(num);
}🤖 Prompt for AI Agents
In packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx
around lines 46 to 57, the publication year validation silently drops invalid
input (non-numeric or outside 1900–2100) without user feedback; update the
component to surface validation errors: add a validation state (e.g., yearError
signal) and in the save/validation flow, when publicationYear is present but
invalid, set the yearError message (or show a toast), stop the save (reset
saving flag) and return; only parse/truncate and clear the error when the value
is valid.
| function addPdfToStudy(studyId, pdfInfo, tag = 'secondary') { | ||
| const ydoc = getYDoc(); | ||
| if (!ydoc || !isSynced()) return null; | ||
|
|
||
| const studiesMap = ydoc.getMap('reviews'); | ||
| const studyYMap = studiesMap.get(studyId); | ||
| if (!studyYMap) return null; | ||
|
|
||
| const pdfsMap = getPdfsMap(studyId, true); | ||
|
|
||
| // If setting as primary or protocol, clear existing tag first | ||
| if (tag === 'primary' || tag === 'protocol') { | ||
| clearTag(studyId, tag); | ||
| } | ||
|
|
||
| const pdfId = nanoid(); | ||
| const pdfYMap = new Y.Map(); | ||
| pdfYMap.set('id', pdfId); | ||
| pdfYMap.set('key', pdfInfo.key); | ||
| pdfYMap.set('fileName', pdfInfo.fileName); | ||
| pdfYMap.set('size', pdfInfo.size); | ||
| pdfYMap.set('uploadedBy', pdfInfo.uploadedBy); | ||
| pdfYMap.set('uploadedAt', pdfInfo.uploadedAt || Date.now()); | ||
| pdfsMap.set(pdfInfo.fileName, pdfYMap); | ||
| pdfYMap.set('tag', tag); | ||
|
|
||
| // Citation metadata (optional, can be added later via updatePdfMetadata) | ||
| if (pdfInfo.title) pdfYMap.set('title', pdfInfo.title); | ||
| if (pdfInfo.firstAuthor) pdfYMap.set('firstAuthor', pdfInfo.firstAuthor); | ||
| if (pdfInfo.publicationYear) pdfYMap.set('publicationYear', pdfInfo.publicationYear); | ||
| if (pdfInfo.journal) pdfYMap.set('journal', pdfInfo.journal); | ||
| if (pdfInfo.doi) pdfYMap.set('doi', pdfInfo.doi); | ||
|
|
||
| pdfsMap.set(pdfId, pdfYMap); | ||
|
|
||
| studyYMap.set('updatedAt', Date.now()); | ||
| return pdfId; | ||
| } |
There was a problem hiding this comment.
Consider how callers handle null return.
The function returns null on multiple early exit paths (lines 65, 69) but also returns pdfId on success. In useProjectPdfHandlers.js, handleUploadPdf (line 106) assigns the result directly without checking for null, and then returns it to the caller.
If the Y.Doc isn't ready or the study doesn't exist, the PDF file will have been uploaded to R2 but won't be tracked in Y.js, causing orphaned files.
Consider returning a consistent result or throwing an error on failure so callers can handle it appropriately.
🤖 Prompt for AI Agents
In packages/web/src/primitives/useProject/pdfs.js around lines 63 to 99, the
function addPdfToStudy currently returns null on early exits (ydoc missing or
study missing) but returns a pdfId string on success, which leads to callers
(e.g., handleUploadPdf) treating a null like a valid id and leaving uploaded
files orphaned. Change the function to return a consistent failure signal by
throwing a descriptive Error when the Y.Doc isn't ready or the study doesn't
exist (e.g., throw new Error('YDoc not ready') / throw new Error('Study not
found')), or alternatively return a consistent object { success: false, reason:
'...' } and on success return { success: true, id: pdfId }; then update callers
(handleUploadPdf and any other callers) to catch the thrown errors or check the
success flag and abort cleanup/upload handling so files are not left orphaned.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/ui/src/zag/Tour.jsx (1)
59-65: Spread order fixed, but ID generation issue persists.The spread order has been corrected—defaults are now properly placed before
...machineProps, allowing user-provided props to override them as documented. However, the previous review's concern about callingcreateUniqueId()inside the factory function remains unaddressed.Per SolidJS best practices and the pattern shown in
packages/ui/src/zag/Editable.jsx(lines 105-127), the ID should be generated once in the component body and then referenced inside the factory to ensure stability.🔎 Recommended pattern (as shown in Editable.jsx):
+ const id = createUniqueId(); + const service = useMachine(tour.machine, () => ({ - id: createUniqueId(), + id, closeOnInteractOutside: true, closeOnEscape: true, keyboardNavigation: true, ...machineProps, }));packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx (1)
46-62: Invalid publication year handling unchanged.The publication year validation still silently converts invalid input to
0without user feedback. This issue was previously flagged in an earlier review.packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
165-165: Add null validation foraddPdfToStudyreturn value before setting state.The past review comment identified that
addPdfToStudycan returnnullwhen the document isn't synced or the study doesn't exist. Line 165 callssetSelectedPdfId(pdfId)without checking ifpdfIdis valid, which could set the state to null and cause unexpected UI behavior.🔎 Apply this diff to validate the return value:
- setSelectedPdfId(pdfId); + if (pdfId) { + setSelectedPdfId(pdfId); + }
🧹 Nitpick comments (2)
packages/web/src/primitives/useProject/pdfs.js (1)
190-200: Consider explicit handling of falsy numeric values.Line 194's condition treats
publicationYear: 0as a removal signal (since0 !== ''is true but the overall logic deletes it). While 0 is not a valid publication year, the implicit behavior may be unclear to callers. Consider documenting this or adding an explicit check for numeric fields.Suggested approach
for (const field of fields) { if (field in metadata) { - if (metadata[field] !== undefined && metadata[field] !== null && metadata[field] !== '') { + const value = metadata[field]; + // For numeric fields like publicationYear, treat 0 as deletion + if (value !== undefined && value !== null && value !== '' && value !== 0) { pdfYMap.set(field, metadata[field]); } else { pdfYMap.delete(field); } } }packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
304-311: Consider showing PDF info when only one PDF exists.The
PdfSelectoris hidden whenstudyPdfs().length <= 1, which means users won't see any PDF information in the header for single-PDF studies. While the dropdown isn't needed, displaying the PDF name and tag (for primary/protocol PDFs) could improve context awareness.💡 Example: Show single PDF info without dropdown
- {/* PDF Selector - only shown when multiple PDFs exist */} - <Show when={studyPdfs().length > 1}> + {/* PDF Selector */} + <Show when={studyPdfs().length > 0}> + <Show when={studyPdfs().length > 1} fallback={ + <div class='flex items-center gap-2 text-sm text-gray-600'> + <CgFileDocument class='w-4 h-4' /> + <span class='truncate max-w-xs'>{currentPdf()?.fileName}</span> + </div> + }> <PdfSelector pdfs={studyPdfs()} selectedPdfId={selectedPdfId()} onSelect={handlePdfSelect} /> + </Show> </Show>Note: You would need to import
CgFileDocumentfromsolid-icons/cg.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/ui/src/zag/Accordion.jsx(2 hunks)packages/ui/src/zag/Clipboard.jsx(3 hunks)packages/ui/src/zag/Combobox.jsx(3 hunks)packages/ui/src/zag/Menu.jsx(4 hunks)packages/ui/src/zag/Progress.jsx(2 hunks)packages/ui/src/zag/RadioGroup.jsx(2 hunks)packages/ui/src/zag/ToggleGroup.jsx(3 hunks)packages/ui/src/zag/Tour.jsx(1 hunks)packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(5 hunks)packages/web/src/components/project-ui/ProjectView.jsx(4 hunks)packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx(1 hunks)packages/web/src/primitives/useProject/pdfs.js(3 hunks)packages/web/src/primitives/useProjectPdfHandlers.js(2 hunks)packages/web/src/primitives/useProjectStudyHandlers.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ui/src/zag/Progress.jsx
- packages/ui/src/zag/RadioGroup.jsx
- packages/ui/src/zag/Combobox.jsx
- packages/web/src/components/project-ui/ProjectView.jsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. Extract sub-modules into a folder (e.g.,ComponentName/withindex.jsxand helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Clipboard.jsxpackages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/web/src/primitives/useProjectPdfHandlers.jspackages/ui/src/zag/Tour.jsxpackages/ui/src/zag/Menu.jsxpackages/ui/src/zag/ToggleGroup.jsxpackages/web/src/primitives/useProject/pdfs.js
**/*.{js,jsx,ts,tsx,json,yaml,yml,toml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using config files rather than hardcoding values
Files:
packages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Clipboard.jsxpackages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/web/src/primitives/useProjectPdfHandlers.jspackages/ui/src/zag/Tour.jsxpackages/ui/src/zag/Menu.jsxpackages/ui/src/zag/ToggleGroup.jsxpackages/web/src/primitives/useProject/pdfs.js
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Keep files small, focused, and modular. If a file exceeds a high number of lines, extract sub-modules into a folder with index.jsx and helper components, or move complex logic into separate utility files or primitives
Each file should handle one coherent responsibility
Group related components in subdirectories with an index.js barrel export
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
Import stores directly where needed instead of passing values through multiple components
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 (when scoped to a feature)
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 (e.g., () => props.name) to ensure they are always up-to-date
Use createMemo for derived values based on props or state to ensure reactive updates
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{js,jsx,ts,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
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 ...
Files:
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/web/src/primitives/useProjectPdfHandlers.jspackages/web/src/primitives/useProject/pdfs.js
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
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
Files:
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsxpackages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx
🧠 Learnings (9)
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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 (e.g., () => props.name) to ensure they are always up-to-date
Applied to files:
packages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Tour.jsxpackages/ui/src/zag/ToggleGroup.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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
Applied to files:
packages/ui/src/zag/Accordion.jsxpackages/ui/src/zag/Tour.jsxpackages/ui/src/zag/ToggleGroup.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context (when scoped to a feature)
Applied to files:
packages/ui/src/zag/Tour.jsxpackages/ui/src/zag/ToggleGroup.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : When you have complex state or state objects, use Solid's `createStore` for better performance and reactivity
Applied to files:
packages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use Solid's createStore for complex state or state objects for better performance and reactivity
Applied to files:
packages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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
Applied to files:
packages/ui/src/zag/Tour.jsxpackages/ui/src/zag/ToggleGroup.jsx
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Applied to files:
packages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Applied to files:
packages/ui/src/zag/Tour.jsx
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Components should receive at most 1–5 props, and only for local configuration, not shared state
Applied to files:
packages/ui/src/zag/ToggleGroup.jsx
🧬 Code graph analysis (10)
packages/ui/src/zag/Accordion.jsx (10)
packages/ui/src/zag/Clipboard.jsx (2)
service(23-27)service(90-94)packages/ui/src/zag/Menu.jsx (1)
service(41-46)packages/ui/src/zag/Progress.jsx (1)
service(29-34)packages/ui/src/zag/RadioGroup.jsx (1)
service(22-26)packages/ui/src/zag/ToggleGroup.jsx (1)
service(25-32)packages/ui/src/zag/Tour.jsx (1)
service(59-65)packages/ui/src/zag/Editable.jsx (1)
service(106-128)packages/ui/src/zag/Avatar.jsx (1)
service(11-14)packages/ui/src/zag/Collapsible.jsx (1)
service(22-30)packages/ui/src/zag/Dialog.jsx (2)
service(25-31)service(117-130)
packages/ui/src/zag/Clipboard.jsx (5)
packages/ui/src/zag/Accordion.jsx (1)
service(22-26)packages/ui/src/zag/Combobox.jsx (1)
service(52-69)packages/ui/src/zag/Progress.jsx (1)
service(29-34)packages/ui/src/zag/Avatar.jsx (1)
service(11-14)packages/ui/src/zag/Dialog.jsx (2)
service(25-31)service(117-130)
packages/web/src/components/project-ui/all-studies/EditPdfMetadataModal.jsx (1)
packages/ui/src/zag/Dialog.jsx (1)
Dialog(18-92)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (1)
PdfSelector(15-99)
packages/web/src/primitives/useProjectStudyHandlers.js (3)
packages/web/src/api/better-auth-store.js (1)
user(147-170)packages/web/src/components/profile-ui/ProfilePage.jsx (1)
user(33-33)packages/web/src/api/pdf-api.js (1)
deletePdf(113-127)
packages/web/src/primitives/useProjectPdfHandlers.js (2)
packages/web/src/primitives/pdfCache.js (3)
getCachedPdf(82-102)cachePdf(111-138)removeCachedPdf(146-163)packages/web/src/api/pdf-api.js (3)
downloadPdf(79-93)uploadPdf(40-70)deletePdf(113-127)
packages/ui/src/zag/Tour.jsx (15)
packages/ui/src/zag/Accordion.jsx (1)
service(22-26)packages/ui/src/zag/Clipboard.jsx (2)
service(23-27)service(90-94)packages/ui/src/zag/Combobox.jsx (1)
service(52-69)packages/ui/src/zag/Menu.jsx (1)
service(41-46)packages/ui/src/zag/Progress.jsx (1)
service(29-34)packages/ui/src/zag/RadioGroup.jsx (1)
service(22-26)packages/ui/src/zag/ToggleGroup.jsx (1)
service(25-32)packages/ui/src/zag/Editable.jsx (1)
service(106-128)packages/ui/src/zag/Avatar.jsx (1)
service(11-14)packages/ui/src/zag/Collapsible.jsx (1)
service(22-30)packages/ui/src/zag/Checkbox.jsx (1)
service(39-49)packages/ui/src/zag/Dialog.jsx (2)
service(25-31)service(117-130)packages/ui/src/zag/FileUpload.jsx (1)
service(123-148)packages/ui/src/zag/FloatingPanel.jsx (1)
service(39-58)packages/ui/src/zag/Toast.jsx (1)
machineProps(22-26)
packages/ui/src/zag/Menu.jsx (4)
packages/ui/src/zag/Accordion.jsx (3)
local(20-20)service(22-26)api(28-28)packages/ui/src/zag/Popover.jsx (1)
local(28-37)packages/ui/src/zag/Avatar.jsx (2)
service(11-14)api(16-16)packages/ui/src/zag/Checkbox.jsx (2)
service(39-49)api(51-51)
packages/ui/src/zag/ToggleGroup.jsx (5)
packages/ui/src/zag/Accordion.jsx (2)
service(22-26)api(28-28)packages/ui/src/zag/Clipboard.jsx (4)
service(23-27)service(90-94)api(29-29)api(96-96)packages/ui/src/zag/Combobox.jsx (2)
service(52-69)api(71-71)packages/ui/src/zag/RadioGroup.jsx (3)
service(22-26)isVertical(30-30)api(28-28)packages/ui/src/zag/Avatar.jsx (2)
service(11-14)api(16-16)
packages/web/src/primitives/useProject/pdfs.js (2)
packages/web/src/primitives/useProject/index.js (2)
ydoc(29-29)getYDoc(48-48)packages/web/src/primitives/useProject/sync.js (1)
pdfsMap(109-109)
⏰ 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 (16)
packages/ui/src/zag/ToggleGroup.jsx (2)
25-32: Previous issue resolved - props now correctly override defaults.The initialization order has been fixed. By placing defaults before
...machineProps, user-provided values now properly override the defaults (e.g.,orientation="vertical"will work as documented), matching the pattern used in other Zag component wrappers like Accordion, RadioGroup, and Clipboard.
47-47: LGTM - consistent with service-based API pattern.Accessing orientation via
api().orientationcorrectly aligns with the refactored initialization pattern and matches similar usage in RadioGroup.jsx.packages/ui/src/zag/Menu.jsx (1)
97-117: Conditional indicator rendering implemented correctly.The trigger button properly integrates the new
hideIndicatorprop, conditionally rendering the chevron indicator based on the prop value. The implementation maintains accessibility and follows the existing styling patterns.packages/web/src/primitives/useProject/pdfs.js (3)
25-39: LGTM!The
getPdfsMaphelper properly handles null-safety and lazy initialization of the PDFs map.
45-54: LGTM!The
clearTaghelper correctly enforces the single primary/protocol constraint by demoting conflicting PDFs to secondary.
64-104: Error handling improved.The function now throws descriptive errors instead of returning
null, which addresses the previous review concern about orphaned files. Callers can now handle failures appropriately.packages/web/src/primitives/useProjectStudyHandlers.js (2)
78-92: Cleanup pattern is appropriate.The fire-and-forget cleanup on line 90 (not awaited before rethrowing) is acceptable since it prevents blocking the error flow. Failed cleanup is logged via
.catch(console.warn).
213-225: LGTM!The cleanup pattern on metadata failure is consistent with the Google Drive import flow. The orphaned file is deleted without blocking the error handling.
packages/web/src/primitives/useProjectPdfHandlers.js (5)
57-83: LGTM!The cache-first download approach is efficient, and resource cleanup with
URL.revokeObjectURLis properly handled.
107-119: LGTM!The function properly captures and returns the generated PDF ID, allowing callers to reference the newly uploaded PDF.
137-150: LGTM!The delete handler properly coordinates server deletion, Y.js removal, and cache cleanup.
175-204: LGTM!The Google Drive import handler applies consistent tagging logic and cleanup patterns as the upload flow.
96-98: No action needed. The code correctly accesses thepdfsproperty on study objects. Verification confirms that studies are initialized withpdfs: []in the sync layer, and PDFs are properly populated into this array when synced from Y.js. The implementation at lines 96-98 is correct.packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (3)
50-72: Well-structured reactive state management for multi-PDF support.The memo chain (studyPdfs → defaultPdf → currentPdf) correctly uses
createMemofor derived values and implements a sensible fallback hierarchy: selected PDF → primary PDF → first available → null. Safe access patterns with optional chaining and fallback values are used throughout.
180-199: Correctly adapted for multi-PDF deletion.The delete flow properly retrieves the current PDF, uses
pdf.idfor Y.js metadata removal (instead of fileName), and thoroughly cleans up state including the newselectedPdfId. The early return guard at line 183 prevents errors when no PDF is selected.
146-147: Auto-tagging logic limits tag specification to upload time; post-upload modifications are available.The current implementation auto-tags new PDFs as
'primary'if no PDFs exist, otherwise'secondary'. WhileupdatePdfTag()andhandleTagChange()support changing tags to any value ('primary','protocol','secondary') after upload, users cannot:
- Upload a protocol PDF directly
- Specify tags during the upload process
This means users must upload first, then manually change the tag via the tag selection UI—a two-step workflow instead of one-step specification. For the PR objective "Allow multiple pdf upload and tagging", consider whether tag selection during upload would improve UX, or if the current post-upload modification approach is sufficient.
| const service = useMachine(accordion.machine, () => ({ | ||
| id: createUniqueId(), | ||
| collapsible: true, | ||
| ...machineProps, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Spread order fixed; consider moving ID generation outside the factory.
The default prop override issue from the previous review has been resolved—collapsible and other defaults now correctly appear before ...machineProps, allowing user configuration to take precedence.
However, following the pattern demonstrated in packages/ui/src/zag/Editable.jsx (lines 105-127), calling createUniqueId() inside the factory function should be avoided. Generate the ID once in the component body to ensure stability.
🔎 Apply this pattern for consistency:
+ const id = createUniqueId();
+
const service = useMachine(accordion.machine, () => ({
- id: createUniqueId(),
+ id,
collapsible: true,
...machineProps,
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const service = useMachine(accordion.machine, () => ({ | |
| id: createUniqueId(), | |
| collapsible: true, | |
| ...machineProps, | |
| })); | |
| const id = createUniqueId(); | |
| const service = useMachine(accordion.machine, () => ({ | |
| id, | |
| collapsible: true, | |
| ...machineProps, | |
| })); |
🤖 Prompt for AI Agents
packages/ui/src/zag/Accordion.jsx around lines 22 to 26: currently
createUniqueId() is called inside the useMachine factory; move ID generation out
of the factory so the ID is created once per component render (e.g., generate
the ID in the component body before calling useMachine) and then pass that
stable id into the factory parameters, ensuring the id does not get re-generated
on each factory invocation.
| const service = useMachine(clipboard.machine, () => ({ | ||
| id: createUniqueId(), | ||
| timeout: 3000, | ||
| }); | ||
|
|
||
| const service = useMachine(clipboard.machine, context); | ||
| ...machineProps, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Spread order fixed in both components; consider moving ID generation outside the factory.
The default prop override issues from the previous review have been resolved in both the Clipboard and CopyButton components—timeout defaults now correctly precede ...machineProps, allowing user-provided timeout values to override the default.
However, both components call createUniqueId() inside the factory function. Following the pattern in packages/ui/src/zag/Editable.jsx (lines 105-127), generate the ID once in each component body to ensure stability and consistency with best practices.
🔎 Apply this pattern for both components:
For Clipboard component (lines 23-27):
+ const id = createUniqueId();
+
const service = useMachine(clipboard.machine, () => ({
- id: createUniqueId(),
+ id,
timeout: 3000,
...machineProps,
}));For CopyButton component (lines 90-94):
+ const id = createUniqueId();
+
const service = useMachine(clipboard.machine, () => ({
- id: createUniqueId(),
+ id,
timeout: 3000,
...machineProps,
}));Also applies to: 90-94
🤖 Prompt for AI Agents
In packages/ui/src/zag/Clipboard.jsx around lines 23-27 and
packages/ui/src/zag/CopyButton.jsx around lines 90-94, move the call to
createUniqueId() out of the useMachine factory so the ID is generated once per
component render (like Editable.jsx lines 105-127); create a const id =
createUniqueId() at the top of the component body and then pass that id into the
useMachine factory return object instead of calling createUniqueId() inside the
factory, ensuring a stable, single ID instance while keeping timeout and
...machineProps order as is.
| const [local, machineProps] = splitProps(props, [ | ||
| 'trigger', | ||
| 'items', | ||
| 'inDialog', | ||
| 'hideIndicator', | ||
| 'placement', | ||
| 'class', | ||
| ]); | ||
|
|
||
| const context = mergeProps(machineProps, { | ||
| const service = useMachine(menu.machine, () => ({ | ||
| id: createUniqueId(), | ||
| closeOnSelect: true, | ||
| positioning: { placement: 'bottom-start' }, | ||
| }); | ||
|
|
||
| const service = useMachine(menu.machine, context); | ||
| positioning: { placement: local.placement || 'bottom-start' }, | ||
| ...machineProps, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
New props implemented correctly; consider moving ID generation outside the factory.
The new hideIndicator and placement props are properly extracted and used:
placementis correctly converted to the machine'spositioningconfiguration (line 44)hideIndicatoris appropriately used to conditionally render the indicator (line 102)- Default values are placed before
...machineProps, allowing user overrides
However, consistent with the pattern in packages/ui/src/zag/Editable.jsx (lines 105-127), createUniqueId() should be called once in the component body rather than inside the factory function to ensure stability.
🔎 Apply this pattern:
+ const id = createUniqueId();
+
const service = useMachine(menu.machine, () => ({
- id: createUniqueId(),
+ id,
closeOnSelect: true,
positioning: { placement: local.placement || 'bottom-start' },
...machineProps,
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [local, machineProps] = splitProps(props, [ | |
| 'trigger', | |
| 'items', | |
| 'inDialog', | |
| 'hideIndicator', | |
| 'placement', | |
| 'class', | |
| ]); | |
| const context = mergeProps(machineProps, { | |
| const service = useMachine(menu.machine, () => ({ | |
| id: createUniqueId(), | |
| closeOnSelect: true, | |
| positioning: { placement: 'bottom-start' }, | |
| }); | |
| const service = useMachine(menu.machine, context); | |
| positioning: { placement: local.placement || 'bottom-start' }, | |
| ...machineProps, | |
| })); | |
| const [local, machineProps] = splitProps(props, [ | |
| 'trigger', | |
| 'items', | |
| 'inDialog', | |
| 'hideIndicator', | |
| 'placement', | |
| 'class', | |
| ]); | |
| const id = createUniqueId(); | |
| const service = useMachine(menu.machine, () => ({ | |
| id, | |
| closeOnSelect: true, | |
| positioning: { placement: local.placement || 'bottom-start' }, | |
| ...machineProps, | |
| })); |
🤖 Prompt for AI Agents
In packages/ui/src/zag/Menu.jsx around lines 32 to 46, createUniqueId() is
currently invoked inside the useMachine factory which can create a new id each
time the factory runs; move the call to createUniqueId() to the top of the
component body (before useMachine) to generate the id once per component
instance, then pass that id into the factory via the id property (keep
closeOnSelect, default positioning and spread ...machineProps as-is).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (1)
170-175: Study count displays during loading state.The study count section renders regardless of
hasData()state, which could show "0 studies" while data is loading. This was flagged in a previous review.
🧹 Nitpick comments (1)
packages/web/src/primitives/useProjectStudyHandlers.js (1)
28-36: Consider adding input validation for robustness.The helper works correctly for standard PDF filenames. However, consider validating that the filename actually ends with
🔎 View suggested enhancement:
const getStudyNameFromFilename = pdfFileName => { - if (!pdfFileName) return 'Untitled Study'; - return pdfFileName.replace(/\.pdf$/i, ''); + if (!pdfFileName) return 'Untitled Study'; + // Only remove .pdf extension if present, otherwise use as-is + return pdfFileName.replace(/\.pdf$/i, '') || pdfFileName; };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/web/src/components/project-ui/ProjectHeader.jsx(2 hunks)packages/web/src/components/project-ui/ProjectSettings.jsx(0 hunks)packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/index.js(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx(1 hunks)packages/web/src/components/project-ui/tabs/OverviewTab.jsx(1 hunks)packages/web/src/lib/__tests__/studyNaming.test.js(0 hunks)packages/web/src/lib/studyNaming.js(0 hunks)packages/web/src/primitives/useProjectStudyHandlers.js(7 hunks)
💤 Files with no reviewable changes (3)
- packages/web/src/lib/tests/studyNaming.test.js
- packages/web/src/components/project-ui/ProjectSettings.jsx
- packages/web/src/lib/studyNaming.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/components/project-ui/all-studies/index.js
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. Extract sub-modules into a folder (e.g.,ComponentName/withindex.jsxand helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/components/project-ui/tabs/OverviewTab.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Keep files small, focused, and modular. If a file exceeds a high number of lines, extract sub-modules into a folder with index.jsx and helper components, or move complex logic into separate utility files or primitives
Each file should handle one coherent responsibility
Group related components in subdirectories with an index.js barrel export
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
Import stores directly where needed instead of passing values through multiple components
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 (when scoped to a feature)
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 (e.g., () => props.name) to ensure they are always up-to-date
Use createMemo for derived values based on props or state to ensure reactive updates
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{js,jsx,ts,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
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 ...
Files:
packages/web/src/components/project-ui/tabs/OverviewTab.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
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
Files:
packages/web/src/components/project-ui/tabs/OverviewTab.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
**/*.{js,jsx,ts,tsx,json,yaml,yml,toml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using config files rather than hardcoding values
Files:
packages/web/src/components/project-ui/tabs/OverviewTab.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
🧠 Learnings (1)
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use Solid's createStore for complex state or state objects for better performance and reactivity
Applied to files:
packages/web/src/components/project-ui/tabs/OverviewTab.jsxpackages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx
🧬 Code graph analysis (4)
packages/web/src/primitives/useProjectStudyHandlers.js (3)
packages/web/src/primitives/useProject/sync.js (1)
study(59-88)packages/web/src/api/better-auth-store.js (1)
user(147-170)packages/web/src/api/pdf-api.js (1)
deletePdf(113-127)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (2)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (1)
study(30-30)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
study(29-29)pdfs(30-30)
packages/web/src/components/project-ui/ProjectHeader.jsx (2)
packages/web/src/components/project-ui/ProjectContext.jsx (2)
useProjectContext(51-57)userRole(18-23)packages/ui/src/zag/Editable.jsx (1)
Editable(68-200)
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (3)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (2)
study(26-26)StudyCardHeader(17-146)packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (2)
study(29-29)StudyPdfSection(15-176)packages/ui/src/zag/Collapsible.jsx (1)
Collapsible(15-70)
⏰ 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 (11)
packages/web/src/primitives/useProjectStudyHandlers.js (1)
6-6: LGTM!The import of
deletePdfsupports the new orphaned file cleanup logic in the error handling paths.packages/web/src/components/project-ui/tabs/OverviewTab.jsx (1)
16-24: LGTM!The component follows SolidJS best practices: reads from store directly, uses context for shared handlers, and avoids prop drilling. The removal of ProjectSettings aligns with the PR's broader restructuring.
packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (1)
13-69: Well-structured composition component.The component cleanly separates header and content concerns using the Collapsible pattern. The accessors
study()andexpanded()maintain proper SolidJS reactivity.One observation: While the prop count exceeds the typical 1-5 guideline, this is an acceptable pattern for a composition component that forwards callbacks to children. If the prop list grows further, consider grouping related callbacks (e.g.,
pdfHandlers={{ onView, onDownload, onUpload, onDelete, onTagChange }}).packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (2)
17-66: Clean component with well-structured helpers.Good SolidJS patterns:
const study = () => props.studypreserves reactivity- Derived values (
primaryPdf,assignedReviewers,citationLine) are properly memoized via function gettershandleNameChangecorrectly validates input before triggering the update
68-145: LGTM!The menu implementation is clean with a clear
handleMenuSelectdispatcher. The UI structure properly usestype='button'for accessibility, and the conditional rendering withShowfollows SolidJS best practices.packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx (2)
22-39: Good state organization.Clean separation of concerns with signals for different modal states and UI tracking. Using
createSignal(new Set())forexpandedStudiesis appropriate, and the update pattern intoggleStudyExpandedcorrectly creates a new Set to trigger reactivity.
151-211: Clean rendering structure.Good use of
Showfor conditional rendering with loading/empty states, and properForiteration for the study list. The StudyCard delegation pattern correctly forwards all necessary handlers.packages/web/src/components/project-ui/ProjectHeader.jsx (4)
1-14: Excellent simplification and proper reactive patterns!The refactored imports and state management follow SolidJS best practices:
- Uses
solid-iconslibrary for icons as required by guidelines- Props are never destructured (maintains reactivity)
canEditis properly memoized for derived state- Component remains lean and focused on presentation
16-20: LGTM - good validation logic!The name change handler properly validates input and prevents unnecessary updates when the value hasn't changed.
48-58: Clean simplification with the Editable component!The inline name editing using the
Editablecomponent is well-implemented:
- Properly configured with
activationMode,readOnly, andshowEditIconbased on user permissions- Follows the Zag component reuse guideline
- Significantly simplifies the code by removing manual state management for name editing
68-112: Well-structured description editing UI!The description editing implementation follows good practices:
- Proper use of SolidJS
Showfor conditional rendering- Reactive signal bindings without prop destructuring
- Good hover affordance (edit button appears on
group-hover)- Clean Save/Cancel flow with appropriate button styling
| // PDF handlers | ||
| const handleViewPdf = (studyId, pdf) => { | ||
| handlers.pdfHandlers.handleViewPdf(studyId, pdf); | ||
| }; | ||
|
|
||
| const handleDownloadPdf = (studyId, pdf) => { | ||
| handlers.pdfHandlers.handleDownloadPdf?.(studyId, pdf); | ||
| }; | ||
|
|
||
| const handleUploadPdf = async (studyId, file) => { | ||
| await handlers.pdfHandlers.handleUploadPdf(studyId, file); | ||
| }; | ||
|
|
||
| const handleDeletePdf = (studyId, pdf) => { | ||
| handlers.pdfHandlers.handleDeletePdf?.(studyId, pdf); | ||
| }; | ||
|
|
||
| const handleTagChange = (studyId, pdfId, newTag) => { | ||
| handlers.pdfHandlers.handleTagChange?.(studyId, pdfId, newTag); | ||
| }; |
There was a problem hiding this comment.
Inconsistent optional chaining on PDF handlers.
Some handlers use optional chaining (handleDownloadPdf?., handleDeletePdf?., handleTagChange?.) while others don't (handleViewPdf, handleUploadPdf). This inconsistency could cause runtime errors if a handler is undefined.
Suggested fix for consistency:
const handleViewPdf = (studyId, pdf) => {
- handlers.pdfHandlers.handleViewPdf(studyId, pdf);
+ handlers.pdfHandlers.handleViewPdf?.(studyId, pdf);
};
const handleUploadPdf = async (studyId, file) => {
- await handlers.pdfHandlers.handleUploadPdf(studyId, file);
+ await handlers.pdfHandlers.handleUploadPdf?.(studyId, file);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // PDF handlers | |
| const handleViewPdf = (studyId, pdf) => { | |
| handlers.pdfHandlers.handleViewPdf(studyId, pdf); | |
| }; | |
| const handleDownloadPdf = (studyId, pdf) => { | |
| handlers.pdfHandlers.handleDownloadPdf?.(studyId, pdf); | |
| }; | |
| const handleUploadPdf = async (studyId, file) => { | |
| await handlers.pdfHandlers.handleUploadPdf(studyId, file); | |
| }; | |
| const handleDeletePdf = (studyId, pdf) => { | |
| handlers.pdfHandlers.handleDeletePdf?.(studyId, pdf); | |
| }; | |
| const handleTagChange = (studyId, pdfId, newTag) => { | |
| handlers.pdfHandlers.handleTagChange?.(studyId, pdfId, newTag); | |
| }; | |
| // PDF handlers | |
| const handleViewPdf = (studyId, pdf) => { | |
| handlers.pdfHandlers.handleViewPdf?.(studyId, pdf); | |
| }; | |
| const handleDownloadPdf = (studyId, pdf) => { | |
| handlers.pdfHandlers.handleDownloadPdf?.(studyId, pdf); | |
| }; | |
| const handleUploadPdf = async (studyId, file) => { | |
| await handlers.pdfHandlers.handleUploadPdf?.(studyId, file); | |
| }; | |
| const handleDeletePdf = (studyId, pdf) => { | |
| handlers.pdfHandlers.handleDeletePdf?.(studyId, pdf); | |
| }; | |
| const handleTagChange = (studyId, pdfId, newTag) => { | |
| handlers.pdfHandlers.handleTagChange?.(studyId, pdfId, newTag); | |
| }; |
🤖 Prompt for AI Agents
In packages/web/src/components/project-ui/all-studies/AllStudiesTab.jsx around
lines 115 to 134, the PDF handler calls are inconsistent: some use optional
chaining and others don't, which risks runtime errors if handlers.pdfHandlers or
specific methods are undefined; update the code to consistently guard calls
(either by using optional chaining on handlers.pdfHandlers.method?.(...) for all
calls or by first checking that handlers.pdfHandlers and the specific method
exist before invoking), and ensure async handlers remain awaited where needed
(e.g., await handlers.pdfHandlers.handleUploadPdf?.(studyId, file)).
| // Note: Studies require a PDF, so references need PDFs attached | ||
| const handleImportReferences = references => { | ||
| let successCount = 0; | ||
| const namingConvention = getNamingConvention(); | ||
|
|
||
| for (const ref of references) { | ||
| try { | ||
| // Preserve the original title before any naming convention transformations | ||
| const originalTitle = ref.title || 'Untitled Study'; | ||
|
|
||
| // Generate study name based on naming convention | ||
| const studyName = generateStudyName(ref, namingConvention); | ||
| // Use PDF filename if available, otherwise use title temporarily | ||
| const studyName = | ||
| ref.pdfFileName ? | ||
| getStudyNameFromFilename(ref.pdfFileName) | ||
| : ref.title || 'Untitled Study'; |
There was a problem hiding this comment.
Update misleading comment about PDF attachment.
The comment states "Studies require a PDF, so references need PDFs attached," but this function doesn't actually attach any PDFs—it only creates study records with metadata. The logic uses ref.pdfFileName for naming but doesn't upload or attach the corresponding PDF file.
Consider updating the comment to reflect the actual behavior or documenting that PDFs need to be uploaded separately.
🔎 Apply this diff to clarify the comment:
- // Handle importing references from Zotero/EndNote files
- // Note: Studies require a PDF, so references need PDFs attached
+ // Handle importing references from Zotero/EndNote files
+ // Note: Creates study records with metadata. PDFs must be uploaded separately.
const handleImportReferences = references => {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/primitives/useProjectStudyHandlers.js around lines 258 to
268, the comment "Studies require a PDF, so references need PDFs attached" is
misleading because this function only creates study metadata and uses
ref.pdfFileName for naming — it does not upload or attach PDFs. Update the
comment to state that the handler only creates study records and derives a study
name from ref.pdfFileName if present, and add a note that actual PDF
upload/attachment must be handled separately (or reference the function
responsible for uploads).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web/src/primitives/useProjectStudyHandlers.js (1)
255-255: Clarify comment about PDF handling.The comment suggests that PDFs are attached in this function, but
handleImportReferencesonly creates study records with metadata and derives study names from PDF filenames when available. It doesn't actually upload or attach PDF files.🔎 Suggested clarification:
- // Handle importing references from Zotero/EndNote files - // Note: Studies require a PDF, so references need PDFs attached + // Handle importing references from Zotero/EndNote files + // Note: Creates study records with metadata. Derives study name from PDF filename if available.
🧹 Nitpick comments (7)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (4)
56-68: Minor edge case in citation formatting.If only
journalhas a value (no author or year), the result would be"- Journal"with a leading dash. Consider conditionally adding the dash only when there's preceding content.🔎 Suggested fix
const citationLine = () => { const pdf = primaryPdf(); const parts = []; // Prefer PDF-level metadata const author = pdf?.firstAuthor || study().firstAuthor; const year = pdf?.publicationYear || study().publicationYear; const journal = pdf?.journal || study().journal; if (author) parts.push(author); if (year) parts.push(`(${year})`); - if (journal) parts.push(`- ${journal}`); + if (journal) parts.push(parts.length > 0 ? `- ${journal}` : journal); return parts.join(' '); };
85-94: Consider adding a default case to the switch statement.While currently only two menu items exist, a default case (even empty or with a comment) would make the code more maintainable when adding future menu actions.
🔎 Suggested fix
const handleMenuSelect = details => { switch (details.value) { case 'assign-reviewers': props.onAssignReviewers?.(); break; case 'delete': props.onDelete?.(); break; + default: + break; } };
129-133: Consider memoizingcitationLine()to avoid double computation.
citationLine()is called twice: once in thewhencondition and once for rendering. UsingcreateMemowould ensure it's computed only once per reactive update.As per coding guidelines, use
createMemofor derived values based on props or state to ensure reactive updates.🔎 Suggested fix
+import { Show, For, createMemo } from 'solid-js'; -import { Show, For } from 'solid-js';Then change the function to a memo:
- // Citation line: author, year, journal from primary PDF - const citationLine = () => { + // Citation line: author, year, journal from primary PDF (memoized) + const citationLine = createMemo(() => { const pdf = primaryPdf(); const parts = []; // Prefer PDF-level metadata const author = pdf?.firstAuthor || study().firstAuthor; const year = pdf?.publicationYear || study().publicationYear; const journal = pdf?.journal || study().journal; if (author) parts.push(author); if (year) parts.push(`(${year})`); if (journal) parts.push(`- ${journal}`); return parts.join(' '); - }; + });
136-149: Consider usingShowwithfallbackprop for cleaner structure.Using two separate
Showblocks works but thefallbackprop provides a more idiomatic pattern.🔎 Suggested fix
{/* Reviewer badges */} - <Show when={hasReviewers()}> + <Show + when={hasReviewers()} + fallback={<span class='text-xs text-gray-400 italic shrink-0'>No reviewers</span>} + > <div class='flex flex-wrap gap-1 shrink-0 cursor-default' data-selectable> <For each={assignedReviewers()}> {name => ( <span class='inline-flex items-center px-2 py-1 rounded-full text-xs font-medium bg-blue-100 text-blue-800 select-text cursor-text'> {name} </span> )} </For> </div> </Show> - <Show when={!hasReviewers()}> - <span class='text-xs text-gray-400 italic shrink-0'>No reviewers</span> - </Show>packages/web/src/components/project-ui/ProjectHeader.jsx (1)
104-111: Consider adding an accessible label to the description textarea.The textarea lacks a
labeloraria-labelattribute, which impacts screen reader accessibility. Consider adding anaria-labelto improve the experience for users relying on assistive technologies.🔎 Apply this diff to add an aria-label:
<textarea value={descriptionDraft()} onInput={e => setDescriptionDraft(e.target.value)} + aria-label="Project description" class='w-full px-3 py-2 border border-gray-300 rounded-md text-gray-700 focus:outline-none focus:ring-2 focus:ring-blue-500 resize-none disabled:bg-gray-100 disabled:cursor-not-allowed' rows={3} placeholder='Add a project description...' disabled={isSaving()} />packages/web/src/primitives/useProjectStudyHandlers.js (1)
78-96: Consider extracting duplicate error handling pattern.The try/catch blocks for adding PDF metadata (lines 78-96 and 204-221) follow an identical pattern: call
addPdfToStudy, catch errors, clean up orphaned PDFs viadeletePdf, and rethrow. Extracting this into a helper function would improve maintainability and reduce duplication.🔎 Example helper function:
/** * Adds PDF metadata to study with automatic orphan cleanup on failure */ const addPdfWithCleanup = async (studyId, pdfMetadata, tag) => { try { addPdfToStudy(studyId, pdfMetadata, tag); } catch (metaErr) { console.error('Failed to add PDF metadata:', metaErr); // Clean up orphaned file deletePdf(projectId, studyId, pdfMetadata.fileName).catch(console.warn); throw metaErr; } };Then replace both try/catch blocks with:
await addPdfWithCleanup(studyId, { key: importedFile.key, fileName: importedFile.fileName, size: importedFile.size, uploadedBy: user()?.id, uploadedAt: Date.now(), source: 'google-drive', }, 'primary');Also applies to: 204-221
packages/ui/src/index.d.ts (1)
119-142: Consider adding proper prop types incrementally.The re-exported components use
Component<any>anduseTourreturnsany, which reduces type safety. While acceptable for initial implementation, consider defining proper prop interfaces for frequently-used components as you iterate.As per coding guidelines, TypeScript definitions help catch errors early and improve developer experience.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/ui/package.json(1 hunks)packages/ui/src/index.d.ts(1 hunks)packages/web/src/components/project-ui/ProjectHeader.jsx(2 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx(1 hunks)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx(1 hunks)packages/web/src/primitives/useProjectStudyHandlers.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Do not use emojis in code, comments, or documentation
Follow standard JavaScript/SolidJS/Cloudflare best practices
Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Keep files small, focused, and modular. Extract sub-modules into a folder (e.g.,ComponentName/withindex.jsxand helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Each file should handle one coherent responsibility
Use Zod for schema and input validation
Files:
packages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/web/src/primitives/useProjectStudyHandlers.jspackages/ui/src/index.d.ts
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{js,jsx,ts,tsx}: Use responsive design principles for UI components
Keep files small, focused, and modular. If a file exceeds a high number of lines, extract sub-modules into a folder with index.jsx and helper components, or move complex logic into separate utility files or primitives
Each file should handle one coherent responsibility
Group related components in subdirectories with an index.js barrel export
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
Import stores directly where needed instead of passing values through multiple components
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 (when scoped to a feature)
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 (e.g., () => props.name) to ensure they are always up-to-date
Use createMemo for derived values based on props or state to ensure reactive updates
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{js,jsx,ts,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
Components should receive at most 1–5 props, and only for local configuration, not shared state. If a component would need more than 5 ...
Files:
packages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/web/src/primitives/useProjectStudyHandlers.js
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/web/src/components/**/*.{js,jsx,ts,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
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
Files:
packages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx
**/*.{js,jsx,ts,tsx,json,yaml,yml,toml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer using config files rather than hardcoding values
Files:
packages/web/src/components/project-ui/ProjectHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsxpackages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsxpackages/ui/package.jsonpackages/web/src/primitives/useProjectStudyHandlers.jspackages/ui/src/index.d.ts
🧠 Learnings (17)
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : Use Zag.js for UI components and design system. Zag components exist in `packages/web/src/components/zag/*` and should be reused
Applied to files:
packages/ui/package.jsonpackages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/components/zag/**/*.{js,jsx,ts,tsx} : Zag component 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 or debugging
Applied to files:
packages/ui/package.json
📚 Learning: 2025-12-16T18:27:52.769Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.769Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Group related components in subdirectories with an index.js barrel export
Applied to files:
packages/ui/package.json
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to **/{index,index.js,index.jsx,index.ts,index.tsx} : Group related components in subdirectories with an `index.js` barrel export
Applied to files:
packages/ui/package.json
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use Zod for schema and input validation
Applied to files:
packages/ui/package.json
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Applied to files:
packages/ui/package.jsonpackages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Keep files small, focused, and modular. Extract sub-modules into a folder (e.g., `ComponentName/` with `index.jsx` and helper components). Move complex logic into separate utility files or primitives. Split large forms into section components
Applied to files:
packages/ui/package.json
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Applied to files:
packages/ui/package.jsonpackages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:27:52.769Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.769Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Each file should handle one coherent responsibility
Applied to files:
packages/ui/package.json
📚 Learning: 2025-12-16T18:27:52.769Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.769Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use responsive design principles for UI components
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : Use responsive design principles for UI components
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : If a component would need more than 5 props, move the shared data into an external store, a primitive, or Solid context (when scoped to a feature)
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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 (e.g., () => props.name) to ensure they are always up-to-date
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : 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
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:27:52.770Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-16T18:27:52.770Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use Solid's createStore for complex state or state objects for better performance and reactivity
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : When you have complex state or state objects, use Solid's `createStore` for better performance and reactivity
Applied to files:
packages/ui/src/index.d.ts
📚 Learning: 2025-12-16T18:28:31.657Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T18:28:31.657Z
Learning: Applies to packages/web/src/components/**/*.{js,jsx,ts,tsx} : For UI icons, use the `solid-icons` library or SVGs only. Do not use emojis
Applied to files:
packages/ui/src/index.d.ts
🧬 Code graph analysis (4)
packages/web/src/components/project-ui/ProjectHeader.jsx (3)
packages/web/src/components/project-ui/ProjectContext.jsx (2)
useProjectContext(51-57)userRole(18-23)packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (1)
handleNameChange(50-54)packages/ui/src/index.d.ts (1)
Editable(14-14)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (3)
packages/web/src/components/project-ui/all-studies/study-card/StudyPdfSection.jsx (1)
study(29-29)packages/web/src/lib/referenceParser.js (1)
author(349-349)packages/ui/src/zag/Menu.jsx (1)
Menu(31-125)
packages/web/src/primitives/useProjectStudyHandlers.js (3)
packages/web/src/primitives/useProject/sync.js (1)
study(59-88)packages/web/src/api/better-auth-store.js (1)
user(147-170)packages/web/src/api/pdf-api.js (1)
deletePdf(113-127)
packages/ui/src/index.d.ts (3)
packages/ui/src/zag/Dialog.jsx (1)
useConfirmDialog(243-321)packages/ui/src/lib/cn.js (1)
cn(16-18)packages/ui/src/zag/Tour.jsx (1)
useTour(21-27)
⏰ 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 (19)
packages/web/src/components/project-ui/all-studies/study-card/StudyCardHeader.jsx (4)
1-17: LGTM!Clean documentation header and appropriate imports from
solid-js,solid-icons, and the UI library.
28-34: LGTM!Proper reactive accessor pattern for
study()andprimaryPdf(). The fallback topdfs[0]handles the case where no primary PDF exists, and the|| []guard handles missing pdfs array.
36-54: LGTM!Reviewer accessors and name change handler are well-implemented. The validation in
handleNameChangecorrectly checks for non-empty trimmed value and only updates when changed.
96-106: LGTM!The
handleHeaderClickimplementation correctly prevents toggle when interacting with buttons, inputs, menus, and editable areas. The selector is comprehensive and well-thought-out.packages/web/src/components/project-ui/all-studies/study-card/StudyCard.jsx (4)
1-12: LGTM!Clean documentation and appropriate imports. The component composition pattern is well-documented.
13-31: High prop count but acceptable for composition component.This component receives many props (~15), which typically exceeds the 1-5 guideline. However, as a thin composition wrapper that forwards callbacks to children, this pattern is reasonable. The alternative would be using context, but that might add unnecessary complexity for this localized component hierarchy.
Consider grouping related callbacks into objects (e.g.,
pdfActions,studyActions) if the prop list grows further.
35-53: LGTM!Good defensive check in
onOpenChangeto prevent redundant toggles. Thetriggeras a function returningStudyCardHeaderis the correct pattern for Collapsible integration.
54-68: LGTM!Clean prop forwarding to
StudyPdfSectionwith appropriate visual separation. All PDF-related callbacks are properly passed through.packages/web/src/components/project-ui/ProjectHeader.jsx (1)
28-49: LGTM! Proper async handling and error management.The description save flow correctly awaits the async update, handles errors with user feedback via toast, and only closes the edit panel on success. This properly addresses the concerns from previous reviews.
packages/ui/src/index.d.ts (9)
3-14: LGTM - Editable type definitions.The
EditablePropsinterface is well-defined with appropriate activation modes and variants.
16-25: LGTM - Collapsible type definitions.The
CollapsiblePropsinterface properly defines controlled/uncontrolled state management with a custom trigger render prop.
27-42: LGTM - Menu type definitions.The
MenuItemandMenuPropsinterfaces are comprehensive, including the newhideIndicatoroption for customizing menu appearance.
44-52: LGTM - Dialog type definitions.Standard dialog interface with appropriate props for controlled state and content.
72-84: LGTM - Select type definitions.The
SelectOptionandSelectPropsinterfaces properly define a controlled select component.
86-92: LGTM - Tooltip type definitions.The
TooltipPropsinterface appropriately allows both string and JSX content.
94-109: LGTM - Toast type definitions.The toast API is well-structured with a
Toastercomponent, imperativetoastermethods, and ashowToasthelper.
112-114: LGTM - useWindowDrag type definition.Clean primitive hook interface for managing window drag state.
117-117: LGTM - cn utility type definition.The
cnfunction signature correctly accepts conditional class values and matches the implementation usingclsxandtwMerge.packages/ui/package.json (1)
8-13: LGTM - Proper TypeScript support configuration.The changes correctly expose TypeScript declarations:
- Top-level
typesfield for backward compatibility- Conditional
exportswithtypesfor modern module resolution- Maintains
defaultexport for runtime codeThis follows standard practices for dual-mode packages with TypeScript support.
| export function useConfirmDialog(): { | ||
| open: () => boolean; | ||
| setOpen: (_open: boolean) => void; | ||
| confirm: () => Promise<boolean>; | ||
| }; |
There was a problem hiding this comment.
Fix incomplete and incorrect return type for useConfirmDialog.
The declared return type doesn't match the actual implementation shown in Dialog.jsx. The actual hook returns additional properties and has different signatures:
Based on the implementation in Dialog.jsx (lines 242-320):
isOpen: () => boolean(notopen: () => boolean)open: (dialogConfig) => Promise<boolean>(takes config, returns Promise, not a getter)close: () => void(missing)setLoading: (loading: boolean) => void(missing)ConfirmDialogComponent: () => JSX.Element(missing)dialogProps: () => {...}(missing)
Apply this diff to correct the return type:
-export function useConfirmDialog(): {
- open: () => boolean;
- setOpen: (_open: boolean) => void;
- confirm: () => Promise<boolean>;
-};
+export function useConfirmDialog(): {
+ isOpen: () => boolean;
+ open: (_config: {
+ title?: string;
+ description?: string;
+ confirmText?: string;
+ cancelText?: string;
+ variant?: 'default' | 'destructive';
+ }) => Promise<boolean>;
+ close: () => void;
+ setLoading: (_loading: boolean) => void;
+ ConfirmDialogComponent: () => JSX.Element;
+ dialogProps: () => {
+ open: boolean;
+ onOpenChange: (_newOpen: boolean) => void;
+ onConfirm: () => void;
+ loading: boolean;
+ title?: string;
+ description?: string;
+ confirmLabel?: string;
+ cancelLabel?: string;
+ destructive?: boolean;
+ };
+};🤖 Prompt for AI Agents
In packages/ui/src/index.d.ts around lines 66 to 70, the declared return type
for useConfirmDialog is incomplete and incorrect compared to Dialog.jsx; update
the type to match the implementation: replace the current shape with an object
exposing isOpen(): boolean, open(dialogConfig): Promise<boolean>, close(): void,
setLoading(loading: boolean): void, ConfirmDialogComponent(): JSX.Element, and
dialogProps(): { title?: string; description?: string; confirmText?: string;
cancelText?: string; hideCancel?: boolean; loading?: boolean; intent?: string; }
(adjust property names/types to match Dialog.jsx), ensuring imports/types (e.g.,
JSX.Element) are available in the d.ts file.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.