Robins i#50
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds multi-checklist-type support (ROBINS‑I alongside AMSTAR2), a checklist registry and metadata, ROBINS‑I data + comparison/reconciliation utilities, a complete ROBINS‑I UI suite, and pervasive type-aware wiring in UI, persistence, export, and PDF lifecycle cleanup. Changes
Sequence Diagram(s)mermaid UI->>Registry: createChecklistOfType(type, options) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
corates | 5c1e7c9 | Dec 15 2025, 04:37 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (17)
.github/copilot-instructions.md (1)
17-18: Minor style nit in file-size guideline bulletCurrent text uses two dashes in one bullet (
- **Keep files...** - If a file...), which reads a bit awkwardly. Consider splitting into two sentences or bullets to improve readability, but it’s not functionally important.packages/web/src/checklist-registry/types.js (1)
11-79: Checklist type metadata is well-structured; consider surfacing unknown-type issuesThe constants and metadata structure look good and align with the new multi-type flow. One thing to consider:
getChecklistMetadata(type)silently falls back to AMSTAR2 for unknown types, which could mask misconfigured type IDs. You might optionally log a warning (in dev) or assert whenCHECKLIST_METADATA[type]is missing so unexpected types are easier to spot while still defaulting in production.packages/web/src/ROBINS-I/checklist-compare.js (1)
204-247: Reuse shared checklist type constants and consider a small deep-clone helperTwo small cleanups to consider here:
createReconciledChecklisthardcodeschecklistType: 'ROBINS_I'. Since you already defineCHECKLIST_TYPES.ROBINS_Iinchecklist-registry/types.js, importing and using that constant here would avoid string duplication and keep type IDs in sync if they ever change.- You repeatedly deep-clone via
JSON.parse(JSON.stringify(...))for structural fields and reconciled answers. Given these objects are plain data, this is OK, but you might want to centralize that into a tinydeepClone(value)utility for readability and to keep the cloning strategy consistent across ROBINS-I helpers.Both are non-blocking, just quality-of-life improvements.
Also applies to: 296-343
packages/web/src/ROBINS-I/modular-checklist-plan.md (1)
1-447: Architecture plan is clear; remember to keep milestones in sync with realityThis is a helpful high-level map of the registry/GenericChecklist approach and future types. As pieces land (e.g., registry foundation, local/cloud integration), it’ll be worth periodically updating the checklists and examples here so the doc reflects the current implementation and doesn’t drift.
packages/web/src/primitives/useProjectChecklistHandlers.js (1)
18-26: Broaden success check forcreateChecklistresultReturning
checklistId !== nullassumes failures are always signaled asnull. IfcreateChecklistever returnsundefinedor another falsy value on failure, this would incorrectly report success. A slightly safer pattern would be:- const checklistId = await createChecklist(studyId, type, assigneeId); - return checklistId !== null; + const checklistId = await createChecklist(studyId, type, assigneeId); + return !!checklistId;assuming valid IDs are always truthy. This keeps the same intent but is more robust to implementation details of
createChecklist.packages/web/src/components/project-ui/ChecklistForm.jsx (1)
6-16: Registry-based checklist type wiring looks goodUsing
DEFAULT_CHECKLIST_TYPEandgetChecklistTypeOptions()keeps this form aligned with the new registry and multi-type flow, and the<For>rendering of value/label-description options is clean. If you later surface submission errors in this component, you might consider only resettingtypeafter a confirmed successfulonSubmitso the user doesn’t lose a non-default selection on failure, but current behavior is reasonable.Also applies to: 28-33
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIDemo.jsx (1)
1-4: Demo state and scoring integration look correctThe store-based checklist state,
onUpdatewiring intoROBINSIChecklist, and header color-coding fromscoreChecklist(checklist)all look solid for a demo. If scoring ever becomes heavier, you could wrap it in acreateMemo(and derive the colour class from that memo) to avoid recomputing on everycurrentScore()call in the JSX, but that’s an optional refinement for now.Also applies to: 9-25, 39-47
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (1)
164-185: Consider reusing the sharedJudgementBadgeto avoid divergenceThere’s already a
JudgementBadgeexported fromDomainJudgement.jsx. If the intent is for judgement badges to look consistent across the checklist, it may be cleaner to import and reuse that implementation here rather than maintaining a second copy.packages/web/src/components/checklist-ui/GenericChecklist.jsx (1)
8-22: Type-aware lazy loading looks good; add an explicit fallback for unknown typesThe memoized
checklistTyperesolution and lazy loading of AMSTAR2 vs ROBINS_I components are a clean fit for the registry. Right now, if a future type value slips through without a matching<Show>block, this component will silently render nothing. Consider adding an explicit “unsupported type” fallback (or defaulting back toDEFAULT_CHECKLIST_TYPE) so such cases fail loudly and are easier to diagnose.Also applies to: 48-79
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1)
9-10: Async, type-aware scoring and propagation look solidDeriving
checklistType()viagetChecklistTypeFromStateand usingcreateResourcewithscoreChecklistOfTypeis a clean way to make scoring non-blocking and type-aware. ThreadingchecklistType()through to bothScoreTagandChecklistWithPdfkeeps the header UI and embedded checklist in sync with the registry metadata. If you ever need to distinguish “score still computing” from “no score”, you could optionally expose the resource’s loading state to render a subtle placeholder aroundScoreTag.Also applies to: 14-17, 144-158, 171-177, 205-207
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
1-1: Type/score integration is good; align the default and clarify ROBINS-I behaviorThe new
checklistTypememo andcurrentScoreresource correctly reusegetChecklistTypeFromStateandscoreChecklistOfType, and passingchecklistType()into bothScoreTagandChecklistWithPdfkeeps this wrapper consistent with the registry.Two follow-ups to tighten things up:
- Instead of hard-coding
'AMSTAR2'as the fallback inchecklistType, it’s safer to use the registry’s default:-import { getChecklistTypeFromState, scoreChecklistOfType } from '@/checklist-registry'; +import { getChecklistTypeFromState, scoreChecklistOfType, DEFAULT_CHECKLIST_TYPE } from '@/checklist-registry'; @@ - const ui = checklistForUI(); - if (ui) return getChecklistTypeFromState(ui); - return 'AMSTAR2'; + const ui = checklistForUI(); + if (ui) return getChecklistTypeFromState(ui); + return DEFAULT_CHECKLIST_TYPE;
handlePartialUpdateis still AMSTAR2-specific (it only forwards keys matching^q\d+[a-z]*$intoupdateChecklistAnswer). Now thatChecklistWithPdfcan render ROBINS_I viaGenericChecklist, any ROBINS_I checklist that ever uses this wrapper would not have its edits persisted. If ROBINS_I isn’t intended for Yjs-backed/project checklists yet, that’s fine, but it’d be good to either guard against that type here or extendonUpdatehandling to support the ROBINS_IonUpdate(key, value)shape when you add that support.Also applies to: 10-10, 159-176, 205-212, 214-221, 223-239, 285-287
packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
94-111:ResponseLegendhardcodes response codes that should align withRESPONSE_LABELS.The
commonResponsesarray is hardcoded and matches the keys inRESPONSE_LABELSfrom the external context. This is acceptable, but consider deriving the list fromRESPONSE_LABELSkeys to ensure consistency if labels are added/removed in the future:- const commonResponses = ['Y', 'PY', 'PN', 'N', 'NI', 'NA', 'WN', 'SN', 'SY', 'WY']; + const commonResponses = Object.keys(RESPONSE_LABELS);packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
31-59: Consider extracting shared color helper functions.Both
OverallSectionandDomainJudgementdefine similargetJudgementColorfunctions with overlapping cases.DomainSectionalso has agetColorhelper (per the relevant snippets). Consider extracting these to a shared utility:// In a shared utils file, e.g., judgement-colors.js export const getJudgementColor = (judgement, options = {}) => { const { withBorder = true } = options; switch (judgement) { case 'Low': case 'Low (except confounding)': return withBorder ? 'bg-green-100 border-green-400 text-green-800' : 'bg-green-100 text-green-800'; // ... other cases } };This would reduce duplication across 3+ files.
packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (1)
15-26: RedundantstopAssessmentrecalculation.The
stopAssessmentflag is set innewStateon line 24, but there's also acreateMemoon line 13 that computesstopAssessment()fromprops.sectionBState. The memo will automatically recompute when the parent passes the updated state, making line 24 potentially redundant depending on how the state is consumed downstream.If
newState.stopAssessmentis used by downstream consumers (e.g., persisted or checked before the parent updates), keep it. Otherwise, consider removing it to avoid confusion:const newState = { ...props.sectionBState, [questionKey]: { ...props.sectionBState[questionKey], answer: value, }, }; - // Update stopAssessment flag - newState.stopAssessment = shouldStopAssessment(newState); props.onUpdate(newState);packages/web/src/primitives/useProject/checklists.js (1)
60-81: HardcodedrobinsKeysarray may need maintenance as ROBINS-I evolves.The list of keys to extract for ROBINS-I is hardcoded. If the ROBINS-I template structure changes, this array must be manually updated. Consider extracting this list to a constant in the checklist-registry or deriving it from the template:
// Option 1: Export from registry import { ROBINS_I_KEYS } from '@/checklist-registry'; // Option 2: Extract non-metadata keys dynamically const metadataKeys = ['id', 'name', 'createdAt', 'checklistType']; Object.entries(checklistTemplate).forEach(([key, value]) => { if (!metadataKeys.includes(key)) { answersData[key] = value; } });packages/web/src/components/checklist-ui/ScoreTag.jsx (1)
65-71: Consider memoizing checklist metadata to avoid redundant lookups.The
aria-labelon line 71 callsgetChecklistMetadata(checklistType())directly, whiletooltipContent()also fetches metadata internally. Consider memoizing the metadata once:+ const metadata = createMemo(() => getChecklistMetadata(checklistType())); const styleClass = createMemo(() => getScoreStyle(props.currentScore, checklistType())); - const infoUrl = createMemo(() => getInfoUrl(checklistType())); - const tooltipContent = createMemo(() => getTooltipContent(checklistType())); + const infoUrl = createMemo(() => metadata().url || AMSTAR2_URL); + const tooltipContent = createMemo(() => `Open ${metadata().shortName || metadata().name} scoring guide`);Then use
metadata().namein the aria-label.packages/web/src/ROBINS-I/checklist-map.js (1)
73-79: Potential naming conflict withCHECKLIST_TYPESfrom registry.This file exports
CHECKLIST_TYPESwith a different structure (object with metadata) thanpackages/web/src/checklist-registry/types.jswhich exportsCHECKLIST_TYPESas an enum-like constant ({ AMSTAR2: 'AMSTAR2', ROBINS_I: 'ROBINS_I' }).Consider renaming this to
ROBINS_I_TYPE_INFOor similar to avoid confusion when both are imported in the same file.#!/bin/bash # Check if both CHECKLIST_TYPES are ever imported together rg -l "from.*checklist-map" packages/web/src/ | xargs -I{} rg -l "from.*checklist-registry" {} 2>/dev/null || echo "No files import both"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.github/copilot-instructions.md(1 hunks)packages/web/src/ROBINS-I/ROBINS-I.md(7 hunks)packages/web/src/ROBINS-I/checklist-compare.js(1 hunks)packages/web/src/ROBINS-I/checklist-map.js(1 hunks)packages/web/src/ROBINS-I/checklist.js(1 hunks)packages/web/src/ROBINS-I/modular-checklist-plan.md(1 hunks)packages/web/src/Routes.jsx(2 hunks)packages/web/src/checklist-registry/index.js(1 hunks)packages/web/src/checklist-registry/types.js(1 hunks)packages/web/src/components/checklist-ui/ChecklistWithPdf.jsx(2 hunks)packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(5 hunks)packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx(3 hunks)packages/web/src/components/checklist-ui/GenericChecklist.jsx(1 hunks)packages/web/src/components/checklist-ui/LocalChecklistView.jsx(4 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIDemo.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/index.js(1 hunks)packages/web/src/components/checklist-ui/ScoreTag.jsx(1 hunks)packages/web/src/components/project-ui/ChecklistForm.jsx(2 hunks)packages/web/src/components/project-ui/tabs/completed/index.js(1 hunks)packages/web/src/primitives/useLocalChecklists.js(2 hunks)packages/web/src/primitives/useProject/checklists.js(4 hunks)packages/web/src/primitives/useProjectChecklistHandlers.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
packages/web/src/checklist-registry/types.js (1)
packages/web/src/components/project-ui/ChecklistForm.jsx (1)
type(9-9)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
packages/web/src/ROBINS-I/checklist-map.js (3)
getResponseOptions(468-470)RESPONSE_LABELS(17-28)RESPONSE_LABELS(17-28)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (3)
packages/web/src/ROBINS-I/checklist-map.js (4)
domain(452-452)ROBINS_I_CHECKLIST(428-437)ROBINS_I_CHECKLIST(428-437)getDomainQuestions(451-465)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (3)
JudgementBadge(125-147)DomainJudgement(15-120)getColor(126-140)packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
SignallingQuestion(13-89)
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIDemo.jsx (1)
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1)
handleUpdate(84-104)
packages/web/src/Routes.jsx (1)
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIDemo.jsx (1)
ROBINSIDemo(8-77)
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx (5)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (2)
stopAssessment(13-13)SectionB(12-138)packages/web/src/ROBINS-I/checklist.js (4)
shouldStopAssessment(147-155)isPerProtocol(171-171)isPerProtocol(282-282)isPerProtocol(311-311)packages/web/src/ROBINS-I/checklist-map.js (1)
getActiveDomainKeys(445-448)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (1)
DomainSection(17-162)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
OverallSection(13-167)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (2)
packages/web/src/ROBINS-I/checklist-map.js (6)
DOMAIN1_DIRECTIONS(53-53)DOMAIN1_DIRECTIONS(53-53)BIAS_DIRECTIONS(42-50)BIAS_DIRECTIONS(42-50)ROB_JUDGEMENTS(31-31)ROB_JUDGEMENTS(31-31)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (1)
getColor(166-179)
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (3)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (2)
checklistType(206-212)currentScore(215-221)packages/web/src/components/checklist-ui/ScoreTag.jsx (2)
checklistType(53-53)ScoreTag(52-79)packages/web/src/checklist-registry/index.js (3)
checklist(93-93)getChecklistTypeFromState(124-143)scoreChecklistOfType(104-107)
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (3)
packages/web/src/ROBINS-I/checklist.js (1)
scoreChecklist(162-202)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (1)
getJudgementColor(18-31)packages/web/src/ROBINS-I/checklist-map.js (4)
OVERALL_ROB_JUDGEMENTS(34-39)OVERALL_ROB_JUDGEMENTS(34-39)BIAS_DIRECTIONS(42-50)BIAS_DIRECTIONS(42-50)
packages/web/src/checklist-registry/index.js (1)
packages/web/src/checklist-registry/types.js (4)
CHECKLIST_TYPES(11-18)CHECKLIST_TYPES(11-18)DEFAULT_CHECKLIST_TYPE(58-58)DEFAULT_CHECKLIST_TYPE(58-58)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (2)
packages/web/src/components/checklist-ui/ScoreTag.jsx (2)
checklistType(53-53)ScoreTag(52-79)packages/web/src/checklist-registry/index.js (3)
checklist(93-93)getChecklistTypeFromState(124-143)scoreChecklistOfType(104-107)
packages/web/src/ROBINS-I/checklist-compare.js (1)
packages/web/src/ROBINS-I/checklist-map.js (5)
ROBINS_I_CHECKLIST(428-437)ROBINS_I_CHECKLIST(428-437)getActiveDomainKeys(445-448)domain(452-452)getDomainQuestions(451-465)
packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx (1)
packages/web/src/checklist-registry/index.js (1)
checklist(93-93)
packages/web/src/components/checklist-ui/GenericChecklist.jsx (3)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
checklistType(206-212)packages/web/src/checklist-registry/index.js (1)
getChecklistTypeFromState(124-143)packages/web/src/checklist-registry/types.js (4)
DEFAULT_CHECKLIST_TYPE(58-58)DEFAULT_CHECKLIST_TYPE(58-58)CHECKLIST_TYPES(11-18)CHECKLIST_TYPES(11-18)
packages/web/src/ROBINS-I/checklist-map.js (4)
packages/web/src/ROBINS-I/checklist-compare.js (2)
isPerProtocol(41-41)isPerProtocol(205-205)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx (1)
isPerProtocol(26-26)packages/web/src/ROBINS-I/checklist.js (5)
isPerProtocol(171-171)isPerProtocol(282-282)isPerProtocol(311-311)domain(133-133)domain(177-177)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (1)
domain(18-18)
packages/web/src/primitives/useProject/checklists.js (2)
packages/web/src/checklist-registry/index.js (1)
createChecklistOfType(91-96)packages/web/src/checklist-registry/types.js (2)
CHECKLIST_TYPES(11-18)CHECKLIST_TYPES(11-18)
packages/web/src/components/checklist-ui/ChecklistWithPdf.jsx (1)
packages/web/src/components/checklist-ui/GenericChecklist.jsx (1)
GenericChecklist(48-80)
🪛 Biome (2.1.2)
packages/web/src/ROBINS-I/checklist.js
[error] 416-416: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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/web/src/components/project-ui/tabs/completed/index.js (1)
1-3: Completed tab barrel export is consistent and clearRe-exports remain straightforward and keep the tab API tidy; no issues here.
packages/web/src/ROBINS-I/ROBINS-I.md (1)
1-238: Confirm ROBINS-I content use is compatible with CC BY‑NC‑ND 4.0You’re embedding substantial ROBINS-I wording under a Creative Commons Attribution–NonCommercial–NoDerivatives 4.0 license. The notice is present, which is good, but it’d be wise to double-check that reproducing this text (and any edits you’ve made) in this repository is compatible with the “NoDerivatives” and “NonCommercial” terms.
packages/web/src/Routes.jsx (1)
19-20: ROBINS-I demo route wiring looks correctImporting
ROBINSIDemoand exposing it at/demo/robins-iunder the main layout is a clean way to surface the new checklist UI for experimentation and demos; routing shape looks consistent with the rest of the app.Also applies to: 35-39
packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx (1)
6-7: Local checklist creation is correctly type-awareThe added
checklistTypesignal, use ofDEFAULT_CHECKLIST_TYPE/getChecklistTypeOptions(), and passingchecklistType()intocreateChecklistall look consistent with the new registry model. The “Assessment Type” select is wired correctly and keeps the chosen type through errors, so users won’t lose their selection if creation fails.Also applies to: 12-13, 19-26, 50-53, 90-110
packages/web/src/components/checklist-ui/ROBINSIChecklist/index.js (1)
1-7: LGTM!Clean barrel file that provides a well-organized public API for the ROBINS-I checklist components. The combination of default and named exports allows flexible import patterns.
packages/web/src/primitives/useLocalChecklists.js (1)
122-138: Field override risk with spread operator placement.The
templateis spread first, then explicit fields are set. This is correct - explicit fields will override template values. However, ifcreateChecklistOfTypereturns achecklistTypefield (which it does according to the registry), it will be correctly overridden by the explicitchecklistType: typeon line 134.packages/web/src/components/checklist-ui/ChecklistWithPdf.jsx (1)
32-38: LGTM!Clean refactor replacing the AMSTAR2-specific component with the type-aware
GenericChecklist. The component correctly passes throughchecklistType,checklist,onUpdate, andreadOnlyprops, allowing the generic wrapper to render the appropriate checklist UI.packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
13-89: LGTM!Well-structured component with:
- Proper SolidJS reactive patterns using getter functions for
options()- Accessible radio inputs with
sr-onlyclass and visible labels- Defensive optional chaining for answer state
- Clean separation of answer and comment update handlers
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
61-166: LGTM!The component correctly:
- Computes and displays the calculated score from domain judgements
- Allows manual override of the overall judgement
- Provides optional direction of bias selection with a clear button
- Uses proper SolidJS patterns (
For,Show, reactive memos)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (2)
15-120: LGTM!Well-implemented domain judgement selector:
- Correctly differentiates direction options for Domain 1 vs other domains
- Uses
props.domainIdfor unique radio group names to prevent conflicts when multiple domains render- Properly guards optional
onDirectionChangecallback with optional chaining- Clear button implementation is correct
125-147:JudgementBadgehandles both 'Low' and 'Low (except confounding)'.Good defensive implementation - the badge correctly maps both regular domain judgement ('Low') and overall judgement ('Low (except confounding)') to the same green color. The fallback "Not assessed" text is also helpful for unset states.
packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (1)
40-137: LGTM!Well-structured Section B implementation:
- Clear visual hierarchy with question text, info hints, and response options
- Proper warning panel when assessment should stop (B2/B3 = Yes/Probably Yes)
- Accessible radio inputs with labels showing both code and full text
- Comment fields for each question
packages/web/src/primitives/useProject/checklists.js (1)
23-85: Good error handling for async template creation.The try/catch block properly handles failures from
createChecklistOfType, logs the error, and returnsnullto signal failure to the caller. This prevents crashes and allows callers to handle the error appropriately.packages/web/src/components/checklist-ui/ScoreTag.jsx (1)
53-57: Good use of memoization for derived values.The pattern of using
createMemoforstyleClass,infoUrl, andtooltipContentis appropriate for SolidJS reactive computations. However, note thatchecklistTypeon line 53 is a simple accessor function (not a memo), which is fine since it's just reading a prop.packages/web/src/checklist-registry/index.js (1)
124-143: Structural type detection is a good fallback strategy.The function properly prioritizes explicit type fields before falling back to structural detection. The order of checks (explicit → structural → default) is logical.
One edge case to be aware of: if malformed data contains both ROBINS-I fields (like
sectionB) and AMSTAR2 fields (likeq1), the ROBINS-I check will match first. This is likely fine given ROBINS-I is the newer format, but worth documenting.packages/web/src/ROBINS-I/checklist.js (3)
21-46: Solid input validation with clear error messages.The validation for
idandnameparameters is thorough, checking for presence, type, and non-empty strings. The date formatting fallback (lines 34-38) handles invalid timestamps gracefully.
162-202: Scoring algorithm correctly implements ROBINS-I hierarchy.The scoring logic properly:
- Returns 'Critical' for early-stopped assessments
- Returns 'Incomplete' when required judgements are missing
- Applies the severity hierarchy (Critical > Serious > Moderate > Low)
This matches the ROBINS-I guidance where the overall risk is determined by the worst domain judgement.
211-243: Heuristic suggestion function is appropriately documented.The docstring correctly states this is a "helper/suggestion" and that "final judgement is made by reviewer." The simple heuristic provides a starting point without being prescriptive. The comment on line 237 acknowledging domain-specific algorithms in the official guidance is important context.
packages/web/src/ROBINS-I/checklist-map.js (3)
218-288: Domain 3's subsection structure is well-designed.The subsection pattern for Domain 3 (Selection of Participants) with parts A, B, and C provides good organizational clarity. The
getDomainQuestionsfunction correctly flattens these subsections when needed.
444-469: Utility functions are correct and well-structured.
getActiveDomainKeyscorrectly returnsdomain1bfor per-protocol studies anddomain1aotherwise, along with the shared domains 2-6.getDomainQuestionsproperly handles the subsection flattening for Domain 3.getResponseOptionsfalls back toWITH_NIfor unknown response types, which is a sensible default.
4-28: Response type definitions are comprehensive.The
RESPONSE_TYPESandRESPONSE_LABELSprovide complete coverage for all ROBINS-I V2 response options including the nuanced "Weak/Strong" variants. The structure allows questions to reference appropriate option sets by key.
| const domain = () => ROBINS_I_CHECKLIST[props.domainKey]; | ||
| const questions = () => getDomainQuestions(props.domainKey); | ||
| const hasSubsections = () => !!domain()?.subsections; | ||
|
|
||
| function handleQuestionUpdate(questionKey, newAnswer) { | ||
| const newAnswers = { | ||
| ...props.domainState.answers, | ||
| [questionKey]: newAnswer, | ||
| }; | ||
| props.onUpdate({ | ||
| ...props.domainState, | ||
| answers: newAnswers, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Harden domainState handling and avoid assuming answers is always present
This component assumes props.domainState and props.domainState.answers are always defined. If a persisted checklist ever lacks a domain or its answers object (e.g. from partial/migrated data), handleQuestionUpdate will throw when spreading props.domainState.answers. You can make this more robust with a small guard:
function handleQuestionUpdate(questionKey, newAnswer) {
- const newAnswers = {
- ...props.domainState.answers,
- [questionKey]: newAnswer,
- };
- props.onUpdate({
- ...props.domainState,
- answers: newAnswers,
- });
+ const prevDomain = props.domainState || {};
+ const prevAnswers = prevDomain.answers || {};
+ props.onUpdate({
+ ...prevDomain,
+ answers: {
+ ...prevAnswers,
+ [questionKey]: newAnswer,
+ },
+ });
}You might also compute const status = completionStatus(); once in the header and use status.answered/status.total to avoid recalculating it twice, but that’s just a small readability/perf tweak.
Also applies to: 47-55, 108-117, 130-139
🤖 Prompt for AI Agents
In packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx
around lines 18-31 (and similarly apply to lines 47-55, 108-117, 130-139), the
component assumes props.domainState and props.domainState.answers always exist
which can throw when spreading undefined; change the handlers and any places
that spread or read answers to defensively default domainState to an object and
answers to an empty object (e.g., use nullish coalescing or logical OR) before
spreading/reading, and compute completionStatus() once and reuse that result in
the header to avoid duplicate calls.
| function handleSectionCToggle() { | ||
| props.onUpdate('sectionC', { | ||
| ...props.checklistState.sectionC, | ||
| isPerProtocol: !isPerProtocol(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add a small guard around sectionC when toggling protocol type
handleSectionCToggle currently spreads props.checklistState.sectionC directly. As long as createChecklist always initializes sectionC this is fine, but a missing sectionC field in persisted data would crash here. You can defensively default it:
function handleSectionCToggle() {
- props.onUpdate('sectionC', {
- ...props.checklistState.sectionC,
- isPerProtocol: !isPerProtocol(),
- });
+ const prev = props.checklistState?.sectionC || {};
+ props.onUpdate('sectionC', {
+ ...prev,
+ isPerProtocol: !isPerProtocol(),
+ });
}🤖 Prompt for AI Agents
In
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx
around lines 35-40, the handler spreads props.checklistState.sectionC directly
which will throw if sectionC is missing from persisted data; change the update
to defensively default sectionC to an object (e.g., const sectionC =
props.checklistState.sectionC || {}), then spread that defaulted object when
calling props.onUpdate and compute isPerProtocol using a safe boolean coercion
(e.g., !Boolean(sectionC.isPerProtocol)) so the toggle works even when sectionC
was undefined.
| export function compareChecklists(checklist1, checklist2) { | ||
| if (!checklist1 || !checklist2) { | ||
| return { | ||
| sectionB: { agreements: [], disagreements: [] }, | ||
| domains: {}, | ||
| stats: { total: 0, agreed: 0, disagreed: 0 }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Guard reconciliation summary against missing overall and agreementRate
If compareChecklists is called with a null/undefined checklist, it returns an object without overall and without stats.agreementRate, but getReconciliationSummary assumes both exist. That can lead to a runtime error (overall.judgementMatch on undefined) or NaN for agreementPercentage.
You can make the comparison output shape consistent and robust with something like:
export function compareChecklists(checklist1, checklist2) {
if (!checklist1 || !checklist2) {
return {
- sectionB: { agreements: [], disagreements: [] },
- domains: {},
- stats: { total: 0, agreed: 0, disagreed: 0 },
+ sectionB: { agreements: [], disagreements: [] },
+ domains: {},
+ overall: {
+ judgementMatch: true,
+ directionMatch: true,
+ reviewer1: null,
+ reviewer2: null,
+ },
+ stats: { total: 0, agreed: 0, disagreed: 0, agreementRate: 0 },
};
}
…
}and make getReconciliationSummary compute the percentage from stats directly and tolerate a missing overall just in case:
export function getReconciliationSummary(comparison) {
const { stats, sectionB, domains, overall } = comparison;
+ const agreementRate =
+ stats && typeof stats.total === 'number' && stats.total > 0
+ ? stats.agreed / stats.total
+ : 0;
const domainDisagreements = [];
const judgementDisagreements = [];
…
return {
totalQuestions: stats.total,
agreementCount: stats.agreed,
disagreementCount: stats.disagreed,
- agreementPercentage: Math.round(stats.agreementRate * 100),
+ agreementPercentage: Math.round(agreementRate * 100),
sectionBDisagreements: sectionB.disagreements.length,
domainDisagreements,
judgementDisagreements,
- overallDisagreement: !overall.judgementMatch,
- needsReconciliation: stats.disagreed > 0 || !overall.judgementMatch,
+ overallDisagreement: overall ? !overall.judgementMatch : false,
+ needsReconciliation:
+ stats.disagreed > 0 || (overall ? !overall.judgementMatch : false),
};
}Also applies to: 379-409
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (1)
256-260: UsechecklistType()for consistent type display.Line 258 displays the checklist type using
currentChecklist()?.type || 'AMSTAR2', which differs from thechecklistType()memo (lines 230-237) that includes state detection logic. This inconsistency could display the wrong type to users when checklist metadata lacks a type field but the state indicates a different type (e.g., ROBINS-I).Apply this diff to align the display with the rest of the component:
<span class='text-gray-900 font-medium'> - {currentChecklist()?.type || 'AMSTAR2'} Checklist + {checklistType()} Checklist </span>
♻️ Duplicate comments (3)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (2)
74-76: Compute completion status once to avoid duplicate calls.As previously noted, calling
completionStatus()twice on line 75 is inefficient. Compute it once at the beginning of the header block and reuse the result.<div class='flex items-center gap-3'> {/* Completion indicator */} + {(() => { + const status = completionStatus(); + return ( <span class='text-xs text-gray-400'> - {completionStatus().answered}/{completionStatus().total} + {status.answered}/{status.total} </span> + ); + })()}Alternatively, if you prefer a cleaner approach, you could use a SolidJS
createMemo:+import { For, Show, createMemo } from 'solid-js'; ... export function DomainSection(props) { const domain = () => ROBINS_I_CHECKLIST[props.domainKey]; const questions = () => getDomainQuestions(props.domainKey); const hasSubsections = () => !!domain()?.subsections; + + const completionStatus = createMemo(() => { + const qs = questions(); + const answered = Object.keys(qs).filter( + k => props.domainState?.answers?.[k]?.answer !== null, + ).length; + const total = Object.keys(qs).length; + return { answered, total }; + });Then use
completionStatus().answeredandcompletionStatus().totalin the JSX, which will now refer to the memoized value.
22-31: Address the previously flagged robustness issue in handler functions.The past review comment about defensive handling of
props.domainStateandprops.domainState.answersremains unaddressed. This handler (and similar ones at lines 33-38 and 40-45) will throw ifdomainStateor itsanswersproperty are undefined during partial data scenarios.Apply the previously suggested fix:
function handleQuestionUpdate(questionKey, newAnswer) { - const newAnswers = { - ...props.domainState.answers, - [questionKey]: newAnswer, - }; - props.onUpdate({ - ...props.domainState, - answers: newAnswers, - }); + const prevDomain = props.domainState || {}; + const prevAnswers = prevDomain.answers || {}; + props.onUpdate({ + ...prevDomain, + answers: { + ...prevAnswers, + [questionKey]: newAnswer, + }, + }); }Similarly for
handleJudgementChangeandhandleDirectionChange:function handleJudgementChange(judgement) { + const prevDomain = props.domainState || {}; props.onUpdate({ - ...props.domainState, + ...prevDomain, judgement, }); } function handleDirectionChange(direction) { + const prevDomain = props.domainState || {}; props.onUpdate({ - ...props.domainState, + ...prevDomain, direction, }); }packages/web/src/ROBINS-I/checklist-compare.js (1)
41-48: Guard reconciliation summary against missingoverallandagreementRate.The early return when checklists are null/undefined doesn't include
overallorstats.agreementRate, butgetReconciliationSummary(lines 417-418) assumes both exist. This will cause runtime errors when called with comparison results from null inputs.Apply this diff to make the early return shape consistent:
export function compareChecklists(checklist1, checklist2) { if (!checklist1 || !checklist2) { return { sectionB: { agreements: [], disagreements: [] }, domains: {}, - stats: { total: 0, agreed: 0, disagreed: 0 }, + overall: { + judgementMatch: true, + directionMatch: true, + reviewer1: null, + reviewer2: null, + }, + stats: { total: 0, agreed: 0, disagreed: 0, agreementRate: 0 }, }; }
🧹 Nitpick comments (8)
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1)
144-157: Consider memoizingchecklistTypefor consistency.The
checklistTypefunction (lines 145-149) is a plain function, while similar patterns inGenericChecklist.jsxandChecklistYjsWrapper.jsxusecreateMemo. Although this works due to SolidJS's fine-grained reactivity, usingcreateMemowould be more consistent with the codebase and avoid redundant calls togetChecklistTypeFromState.- // Get the checklist type from the loaded checklist - const checklistType = () => { - const curr = checklist(); - if (!curr) return null; - return getChecklistTypeFromState(curr); - }; + // Get the checklist type from the loaded checklist + const checklistType = createMemo(() => { + const curr = checklist(); + if (!curr) return null; + return getChecklistTypeFromState(curr); + });packages/web/src/ROBINS-I/checklist-compare.js (3)
214-232: Add null guard for source checklist IDs.Line 224 accesses
checklist1.idandchecklist2.iddirectly, but if either checklist lacks anidfield,sourceChecklistswill containundefined. Consider filtering or validating:- sourceChecklists: [checklist1.id, checklist2.id], + sourceChecklists: [checklist1.id, checklist2.id].filter(Boolean),
276-296: Use thedeepClonehelper for consistency.This function uses inline
JSON.parse(JSON.stringify(...))whiledeepCloneis defined at the top of the file. Using the helper improves readability and consistency.if (!selection || selection === 'reviewer1') { - reconciled[key] = JSON.parse( - JSON.stringify(section1?.[key] || { answer: null, comment: '' }), - ); + reconciled[key] = deepClone(section1?.[key] || { answer: null, comment: '' }); } else if (selection === 'reviewer2') { - reconciled[key] = JSON.parse( - JSON.stringify(section2?.[key] || { answer: null, comment: '' }), - ); + reconciled[key] = deepClone(section2?.[key] || { answer: null, comment: '' }); } else if (typeof selection === 'object') { - reconciled[key] = JSON.parse(JSON.stringify(selection)); + reconciled[key] = deepClone(selection); }
389-420: Add defensive checks foroverallaccess.Even after fixing the early return in
compareChecklists, defensive coding here would prevent future regressions:return { totalQuestions: stats.total, agreementCount: stats.agreed, disagreementCount: stats.disagreed, - agreementPercentage: Math.round(stats.agreementRate * 100), + agreementPercentage: Math.round((stats.agreementRate || 0) * 100), sectionBDisagreements: sectionB.disagreements.length, domainDisagreements, judgementDisagreements, - overallDisagreement: !overall.judgementMatch, - needsReconciliation: stats.disagreed > 0 || !overall.judgementMatch, + overallDisagreement: overall ? !overall.judgementMatch : false, + needsReconciliation: stats.disagreed > 0 || (overall ? !overall.judgementMatch : false), };packages/web/src/components/checklist-ui/GenericChecklist.jsx (2)
40-58: Consider adding a fallback for unknown checklist types.If
checklistType()returns an unrecognized value (e.g., a future type not yet implemented), the component renders nothing silently. Adding a fallback or warning would improve debuggability.<Show when={checklistType() === CHECKLIST_TYPES.ROBINS_I}> <ROBINSIChecklist checklistState={props.checklist} onUpdate={props.onUpdate} showComments={true} showLegend={true} readOnly={props.readOnly} /> </Show> + <Show when={![CHECKLIST_TYPES.AMSTAR2, CHECKLIST_TYPES.ROBINS_I].includes(checklistType())}> + <div class="p-4 text-amber-600 bg-amber-50 rounded"> + Unknown checklist type: {checklistType()} + </div> + </Show> </>
49-57: HardcodedshowCommentsandshowLegendvalues limit flexibility.The ROBINS-I checklist always renders with
showComments={true}andshowLegend={true}. If callers need to control these, consider passing them through from props:<ROBINSIChecklist checklistState={props.checklist} onUpdate={props.onUpdate} - showComments={true} - showLegend={true} + showComments={props.showComments ?? true} + showLegend={props.showLegend ?? true} readOnly={props.readOnly} />packages/web/src/primitives/useProject/checklists.js (2)
254-266: UseCHECKLIST_TYPES.AMSTAR2constant for consistency.Line 257 uses the string literal
'AMSTAR2'while other parts of this file (lines 50, 94) useCHECKLIST_TYPES.AMSTAR2. Using the constant improves maintainability.- // AMSTAR2: Store as nested Y.Map with answers and critical - if (checklistType === 'AMSTAR2' && data.answers !== undefined) { + // AMSTAR2: Store as nested Y.Map with answers and critical + if (checklistType === CHECKLIST_TYPES.AMSTAR2 && data.answers !== undefined) {
57-80: Hardcoded ROBINS-I keys may drift from the checklist definition.The
robinsKeysarray (lines 59-74) duplicates knowledge about ROBINS-I structure. If the checklist-map adds new domains/sections, this list must be manually updated. Consider extracting this to the registry or deriving fromROBINS_I_CHECKLIST.This could be addressed later by adding a
getChecklistKeys(type)helper to the registry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/web/src/ROBINS-I/checklist-compare.js(1 hunks)packages/web/src/ROBINS-I/checklist.js(1 hunks)packages/web/src/checklist-registry/index.js(1 hunks)packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(5 hunks)packages/web/src/components/checklist-ui/GenericChecklist.jsx(1 hunks)packages/web/src/components/checklist-ui/LocalChecklistView.jsx(4 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx(1 hunks)packages/web/src/primitives/useLocalChecklists.js(2 hunks)packages/web/src/primitives/useProject/checklists.js(5 hunks)packages/web/src/primitives/useProjectChecklistHandlers.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx
- packages/web/src/checklist-registry/index.js
- packages/web/src/ROBINS-I/checklist.js
🧰 Additional context used
🧬 Code graph analysis (5)
packages/web/src/ROBINS-I/checklist-compare.js (1)
packages/web/src/ROBINS-I/checklist-map.js (7)
ROBINS_I_CHECKLIST(428-437)ROBINS_I_CHECKLIST(428-437)getActiveDomainKeys(445-448)domain(452-452)getDomainQuestions(451-465)CHECKLIST_TYPES(74-79)CHECKLIST_TYPES(74-79)
packages/web/src/components/checklist-ui/GenericChecklist.jsx (3)
packages/web/src/checklist-registry/index.js (1)
getChecklistTypeFromState(90-109)packages/web/src/checklist-registry/types.js (4)
DEFAULT_CHECKLIST_TYPE(58-58)DEFAULT_CHECKLIST_TYPE(58-58)CHECKLIST_TYPES(11-18)CHECKLIST_TYPES(11-18)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx (1)
ROBINSIChecklist(19-153)
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (6)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (2)
checklistType(231-237)currentScore(240-245)packages/web/src/components/checklist-ui/GenericChecklist.jsx (1)
checklistType(30-38)packages/web/src/components/checklist-ui/ScoreTag.jsx (2)
checklistType(53-53)ScoreTag(52-79)packages/web/src/components/checklist-ui/CreateLocalChecklist.jsx (1)
checklistType(20-20)packages/web/src/checklist-registry/index.js (3)
checklist(69-69)getChecklistTypeFromState(90-109)scoreChecklistOfType(80-83)packages/web/src/components/project-ui/ChecklistForm.jsx (1)
type(9-9)
packages/web/src/primitives/useProject/checklists.js (2)
packages/web/src/checklist-registry/index.js (1)
createChecklistOfType(67-72)packages/web/src/checklist-registry/types.js (2)
CHECKLIST_TYPES(11-18)CHECKLIST_TYPES(11-18)
packages/web/src/primitives/useLocalChecklists.js (2)
packages/web/src/checklist-registry/types.js (2)
DEFAULT_CHECKLIST_TYPE(58-58)DEFAULT_CHECKLIST_TYPE(58-58)packages/web/src/checklist-registry/index.js (2)
createChecklistOfType(67-72)checklist(69-69)
⏰ 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 (13)
packages/web/src/primitives/useProjectChecklistHandlers.js (1)
18-27: No action required — all callers have been properly updated.The function's breaking API changes (new
typeparameter and boolean return value) have been correctly implemented across all callers. ToDoTab.jsx's wrapper function captures the boolean return value and uses it to conditionally hide the checklist form on success, and all call sites properly pass the requiredtypeparameter through the call chain.packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (3)
178-212: Well-structured type-aware update filtering.The key validation logic correctly separates AMSTAR2 (pattern-based) and ROBINS-I (explicit keys) update rules, ensuring only valid keys for the current checklist type are persisted to Y.js state.
262-262: Proper type propagation to child components.Both
ScoreTagandChecklistWithPdfnow correctly receive thechecklistTypeprop, enabling type-aware rendering throughout the component tree.Also applies to: 310-310
10-10: No action required — type strings are correctly implemented.The implementation properly uses
'AMSTAR2'and'ROBINS_I'(underscore, not hyphen) throughout, which matches theCHECKLIST_TYPESconstants defined inchecklist-registry/types.js. ThegetChecklistTypeFromState()function correctly detects and returns both types, andscoreChecklistOfType()properly handles both with fallback logic. ThehandlePartialUpdate()filtering will work as intended with no risk of silent data loss.packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1)
175-175: LGTM!The type propagation to
ScoreTagandChecklistWithPdfis correctly implemented, enabling type-aware rendering and scoring throughout the component hierarchy.Also applies to: 203-213
packages/web/src/ROBINS-I/checklist-compare.js (4)
14-16: JSON-based cloning is acceptable for plain data.The
deepClonefunction usingJSON.parse(JSON.stringify())works correctly for the checklist data structures (plain objects with primitives). Be aware it won't preserveDateobjects orundefinedvalues if those are ever added to the checklist schema.
109-132: LGTM!The
compareSectionBfunction correctly handles null safety with optional chaining and properly categorizes answers into agreements and disagreements.
141-189: LGTM!The
compareDomainfunction correctly compares question-level answers and domain-level judgements with proper null safety.
428-450: LGTM!The utility functions
getQuestionText,getDomainDef, andgetDomainNameare clean accessors with appropriate fallbacks.packages/web/src/components/checklist-ui/GenericChecklist.jsx (1)
28-38: LGTM!The
checklistTypememo correctly prioritizes the explicitchecklistTypeprop, then derives from state, with a sensible default fallback.packages/web/src/primitives/useLocalChecklists.js (2)
122-138: Minor redundancy:checklistTypeis set twice.The
createChecklistOfTypefunction already addschecklistType: typeto the returned checklist (per registry implementation). Setting it again at line 134 is redundant but ensures consistency even if the registry implementation changes.The redundancy is harmless and provides a safety net.
116-116: LGTM!The function signature with
type = DEFAULT_CHECKLIST_TYPEmaintains backwards compatibility while enabling multi-type support.packages/web/src/primitives/useProject/checklists.js (1)
41-80: LGTM!The type-based answer extraction correctly handles both AMSTAR2 (regex-based question keys) and ROBINS-I (explicit domain/section keys) structures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (1)
4-14: Document thedomainIdprop in JSDoc.The
domainIdprop is used in the component (lines 55 and 93) for generating unique radio button names, but it's not documented in the JSDoc comment.Add this to the JSDoc:
* @param {Function} [props.onDirectionChange] - Callback when direction changes * @param {boolean} [props.showDirection] - Whether to show direction selector * @param {boolean} [props.isDomain1] - Whether this is Domain 1 (uses limited direction options) +* @param {string} [props.domainId] - Unique identifier for the domain (used for radio button naming) * @param {boolean} [props.disabled] - Whether the selector is disabledpackages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (1)
74-98: Consider extracting className logic for readability.The nested ternaries in the
classNametemplate literal work correctly but could be more readable if extracted into a helper function or computed variable.For example:
+ const getButtonClass = (isSelected) => { + const base = 'inline-flex items-center justify-center px-4 py-2 rounded-lg text-sm font-medium cursor-pointer transition-all duration-200 border-2 focus-within:outline-none focus-within:ring-2 focus-within:ring-blue-400 focus-within:ring-offset-1'; + const disabled = props.disabled ? 'opacity-60 cursor-not-allowed' : 'hover:border-blue-300'; + const selected = isSelected ? 'bg-blue-50 border-blue-400 text-blue-800' : 'bg-white border-gray-200 text-gray-700 hover:bg-blue-50'; + return `${base} ${disabled} ${selected}`; + }; + <For each={responseOptions}> {option => ( <label - class={` - inline-flex items-center justify-center px-4 py-2 rounded-lg text-sm font-medium - cursor-pointer transition-all duration-200 border-2 - ${props.disabled ? 'opacity-60 cursor-not-allowed' : 'hover:border-blue-300'} - focus-within:outline-none focus-within:ring-2 focus-within:ring-blue-400 focus-within:ring-offset-1 - ${ - props.sectionBState?.[key]?.answer === option ? - 'bg-blue-50 border-blue-400 text-blue-800' - : 'bg-white border-gray-200 text-gray-700 hover:bg-blue-50' - } - `} + class={getButtonClass(props.sectionBState?.[key]?.answer === option)} >packages/web/src/components/checklist-ui/GenericChecklist.jsx (2)
28-38: HardenchecklistTyperesolution against invalid valuesThe memo correctly prefers an explicit
props.checklistType, falls back to structural detection, then defaults toDEFAULT_CHECKLIST_TYPE. However, if a caller passes an invalidchecklistTypestring, the component silently renders nothing (neither<Show>matches).Consider normalizing/validating the explicit type before returning it so you always fall back to a known value:
- const checklistType = createMemo(() => { - if (props.checklistType) { - return props.checklistType; - } + const checklistType = createMemo(() => { + if (props.checklistType && Object.values(CHECKLIST_TYPES).includes(props.checklistType)) { + return props.checklistType; + }This keeps the behavior robust even if upstream callers make a mistake.
49-57: Align/updateonUpdatecontract between ROBINS-I and callersHere
GenericChecklistpassesprops.onUpdatestraight through toROBINSIChecklist, which internally calls it asonUpdate(fieldKey, newFieldState). In contrast, AMSTAR2 usesonExternalUpdate(newStatePatch).Because
GenericChecklistexposes only a singleonUpdateprop for both types, call sites that previously only handled AMSTAR2 patches may now receive a different argument shape when rendering ROBINS-I, which can easily lead to subtle state bugs.Two concrete suggestions:
- Normalize at the GenericChecklist boundary (recommended): have
GenericChecklistadapt ROBINS-I updates into a common “patch object” shape before forwarding to the parent:- <ROBINSIChecklist - checklistState={props.checklist} - onUpdate={props.onUpdate} + <ROBINSIChecklist + checklistState={props.checklist} + onUpdate={(fieldKey, newFieldState) => + props.onUpdate && props.onUpdate({ [fieldKey]: newFieldState }) + }
- Or enforce distinct contracts: document that for ROBINS-I,
onUpdatemust accept(fieldKey, newFieldState)and ensure all call sites branching on checklist type provide the appropriate handler.Also note that
ROBINSIChecklistassumes bothchecklistStateandonUpdateare non-null; ifGenericChecklistis ever used in a “create new ROBINS-I checklist” context without those props, this path will throw. If such use is expected, consider adding a simple guard or fallback state similar to AMSTAR2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/web/src/components/checklist-ui/GenericChecklist.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx
- packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx
🧰 Additional context used
🧬 Code graph analysis (4)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (2)
packages/web/src/ROBINS-I/checklist-map.js (6)
DOMAIN1_DIRECTIONS(53-53)DOMAIN1_DIRECTIONS(53-53)BIAS_DIRECTIONS(42-50)BIAS_DIRECTIONS(42-50)ROB_JUDGEMENTS(31-31)ROB_JUDGEMENTS(31-31)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
getJudgementColor(31-44)
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (3)
packages/web/src/ROBINS-I/checklist.js (1)
scoreChecklist(162-202)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (1)
getJudgementColor(18-31)packages/web/src/ROBINS-I/checklist-map.js (4)
OVERALL_ROB_JUDGEMENTS(34-39)OVERALL_ROB_JUDGEMENTS(34-39)BIAS_DIRECTIONS(42-50)BIAS_DIRECTIONS(42-50)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (2)
packages/web/src/ROBINS-I/checklist.js (1)
shouldStopAssessment(147-155)packages/web/src/ROBINS-I/checklist-map.js (4)
SECTION_B(82-100)SECTION_B(82-100)RESPONSE_LABELS(17-28)RESPONSE_LABELS(17-28)
packages/web/src/components/checklist-ui/GenericChecklist.jsx (4)
packages/web/src/checklist-registry/index.js (1)
getChecklistTypeFromState(90-109)packages/web/src/checklist-registry/types.js (4)
DEFAULT_CHECKLIST_TYPE(58-58)DEFAULT_CHECKLIST_TYPE(58-58)CHECKLIST_TYPES(11-18)CHECKLIST_TYPES(11-18)packages/web/src/components/checklist-ui/AMSTAR2Checklist.jsx (1)
AMSTAR2Checklist(784-903)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx (1)
ROBINSIChecklist(19-154)
⏰ 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 (13)
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (4)
1-12: LGTM!The imports and documentation are clear and comprehensive.
13-29: LGTM!The reactive computed value and event handlers are implemented correctly, properly preserving state while updating specific fields.
61-81: LGTM!The calculated score display is well-structured and provides clear guidance to users about the auto-calculated score and their ability to override it.
83-166: LGTM!The judgement and direction selectors are well-implemented with proper accessibility (sr-only radio inputs), disabled state handling, and conditional Clear button rendering.
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (4)
15-16: LGTM!The dynamic selection of direction options based on
isDomain1is well-implemented and correctly uses the appropriate constant for each domain type.
33-67: LGTM!The judgement selector is well-implemented with proper accessibility, disabled state handling, and unique radio button naming.
69-118: LGTM!The direction selector is properly implemented with conditional rendering, optional chaining for the callback, and a Clear button that appropriately appears only when a direction is set.
122-147: LGTM!The JudgementBadge component provides a clean, compact display with appropriate fallback text and color coding. The
getColorfunction correctly handles both "Low" and "Low (except confounding)" variants.packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (3)
1-14: LGTM!The imports are well-organized, JSDoc documentation is clear, and the use of
createMemofor the derivedstopAssessmentflag is appropriate and efficient for Solid.js reactivity.
16-37: LGTM!The event handlers correctly maintain immutable state updates.
handleAnswerChangeappropriately recalculates thestopAssessmentflag when answers change, whilehandleCommentChangepreserves it via the spread operator since comments don't affect the stop logic.
39-136: LGTM!The render logic is well-structured with proper use of Solid.js primitives. The stop assessment warning panel correctly responds to the computed memo, and all form inputs properly handle the disabled state. The UI provides clear visual feedback with appropriate styling and informational hints.
packages/web/src/components/checklist-ui/GenericChecklist.jsx (2)
1-27: Docs and component surface are clear and consistentThe JSDoc-style header and prop documentation make the generic nature of this component and its expected props clear and match how the rest of the registry is structured. No changes needed here.
40-48: AMSTAR2 mapping looks correct and preserves existing behaviorRouting AMSTAR2 through
externalChecklist/onExternalUpdatewithreadOnlymatches the existing AMSTAR2Checklist contract and preserves its internal fallback behavior whenprops.checklistis undefined. This segment looks good as-is.
| const getJudgementColor = judgement => { | ||
| switch (judgement) { | ||
| case 'Low (except confounding)': | ||
| return 'bg-green-100 border-green-400 text-green-800'; | ||
| case 'Moderate': | ||
| return 'bg-yellow-100 border-yellow-400 text-yellow-800'; | ||
| case 'Serious': | ||
| return 'bg-orange-100 border-orange-400 text-orange-800'; | ||
| case 'Critical': | ||
| return 'bg-red-100 border-red-400 text-red-800'; | ||
| default: | ||
| return 'bg-gray-50 border-gray-200 text-gray-600'; | ||
| } | ||
| }; | ||
|
|
||
| const getScoreColor = score => { | ||
| switch (score) { | ||
| case 'Low': | ||
| return 'text-green-600'; | ||
| case 'Moderate': | ||
| return 'text-yellow-600'; | ||
| case 'Serious': | ||
| return 'text-orange-600'; | ||
| case 'Critical': | ||
| return 'text-red-600'; | ||
| default: | ||
| return 'text-gray-500'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract color mapping functions to a shared utility.
The getJudgementColor function is duplicated between OverallSection.jsx and DomainJudgement.jsx (line 18-31), with only minor differences in handling "Low (except confounding)". Additionally, JudgementBadge's getColor function (DomainJudgement.jsx lines 126-140) has similar logic.
Consider creating a shared utility file (e.g., packages/web/src/ROBINS-I/color-utils.js) that exports these color mapping functions with appropriate parameters to handle the different cases:
// packages/web/src/ROBINS-I/color-utils.js
export function getJudgementColor(judgement) {
switch (judgement) {
case 'Low':
case 'Low (except confounding)':
return 'bg-green-100 border-green-400 text-green-800';
case 'Moderate':
return 'bg-yellow-100 border-yellow-400 text-yellow-800';
case 'Serious':
return 'bg-orange-100 border-orange-400 text-orange-800';
case 'Critical':
return 'bg-red-100 border-red-400 text-red-800';
default:
return 'bg-gray-50 border-gray-200 text-gray-600';
}
}
export function getScoreColor(score) {
switch (score) {
case 'Low':
return 'text-green-600';
case 'Moderate':
return 'text-yellow-600';
case 'Serious':
return 'text-orange-600';
case 'Critical':
return 'text-red-600';
default:
return 'text-gray-500';
}
}
export function getBadgeColor(judgement) {
switch (judgement) {
case 'Low':
case 'Low (except confounding)':
return 'bg-green-100 text-green-800';
case 'Moderate':
return 'bg-yellow-100 text-yellow-800';
case 'Serious':
return 'bg-orange-100 text-orange-800';
case 'Critical':
return 'bg-red-100 text-red-800';
default:
return 'bg-gray-100 text-gray-600';
}
}Then import and use these shared functions in both components.
🤖 Prompt for AI Agents
In packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx
around lines 31 to 59, the getJudgementColor and getScoreColor functions
duplicate color-mapping logic that also appears in DomainJudgement.jsx; extract
these mappings into a shared utility (e.g.,
packages/web/src/ROBINS-I/color-utils.js) exporting getJudgementColor,
getScoreColor, and getBadgeColor (with 'Low' and 'Low (except confounding)'
handled as the same case where appropriate), replace the local functions with
imports from that utility in OverallSection.jsx and DomainJudgement.jsx, and
update any badge-specific variants to call getBadgeColor so all components reuse
the same centralized logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx (1)
38-45: Add defensive guard forsectionCto prevent runtime errors.If
props.checklistState.sectionCis undefined or null (e.g., from corrupted persisted data or migration issues), line 41 will throw a TypeError when attempting to spread it. Add a defensive default to handle this edge case:function handleSectionCToggle() { + const prev = props.checklistState?.sectionC || {}; props.onUpdate({ sectionC: { - ...props.checklistState.sectionC, + ...prev, isPerProtocol: !isPerProtocol(), }, }); }
🧹 Nitpick comments (3)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (2)
178-220: Use constants from checklist-registry instead of string literals.The type comparisons use hardcoded strings
'AMSTAR2'and'ROBINS_I'. Based on the provided context, the checklist-registry module exportsCHECKLIST_TYPESconstants. Using these constants instead would prevent typos and improve maintainability.Import the constants at the top of the file:
-import { getChecklistTypeFromState, scoreChecklistOfType } from '@/checklist-registry'; +import { getChecklistTypeFromState, scoreChecklistOfType, CHECKLIST_TYPES } from '@/checklist-registry';Then use them in the type comparisons:
// AMSTAR2: keys like q1, q2a, etc. - if (type === 'AMSTAR2' && AMSTAR2_KEY_PATTERN.test(key)) { + if (type === CHECKLIST_TYPES.AMSTAR2 && AMSTAR2_KEY_PATTERN.test(key)) { updateChecklistAnswer(params.studyId, params.checklistId, key, value); } // ROBINS-I: section and domain keys - else if (type === 'ROBINS_I' && ROBINS_I_KEYS.has(key)) { + else if (type === CHECKLIST_TYPES.ROBINS_I && ROBINS_I_KEYS.has(key)) { updateChecklistAnswer(params.studyId, params.checklistId, key, value); }Also update line 244:
- return 'AMSTAR2'; + return CHECKLIST_TYPES.AMSTAR2;
264-267: Use thechecklistType()memo for consistency.Line 266 uses
currentChecklist()?.type || 'AMSTAR2'to display the checklist type, but the rest of the component uses thechecklistType()memo (lines 239-245) which has more sophisticated fallback logic including state-based detection. This inconsistency could cause the header to display a different type than what's actually being used.Apply this diff to use the memo consistently:
<span class='text-gray-900 font-medium'> - {currentChecklist()?.type || 'AMSTAR2'} Checklist + {checklistType()} Checklist </span>packages/web/src/primitives/useProject/checklists.js (1)
346-353: Consider removing debug console.log statements.The console.log statements on lines 346-351 and 353 appear to be debugging artifacts. Consider removing them or replacing with a proper logging utility to keep production code clean.
function updateChecklistAnswer(studyId, checklistId, key, data) { - console.log('[checklists.js] updateChecklistAnswer called', { - studyId, - checklistId, - key, - data, - }); const ydoc = getYDoc(); - console.log('[checklists.js] ydoc:', !!ydoc, 'isSynced:', isSynced()); if (!ydoc || !isSynced()) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(5 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx(1 hunks)packages/web/src/primitives/useProject/checklists.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (3)
packages/web/src/components/checklist-ui/GenericChecklist.jsx (1)
checklistType(30-38)packages/web/src/components/checklist-ui/ScoreTag.jsx (2)
checklistType(53-53)ScoreTag(52-79)packages/web/src/checklist-registry/index.js (3)
checklist(69-69)getChecklistTypeFromState(90-109)scoreChecklistOfType(80-83)
packages/web/src/components/checklist-ui/ROBINSIChecklist/ROBINSIChecklist.jsx (5)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (2)
stopAssessment(14-14)SectionB(13-134)packages/web/src/ROBINS-I/checklist.js (4)
shouldStopAssessment(147-155)isPerProtocol(171-171)isPerProtocol(282-282)isPerProtocol(311-311)packages/web/src/ROBINS-I/checklist-map.js (1)
getActiveDomainKeys(445-448)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainSection.jsx (1)
DomainSection(17-162)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
OverallSection(13-167)
⏰ 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 (5)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (3)
10-10: LGTM! Import changes align with type-aware architecture.The new imports
getChecklistTypeFromStateandscoreChecklistOfTypeproperly support the multi-checklist-type functionality introduced in this PR.
238-253: LGTM! Type detection and scoring logic is well-structured.The
checklistTypememo has appropriate fallback logic (metadata → state detection → default), and thecurrentScorecomputation correctly delegates to the type-aware scoring function with proper null guards.
270-270: LGTM! Type prop correctly passed to child components.The
checklistTypeis properly propagated to bothScoreTagandChecklistWithPdf, enabling them to render type-specific UI and behavior.Also applies to: 318-318
packages/web/src/primitives/useProject/checklists.js (2)
102-160: Well done! ROBINS-I now supports concurrent edits at the question level.The nested Y.Map structure properly isolates each question within domains (lines 115-125), enabling multiple users to edit different questions in the same domain concurrently without conflicts. This addresses the critical concurrency concern from the previous review.
382-447: Update logic correctly maintains concurrent edit support.The granular Y.Map updates for ROBINS-I (especially lines 409-417) ensure that concurrent edits to different questions within the same domain are properly merged, maintaining data consistency across collaborators.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
32-60: Extract color mapping to shared utility as previously suggested.The duplication of
getJudgementColorandgetScoreColorwith similar logic in DomainJudgement.jsx was already flagged in the previous review. Consider implementing the suggested shared utility approach.
🧹 Nitpick comments (6)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
95-111: Consider derivingcommonResponsesfromRESPONSE_LABELSkeys.The hardcoded array duplicates the keys already defined in
RESPONSE_LABELS. Deriving it ensures the legend stays in sync if response types are added or removed.export function ResponseLegend() { - const commonResponses = ['Y', 'PY', 'PN', 'N', 'NI', 'NA', 'WN', 'SN', 'SY', 'WY']; + const commonResponses = Object.keys(RESPONSE_LABELS); return (If a specific display order is needed, keep the explicit array but add a comment noting it must stay in sync with
RESPONSE_LABELS.packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (2)
178-196: Type-aware key filtering inhandlePartialUpdatelooks solid; consider centralizing allowlistsThe per-type filtering (
AMSTAR2_KEY_PATTERNvsROBINS_I_KEYS) makes partial updates safer and prevents accidental cross-checklist writes; the control flow inhandlePartialUpdateis straightforward and easy to extend.Two small design nits you might consider (non-blocking):
- The ROBINS-I key set is hard-coded here; if there’s already a single source of truth for ROBINS-I schema in
@/checklist-registry, pulling the allowlist from there would reduce drift risk when fields change.- Unknown keys are silently dropped; if you ever hit subtle UI bugs from typos, a dev-only assertion/log hook for unexpected keys (behind a debug flag) could help, without reintroducing noisy console logging in production.
Also applies to: 197-212
10-10: Type-aware checklist detection and scoring are wired correctly; minor header consistency nitThe new
checklistTypememo (falling back fromchecklist.type→getChecklistTypeFromState(ui)→'AMSTAR2') plusscoreChecklistOfType(type, checklist)and passingchecklistType()through toScoreTagandChecklistWithPdfform a coherent, type-driven flow. This should generalize cleanly as more checklist types are added.One optional polish point: the header label still uses
{currentChecklist()?.type || 'AMSTAR2'}rather thanchecklistType(), so in edge cases where the type is only detectable from state (but not yet persisted in metadata), the displayed type could differ from the one driving scoring/behavior. UsingchecklistType()there as well would keep the UI and behavior perfectly in sync.Also applies to: 231-238, 241-246, 263-263, 311-311
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (3)
4-15: Align JSDoc with nullable/optional props used at runtimeThe JSDoc currently declares
domainId,judgement, anddirectionas required/non‑nullable strings, but the implementation treats them as optional and allowsnull(via the Clear buttons). This can mislead consumers and tooling.Consider updating the JSDoc to match the actual contract, e.g.:
- * @param {Object} props - * @param {string} props.domainId - Unique domain identifier - * @param {string} props.judgement - Current judgement value - * @param {string} [props.direction] - Current direction value (if applicable) + * @param {Object} props + * @param {string} [props.domainId] - Unique domain identifier (used in form control names) + * @param {string|null} [props.judgement] - Current judgement value (null/undefined when not assessed) + * @param {string|null} [props.direction] - Current direction value (if applicable; null/undefined when cleared)This keeps the Clear behaviour (
onJudgementChange(null),onDirectionChange?.(null)) consistent with the documented API surface.Also applies to: 69-77, 118-126
20-33: Reduce duplication in ROBINS‑I judgement→color mapping
getJudgementColorinDomainJudgementandgetColorinJudgementBadgeimplement near‑identical mappings, and there’s a third very similargetJudgementColorinOverallSection.jsx(lines 31‑44 in that file). This increases the risk of subtle divergence if a judgement label or colour changes.Consider centralizing the mapping, for example:
- Extract a small helper (e.g.
getRobinsJudgementTone(judgement)) that returns a semantic token ('green' | 'yellow' | 'orange' | 'red' | 'gray'), or- Extract a base Tailwind class map and let each component add its own border/shape specifics.
Then reuse that helper in
DomainJudgement,JudgementBadge, andOverallSection.jsxso a change in one place stays consistent across the ROBINS‑I UI.Also applies to: 138-153
82-130: Clarify contract betweenshowDirectionandonDirectionChangeThe direction chips are rendered whenever
props.showDirectionis truthy, but user interaction is only wired ifprops.onDirectionChangeis provided (via optional chaining). If a parent mistakenly setsshowDirectionwithout supplying a handler, the UI appears interactive but won’t respond.Two small options to make this more robust:
- Treat
onDirectionChangeas required whenevershowDirectionis true (and document/enforce that), or- Provide a default no‑op (
const handleDirectionChange = props.onDirectionChange ?? (() => {});) and drop the optional chaining so the component’s behaviour is predictable.Not critical, but tightening this contract will make misconfiguration easier to spot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/web/src/checklist-registry/index.js(1 hunks)packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx(5 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx(1 hunks)packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx
- packages/web/src/checklist-registry/index.js
🧰 Additional context used
🧬 Code graph analysis (3)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (4)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (1)
uniqueId(17-17)packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
uniqueId(14-14)packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (1)
uniqueId(14-14)packages/web/src/ROBINS-I/checklist-map.js (3)
getResponseOptions(468-470)RESPONSE_LABELS(17-28)RESPONSE_LABELS(17-28)
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (4)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (2)
uniqueId(17-17)getJudgementColor(20-33)packages/web/src/components/checklist-ui/ROBINSIChecklist/SectionB.jsx (1)
uniqueId(14-14)packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
uniqueId(14-14)packages/web/src/ROBINS-I/checklist-map.js (4)
OVERALL_ROB_JUDGEMENTS(34-39)OVERALL_ROB_JUDGEMENTS(34-39)BIAS_DIRECTIONS(42-50)BIAS_DIRECTIONS(42-50)
packages/web/src/components/checklist-ui/ROBINSIChecklist/DomainJudgement.jsx (2)
packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (1)
getJudgementColor(32-45)packages/web/src/ROBINS-I/checklist-map.js (6)
DOMAIN1_DIRECTIONS(53-53)DOMAIN1_DIRECTIONS(53-53)BIAS_DIRECTIONS(42-50)BIAS_DIRECTIONS(42-50)ROB_JUDGEMENTS(31-31)ROB_JUDGEMENTS(31-31)
⏰ 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 (8)
packages/web/src/components/checklist-ui/ROBINSIChecklist/SignallingQuestion.jsx (1)
13-90: Well-structured SolidJS component.The implementation correctly uses SolidJS patterns:
createUniqueId()for unique radio names, derived signal foroptions(), and proper reactivity. Good accessibility practices withsr-onlyinputs, associated labels, and title tooltips. Defensive optional chaining forprops.answeris appropriate.packages/web/src/components/checklist-ui/ROBINSIChecklist/OverallSection.jsx (7)
1-3: LGTM! Imports are clean and properly aliased.All imports are used appropriately throughout the component.
13-16: LGTM! Component setup follows SolidJS patterns correctly.The reactive
calculatedScore()anduniqueIdare properly utilized.
18-30: LGTM! State update handlers follow immutable update patterns correctly.Both handlers properly spread the existing state and update specific fields without mutation.
85-127: LGTM! Judgement selection UI is well-structured and accessible.The radio button implementation follows accessibility best practices with
sr-onlyinputs and proper label wrapping. The unique ID prevents naming conflicts, and the Clear button provides good UX.
130-175: LGTM! Direction selection follows consistent patterns.The optional bias direction UI mirrors the judgement selection structure with appropriate styling differences (blue theme) to visually distinguish it as a supplementary field.
181-181: LGTM! Dual export pattern supports flexible imports.Both named and default exports are provided, consistent with the codebase patterns.
71-82: This review comment is incorrect. The 'Low' vs 'Low (except confounding)' distinction is intentional ROBINS-I methodology design.The calculated score returns 'Low' while the manual override options include 'Low (except confounding)' because these are different judgment contexts. ROBINS-I tool documentation explicitly states: "Overall ROB includes special option for confounding." This distinction is not a semantic mismatch or bug—it is the correct implementation of the ROBINS-I assessment methodology, where domain-level judgments use the standard scale but the overall judgment includes the specialized 'Low (except confounding)' option.
No changes needed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (1)
531-573: Comprehensive cleanup, butrenderedPages.clear()is missing for consistency.The cleanup logic properly cancels render tasks, destroys the document, clears canvases, and releases observers/listeners. However,
renderedPages.clear()is called inclearPdf(line 472) but not here. While not a memory leak sincerenderedPagesis instance-scoped and the component is unmounting, adding it would maintain consistency withclearPdf.// Clear refs pageRefs.clear(); + renderedPages.clear(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (2)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (2)
217-228: Good cleanup pattern for document replacement.Properly awaiting
destroy()before loading a new document ensures PDF.js releases internal resources (canvas contexts, workers) and prevents memory leaks when switching PDFs. Clearing the render tracking state prevents stale render tasks from interfering with the new document.
430-448: Fire-and-forget destroy is acceptable here but differs fromloadPdf.Unlike
loadPdf(line 224) which awaitsdestroy(), here it's fire-and-forget. This is fine forclearPdfsince it needs synchronous UI feedback, and PDF.js's destroy primarily releases internal references without requiring the caller to wait. The canvas clearing that follows is safe since it operates on DOM elements independently of PDF.js internals.
2 participants
Add robins-i v2 checklist and scoring similar to the existing amstar2 one.
Summary by CodeRabbit
New Features
Documentation
Removed
Stability
✏️ Tip: You can customize this high-level summary in your review settings.