51 scoring for robins#208
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR implements a comprehensive ROBINS-I auto-scoring engine, refactors checklist operations into modular type-specific handlers, replaces inline text inputs with collaborative NoteEditor components across sections, and introduces auto/manual scoring mode switching for domain and overall judgements in the checklist UI. Changes
Sequence DiagramssequenceDiagram
autonumber
actor User
participant ROBINSIChecklist
participant DomainSection
participant ScoringEngine as Scoring Engine<br/>(robins-scoring.js)
participant State as Checklist State
User->>DomainSection: Updates domain question answer
DomainSection->>ScoringEngine: scoreRobinsDomain(domain, answers)
ScoringEngine-->>DomainSection: {judgement, ruleId, isComplete}
DomainSection->>State: Store auto score + update domainState
DomainSection->>DomainSection: Compute effectiveJudgement<br/>(auto or manual override)
alt In Auto Mode
DomainSection->>DomainSection: Display "Calculated" judgement
Note over DomainSection: Judgement source: 'auto'
else User Switches to Manual
User->>DomainSection: Click "Manual" toggle
DomainSection->>State: judgementSource = 'manual'<br/>preserve current/auto judgement
DomainSection->>DomainSection: Display manual edit option
end
alt Manual Override Changed
User->>DomainSection: Update manual judgement
DomainSection->>State: Update judgement + judgementSource
DomainSection->>DomainSection: Show "Revert to calculated" button
end
DomainSection-->>ROBINSIChecklist: Render with mode indicator
ROBINSIChecklist->>ScoringEngine: scoreAllDomains(checklistState)
ScoringEngine-->>ROBINSIChecklist: {domain results, overall, isComplete}
sequenceDiagram
autonumber
actor User
participant ChecklistYjsWrapper
participant useProject as useProject Hook
participant ChecklistOps as Checklist Operations<br/>(handlers)
participant ROBINSIHandler
participant YDoc as Yjs Document
ChecklistYjsWrapper->>useProject: getRobinsText(studyId, checklistId,<br/>sectionKey, fieldKey, questionKey)
useProject->>ChecklistOps: checklistOps.getRobinsText(...args)
ChecklistOps->>ROBINSIHandler: Handler.getTextGetter(getYDoc)
ROBINSIHandler->>ROBINSIHandler: Return navigation function for<br/>reviews → study → checklist
ROBINSIHandler-->>ChecklistOps: textGetter function
ChecklistOps->>YDoc: Navigate to field path<br/>Create Y.Text if missing
YDoc-->>ChecklistOps: Y.Text instance
ChecklistOps-->>useProject: Y.Text reference
useProject-->>ChecklistYjsWrapper: Y.Text for NoteEditor
ChecklistYjsWrapper->>ChecklistYjsWrapper: Pass Y.Text to child components<br/>(PlanningSection, SectionA, etc.)
User->>ChecklistYjsWrapper: Edit text in NoteEditor
Note over YDoc: Yjs syncs edits<br/>collaboratively across<br/>connected clients
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: The PR spans 30+ files with significant heterogeneous changes: (1) new deterministic scoring engine with 9 decision tables requiring validation against ROBINS-I specifications; (2) complete refactoring of checklist operations with new modular handler architecture affecting critical data-access paths; (3) substantial UI state management additions (auto/manual modes, derived computed values) in 3 core components; (4) repetitive text-input replacements across 6 sections but with distinct integration points; (5) new ScoringSummary component with modal and navigation logic. The scoring engine and handler refactoring demand careful code review for correctness and API compatibility; DomainSection and OverallSection changes require verification of state transitions and manual override logic. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | a702dbb | Commit Preview URL | Jan 03 2026, 05:23 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: 5
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/ROBINSIChecklist/DomainSection.jsx (1)
198-206: Subsection questions lack collaborative comment editing props; verify if intentional.Subsection questions (Domain 3, lines 198-206) don't receive
domainKey,questionKey, andgetRobinsTextprops, unlike non-subsection questions (lines 181-183). These props are optional and only affect Y.Text-synced collaborative comment editing viaNoteEditor. When these props are absent,commentYText()returns null, andNoteEditorgracefully falls back to local-only comments without real-time sync.Functionally, subsection questions work correctly, but they lose collaborative comment editing. If comment collaboration should be consistent across all questions, pass the missing props with appropriate keys. If subsections intentionally exclude collaborative features, add a comment explaining why.
🧹 Nitpick comments (9)
packages/web/src/primitives/useProject/checklists/common.js (2)
19-37: Consider updating study's updatedAt for consistency.The
deleteChecklistfunction updatesstudyYMap.set('updatedAt', Date.now())(line 56), butupdateChecklistonly updates the checklist's ownupdatedAt(line 36). For consistency, consider updating the study'supdatedAtwhen a checklist is modified, so that study-level change tracking reflects checklist updates.Proposed change
if (updates.title !== undefined) checklistYMap.set('title', updates.title); if (updates.assignedTo !== undefined) checklistYMap.set('assignedTo', updates.assignedTo); if (updates.status !== undefined) checklistYMap.set('status', updates.status); checklistYMap.set('updatedAt', Date.now()); + studyYMap.set('updatedAt', Date.now());
65-80: Ensure consistent null return for missing answers.Line 79 returns
checklistYMap.get('answers'), which will returnundefinedif the 'answers' key doesn't exist, but the JSDoc states it returnsY.Map|null. For consistency with the rest of the function's null-checking pattern, consider explicitly returningnullwhen answers are missing.Proposed change
const checklistYMap = checklistsMap.get(checklistId); if (!checklistYMap) return null; - return checklistYMap.get('answers'); + const answersMap = checklistYMap.get('answers'); + return answersMap || null;packages/web/src/components/checklist/ROBINSIChecklist/DomainJudgement.jsx (1)
48-53: Consider extracting getShortLabel to a shared utility.Both
DomainJudgementandJudgementBadgedefine identicalgetShortLabelfunctions. While this works fine, extracting it to a shared utility (e.g., inchecklist-map.jsor a separate utils file) would eliminate duplication and ensure consistency if the label shortening logic needs to change in the future.Also applies to: 140-145
packages/web/src/components/checklist/ROBINSIChecklist/ROBINSIChecklist.jsx (1)
96-105: ScoringSummary sticky positioning.The sticky positioning with
z-40and inlinetop: '8px'provides a floating summary header. Consider using a CSS variable or Tailwind class for the top offset to maintain consistency with thescroll-margin-topcalculation in domain sections.packages/web/src/components/checklist/ROBINSIChecklist/ScoringSummary.jsx (1)
150-164: Domain chips with accessibility title.The domain chips include a descriptive
titleattribute showing the full domain name and status. Consider addingaria-labelfor better screen reader support since the visible text is abbreviated (D1, D2, etc.).Optional: Add aria-label for improved accessibility
<button type='button' onClick={() => props.onDomainClick?.(domainKey)} title={`${ROBINS_I_CHECKLIST[domainKey]?.name}: ${getDomainStatusText(domainKey)}`} + aria-label={`${ROBINS_I_CHECKLIST[domainKey]?.name}: ${getDomainStatusText(domainKey)}`} class={`inline-flex items-center gap-1 rounded border px-2 py-0.5 text-xs font-medium transition-colors hover:opacity-80 ${getDomainChipColor(domainKey)}`} >packages/web/src/components/checklist/ROBINSIChecklist/OverallSection.jsx (1)
165-199: Mode toggle UI with duplicate revert action.The "Revert to calculated" link (lines 168-176) and the "Auto" button (lines 178-187) both call
handleRevertToAuto. Consider removing the redundant link since the Auto button provides the same functionality with clearer UX.Optional: Remove redundant revert link
The Auto/Manual toggle buttons already provide the ability to switch modes. The additional "Revert to calculated" link may be redundant:
{/* Mode toggle */} <div class='flex items-center gap-2'> - <Show when={isManualMode() && isOverridden()}> - <button - type='button' - onClick={handleRevertToAuto} - disabled={props.disabled} - class='text-xs text-blue-600 hover:text-blue-800 hover:underline disabled:opacity-50' - > - Revert to calculated - </button> - </Show> <div class='flex rounded-md border border-gray-200 bg-white text-xs'>packages/web/src/primitives/useProject/checklists/index.js (1)
20-22: UnusedprojectIdparameter.The
projectIdparameter is declared but never used within the function body. If it's intended for future use, consider prefixing it with an underscore (_projectId) like_isSyncedto indicate intentional non-use. Otherwise, remove it.packages/web/src/components/checklist/ROBINSIChecklist/scoring/robins-scoring.js (1)
376-379: Unused variable_q10in Domain 4 scoring.The variable
_q10(d4_10) is extracted from answers but is not used in the current scoring logic. The underscore prefix indicates awareness, but consider adding a comment explaining this is reserved for future table expansion, or remove it if not needed.packages/web/src/components/checklist/ROBINSIChecklist/DomainSection.jsx (1)
247-278: Auto/Manual toggle buttons have redundant click handler.The "Auto" button (lines 248-262) calls
handleRevertToAuto()on click, which is the same action as the "Revert to calculated" link above (lines 235-246). This is intentional UX (clicking "Auto" when in manual mode reverts to auto), but consider extracting the common styling logic to reduce duplication.Consider extracting button styling
const modeButtonClass = (isActive, disabled) => ` px-2.5 py-1 transition-colors ${isActive ? 'bg-blue-100 text-blue-800' : 'text-gray-600 hover:bg-gray-50'} ${disabled ? 'cursor-not-allowed opacity-50' : ''} `;
| | Rule | Q1.1 Appropriate analysis | Q1.2 All confounders controlled | Q1.3 Confounders measured validly | Q1.4 Post‑intervention vars controlled | Q1.5 Uncontrolled confounding | Risk of Bias | | ||
| | ---- | ------------------------- | ------------------------------- | --------------------------------- | -------------------------------------- | ----------------------------- | ----------------------------- | | ||
| | 1 | Y/PY | Y/PY | Y/PY | — | N/PN | **Low** | | ||
| | 2 | Y/PY | Y/PY | Y/PY | — | Y/PY | **Moderate** | | ||
| | 3 | Y/PY | WN | Y/PY/WN | — | N/PN | **Low (except uncontrolled)** | | ||
| | 4 | Y/PY | WN | Y/PY/WN | — | Y/PY | **Moderate** | | ||
| | 5 | Y/PY | SN/NI | — | — | — | **Serious** | | ||
| | 6 | N/PN/NI | — | — | Y/PY | — | **Critical** | | ||
| | 7 | N/PN/NI | — | — | N/PN/NI | Y/PY | **Critical** | | ||
| | 8 | N/PN/NI | — | — | N/PN/NI | N/PN | **Serious** | |
There was a problem hiding this comment.
Replace unicode em-dashes with ASCII hyphens.
The table uses unicode em-dash characters (—) for cells indicating "not applicable," which violates the coding guideline prohibiting unicode symbols. Use regular ASCII hyphens or plain text instead.
🔎 Proposed fix
Replace all unicode em-dashes (—) with regular hyphens (-) or dashes (--):
| Rule | Q1.1 Appropriate analysis | Q1.2 All confounders controlled | Q1.3 Confounders measured validly | Q1.4 Post‑intervention vars controlled | Q1.5 Uncontrolled confounding | Risk of Bias |
| ---- | ------------------------- | ------------------------------- | --------------------------------- | -------------------------------------- | ----------------------------- | ----------------------------- |
- | 3 | Y/PY | WN | Y/PY/WN | — | N/PN | **Low (except uncontrolled)** |
+ | 3 | Y/PY | WN | Y/PY/WN | - | N/PN | **Low (except uncontrolled)** |
- | 5 | Y/PY | SN/NI | — | — | — | **Serious** |
+ | 5 | Y/PY | SN/NI | - | - | - | **Serious** |[As per coding guidelines: "Do NOT use unicode symbols - unicode symbols are forbidden anywhere in the codebase"]
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/web/src/components/checklist/ROBINSIChecklist/scoring/decision-tables/table
(1).md around lines 1 to 10, the table uses unicode em-dashes (—) to indicate
"not applicable"; replace every em-dash character with an ASCII hyphen (-) (or
plain text like "N/A" if preferred by project style) so no unicode symbols
remain, ensuring the replacements preserve table alignment and spacing.
| | Rule | Q2.1 Distinguishable at start | Q2.2 Outcomes distinguishable | Q2.3 Appropriate analysis | Q2.4 Outcome‑dependent classification | Q2.5 Further errors | Risk of Bias | | ||
| | ---- | ----------------------------- | ----------------------------- | ------------------------- | ------------------------------------- | ------------------- | ------------ | | ||
| | 1 | Y/PY | — | — | N/PN | N/PN | **Low** | | ||
| | 2 | Y/PY | — | — | N/PN | Y/PY/NI | **Moderate** | | ||
| | 3 | Y/PY | — | — | WY/NI | N/PN | **Moderate** | | ||
| | 4 | Y/PY | — | — | SY | — | **Serious** | | ||
| | 5 | N/PN/NI | Y/PY | — | N/PN | N/PN | **Low** | | ||
| | 6 | N/PN/NI | N/PN/NI | Y/PY | WY/NI | Y/PY/NI | **Serious** | | ||
| | 7 | N/PN/NI | N/PN/NI | N/PN | SY/WY/NI | — | **Critical** | |
There was a problem hiding this comment.
Replace unicode em-dashes with ASCII hyphens.
The table uses unicode em-dash characters (—) for cells indicating "not applicable," which violates the coding guideline prohibiting unicode symbols. Use regular ASCII hyphens or plain text instead.
🔎 Proposed fix
Replace all unicode em-dashes (—) with regular hyphens (-):
| Rule | Q2.1 Distinguishable at start | Q2.2 Outcomes distinguishable | Q2.3 Appropriate analysis | Q2.4 Outcome‑dependent classification | Q2.5 Further errors | Risk of Bias |
| ---- | ----------------------------- | ----------------------------- | ------------------------- | ------------------------------------- | ------------------- | ------------ |
- | 1 | Y/PY | — | — | N/PN | N/PN | **Low** |
+ | 1 | Y/PY | - | - | N/PN | N/PN | **Low** |
- | 2 | Y/PY | — | — | N/PN | Y/PY/NI | **Moderate** |
+ | 2 | Y/PY | - | - | N/PN | Y/PY/NI | **Moderate** |
- | 3 | Y/PY | — | — | WY/NI | N/PN | **Moderate** |
+ | 3 | Y/PY | - | - | WY/NI | N/PN | **Moderate** |
- | 4 | Y/PY | — | — | SY | — | **Serious** |
+ | 4 | Y/PY | - | - | SY | - | **Serious** |[As per coding guidelines: "Do NOT use unicode symbols - unicode symbols are forbidden anywhere in the codebase"]
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/web/src/components/checklist/ROBINSIChecklist/scoring/decision-tables/table
(2).md around lines 1 to 9, the table uses unicode em-dashes (—) to indicate
"not applicable"; replace every unicode em-dash with a plain ASCII hyphen (-) so
cells show "-" instead of "—", and save the file ensuring no other unicode dash
characters remain.
| | Rule | Complete data (4.1–4.3) | Method | Missingness handled | Evidence not biased | Risk | | ||
| | ---- | ----------------------- | ------------- | ---------------------- | ------------------- | ------------ | | ||
| | 1 | All Y/PY | — | — | — | **Low** | | ||
| | 2 | Any N/PN/NI | Complete‑case | Not outcome‑related | — | **Low** | | ||
| | 3 | Any N/PN/NI | Complete‑case | Explained by model | Y/PY | **Moderate** | | ||
| | 4 | Any N/PN/NI | Complete‑case | Explained by model | N/PN | **Serious** | | ||
| | 5 | Any N/PN/NI | Imputation | MAR/MCAR + appropriate | — | **Low** | | ||
| | 6 | Any N/PN/NI | Imputation | Weak/unclear | Y/PY | **Moderate** | | ||
| | 7 | Any N/PN/NI | Imputation | Weak/unclear | N/PN | **Serious** | | ||
| | 8 | Any N/PN/NI | Poor/none | Strong concern | N/PN | **Critical** | |
There was a problem hiding this comment.
Replace unicode dashes with ASCII hyphens throughout the table.
The table uses unicode characters: em-dashes (—) for "not applicable" cells and en-dashes in terms like "Complete‑case" and "outcome‑related." These violate the coding guideline prohibiting unicode symbols. Replace with regular ASCII hyphens.
🔎 Proposed fix
Replace unicode em-dashes and en-dashes with ASCII hyphens:
| Rule | Complete data (4.1–4.3) | Method | Missingness handled | Evidence not biased | Risk |
| ---- | ----------------------- | ------------- | ---------------------- | ------------------- | ------------ |
- | 1 | All Y/PY | — | — | — | **Low** |
+ | 1 | All Y/PY | - | - | - | **Low** |
- | 2 | Any N/PN/NI | Complete‑case | Not outcome‑related | — | **Low** |
+ | 2 | Any N/PN/NI | Complete-case | Not outcome-related | - | **Low** |
- | 3 | Any N/PN/NI | Complete‑case | Explained by model | Y/PY | **Moderate** |
+ | 3 | Any N/PN/NI | Complete-case | Explained by model | Y/PY | **Moderate** |
- | 4 | Any N/PN/NI | Complete‑case | Explained by model | N/PN | **Serious** |
+ | 4 | Any N/PN/NI | Complete-case | Explained by model | N/PN | **Serious** |[As per coding guidelines: "Do NOT use unicode symbols - unicode symbols are forbidden anywhere in the codebase"]
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/web/src/components/checklist/ROBINSIChecklist/scoring/decision-tables/table
(6).md around lines 1 to 10, replace all unicode dash characters (em-dash U+2014
"—", en-dash U+2013 "–", and non‑breaking hyphen U+2011 "‑") used in the table
with the ASCII hyphen-minus character '-' so that cells like "—" become "-" and
terms like "Complete‑case" and "outcome‑related" use "Complete-case" and
"outcome-related"; update every occurrence in the file and save.
| {responseOptions.map(option => ( | ||
| <div | ||
| class={`inline-flex items-center justify-center rounded-lg border-2 px-4 py-2 text-sm font-medium transition-all ${ | ||
| reviewer1Answer === option ? | ||
| 'border-blue-400 bg-blue-50 text-blue-800' | ||
| : 'border-gray-200 bg-white text-gray-700' | ||
| }`} | ||
| > | ||
| <span class='mr-1'>{option}</span> | ||
| <span class='text-xs opacity-70'>({RESPONSE_LABELS[option]})</span> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use SolidJS For component instead of Array.map().
The component uses Array.map() to render response options in three places. Per the coding guidelines, SolidJS applications should use the For component for rendering lists instead of Array.map().
Example refactor for Reviewer 1 panel
+import { createSignal, For } from 'solid-js';
...
- {responseOptions.map(option => (
+ <For each={responseOptions}>
+ {option => (
<div
class={`inline-flex items-center justify-center rounded-lg border-2 px-4 py-2 text-sm font-medium transition-all ${
reviewer1Answer === option ?
'border-blue-400 bg-blue-50 text-blue-800'
: 'border-gray-200 bg-white text-gray-700'
}`}
>
<span class='mr-1'>{option}</span>
<span class='text-xs opacity-70'>({RESPONSE_LABELS[option]})</span>
</div>
- ))}
+ )}
+ </For>Apply the same pattern to the other two panels (Reviewer 2 and Final).
As per coding guidelines: "Use Solid's For component for rendering lists instead of Array.map()"
Also applies to: 110-121, 141-160
🤖 Prompt for AI Agents
In packages/web/src/components/mock/RobinsReconcileSectionBQuestionMock.jsx
around lines 79-90 (and similarly at 110-121 and 141-160), the JSX uses
Array.map() to render responseOptions; replace each Array.map usage with
SolidJS's For component (import For from 'solid-js' or 'solid-js/web' as
appropriate), move the mapped markup into the For child function, and ensure
each item is rendered using the item and index provided by For (no manual key
prop needed). Apply the same For-based refactor to the Reviewer 2 and Final
panels at the other line ranges so all list rendering follows the SolidJS
guideline.
| Object.entries(data.answers).forEach(([qKey, qValue]) => { | ||
| let questionYMap = answersNestedYMap.get(qKey); | ||
| if (!questionYMap || !(questionYMap instanceof Y.Map)) { | ||
| questionYMap = new Y.Map(); | ||
| answersNestedYMap.set(qKey, questionYMap); | ||
| } | ||
| if (qValue.answer !== undefined) questionYMap.set('answer', qValue.answer); | ||
| if (qValue.comment !== undefined) questionYMap.set('comment', qValue.comment); | ||
| }); |
There was a problem hiding this comment.
Comment field stored as string instead of Y.Text in updateAnswer.
In updateAnswer for domain questions (line 245), qValue.comment is set directly as a string. However, in createAnswersYMap (line 68), comments are initialized as Y.Text. This inconsistency could cause issues when collaborative edits occur on comments.
Suggested fix: Handle comment as Y.Text in updateAnswer
Object.entries(data.answers).forEach(([qKey, qValue]) => {
let questionYMap = answersNestedYMap.get(qKey);
if (!questionYMap || !(questionYMap instanceof Y.Map)) {
questionYMap = new Y.Map();
answersNestedYMap.set(qKey, questionYMap);
}
if (qValue.answer !== undefined) questionYMap.set('answer', qValue.answer);
- if (qValue.comment !== undefined) questionYMap.set('comment', qValue.comment);
+ if (qValue.comment !== undefined) {
+ const commentText = questionYMap.get('comment');
+ if (commentText instanceof Y.Text) {
+ // Update existing Y.Text content
+ commentText.delete(0, commentText.length);
+ commentText.insert(0, qValue.comment);
+ } else {
+ // Create new Y.Text if missing
+ const newComment = new Y.Text();
+ newComment.insert(0, qValue.comment);
+ questionYMap.set('comment', newComment);
+ }
+ }
});The same pattern should be applied to sectionB comment handling at lines 257-258.
🤖 Prompt for AI Agents
packages/web/src/primitives/useProject/checklists/handlers/robins-i.js around
lines 238-246 (and similarly adjust sectionB at lines ~257-258): updateAnswer is
storing comment as a plain string while createAnswersYMap initializes comments
as Y.Text; change the logic so you always store a Y.Text for comment: if the map
already contains a Y.Text, replace its contents with the new string (not
overwrite the entry with a string); if it does not, create a new Y.Text, set its
contents to the comment string, and put that Y.Text into the Y.Map. Apply the
same pattern for the sectionB comment handling at the noted lines.
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.