Skip to content

266 Move checklist logic to shared#267

Merged
InfinityBowman merged 4 commits into
mainfrom
266-importexport-project-data
Jan 9, 2026
Merged

266 Move checklist logic to shared#267
InfinityBowman merged 4 commits into
mainfrom
266-importexport-project-data

Conversation

@InfinityBowman
Copy link
Copy Markdown
Owner

@InfinityBowman InfinityBowman commented Jan 9, 2026

Summary by CodeRabbit

  • New Features

    • Added development tools panel for inspecting and editing checklist state (dev mode only)
    • Enhanced mock UI flows for adding studies: wizard modal, slide-over panel, and inline progressive options
  • Refactor

    • Reorganized checklist functionality (AMSTAR2, ROBINS-I) into shared package with centralized utilities
    • Updated mock routes from /mock to /mocks with new sub-routes
  • Tests

    • Added comprehensive test suites for AMSTAR2, ROBINS-I, and checklist status modules
  • Chores

    • Updated ESLint configuration to exclude mock directories

✏️ Tip: You can customize this high-level summary in your review settings.

@InfinityBowman InfinityBowman linked an issue Jan 9, 2026 that may be closed by this pull request
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Jan 9, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
corates 5249b37 Jan 09 2026, 08:22 PM

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jan 9, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

Introduces a comprehensive centralization of AMSTAR2 and ROBINS-I checklist logic into a shared package with schemas, creation, scoring, answer extraction, and comparison utilities. The web package now re-exports these utilities for backward compatibility. Mock UI components for study import workflows and project views are added to the web package.

Changes

Cohort / File(s) Summary
Shared Checklist Type System
packages/shared/src/checklists/types.ts, packages/shared/src/checklists/status.ts
New type definitions for AMSTAR2 and ROBINS-I checklists, checklist metadata, and status constants with transition/validation helpers (isEditable, canTransitionTo, getStatusLabel, getStatusStyle).
Shared Domain Logic
packages/shared/src/checklists/domain.ts
New module providing tab-based visibility, status transitions, and reconciliation workflow utilities (getTodoChecklists, getCompletedChecklists, isDualReviewerStudy, getNextStatusForCompletion, etc.).
Shared AMSTAR2 Schema & Creation
packages/shared/src/checklists/amstar2/schema.ts, packages/shared/src/checklists/amstar2/create.ts
Complete AMSTAR2 checklist schema with question definitions, critical/non-critical categorization, and factory function createAMSTAR2Checklist with validation.
Shared AMSTAR2 Scoring & Answers
packages/shared/src/checklists/amstar2/score.ts, packages/shared/src/checklists/amstar2/answers.ts
Scoring engine (scoreAMSTAR2Checklist with critical/non-critical flaw logic) and answer extraction utilities (getSelectedAnswer, consolidateAnswers, getAnswers); handles multi-part questions (q9a/b, q11a/b).
Shared AMSTAR2 Comparison
packages/shared/src/checklists/amstar2/compare.ts
Comprehensive comparison and reconciliation module with per-question analysis, multi-part question handling, and reconciled checklist creation; exposes QuestionComparison, ComparisonResult types.
Shared AMSTAR2 Exports
packages/shared/src/checklists/amstar2/index.ts
Barrel module re-exporting schema, create, score, answers, and compare modules.
Shared ROBINS-I Schema
packages/shared/src/checklists/robins-i/schema.ts
Complete ROBINS-I V2 domain-based risk-of-bias schema with sections, domains, response types, judgements, and utility accessors (getDomainKeys, getDomainQuestions, getResponseOptions).
Shared ROBINS-I Creation & Answers
packages/shared/src/checklists/robins-i/create.ts, packages/shared/src/checklists/robins-i/answers.ts
Factory function createROBINSIChecklist and utilities for assessment logic, answer extraction, and domain summaries (shouldStopAssessment, getAnswers, getDomainSummary, isROBINSIComplete).
Shared ROBINS-I Scoring
packages/shared/src/checklists/robins-i/scoring.ts
Deterministic scoring engine with per-domain rule-based judges (domains 1A–6), overall judgement aggregation, and display mappings (scoreRobinsDomain, scoreAllDomains, mapOverallJudgementToDisplay).
Shared ROBINS-I Exports
packages/shared/src/checklists/robins-i/index.ts
Barrel module re-exporting schema, create, scoring, and answers modules.
Shared Checklist Index
packages/shared/src/checklists/index.ts, packages/shared/src/index.ts
Main aggregator exposing checklists module with convenience aliases (createAMSTAR2Checklist, compareAMSTAR2Checklists, createROBINSIChecklist, scoreAllDomains, etc.) and namespace exports.
Checklist Test Suites
packages/shared/src/checklists/__tests__/amstar2.test.ts, packages/shared/src/checklists/__tests__/robins-i.test.ts, packages/shared/src/checklists/__tests__/status.test.ts
Comprehensive Vitest unit tests covering creation, scoring, completion checks, answer consolidation, multi-part question handling, status transitions, and domain-specific logic.
Web AMSTAR2 Migration
packages/web/src/components/checklist/AMSTAR2Checklist/checklist-map.js, packages/web/src/components/checklist/AMSTAR2Checklist/checklist.js, packages/web/src/components/checklist/AMSTAR2Checklist/checklist-compare.js
Re-export AMSTAR2 logic from @corates/shared with backward-compatible aliases; removes local implementations of createChecklist, scoreChecklist, getAnswers, consolidateAnswers, and comparison utilities.
Web ROBINS-I Migration
packages/web/src/components/checklist/ROBINSIChecklist/checklist-map.js, packages/web/src/components/checklist/ROBINSIChecklist/checklist.js, packages/web/src/components/checklist/ROBINSIChecklist/scoring/robins-scoring.js
Re-export ROBINS-I schema, creation, scoring, and answer utilities from @corates/shared; removes ~1500 lines of local implementations while preserving public API surface.
Web Mock Components
packages/web/src/components/mocks/MockIndex.jsx, packages/web/src/components/mocks/AddStudiesWizard.jsx, packages/web/src/components/mocks/AddStudiesPanel.jsx, packages/web/src/components/mocks/AddStudiesInline.jsx, packages/web/src/components/mocks/ProjectViewEditorial.jsx, packages/web/src/components/mocks/ProjectViewDashboard.jsx, packages/web/src/components/mocks/ProjectViewKanban.jsx, packages/web/src/components/mocks/ProjectViewComplete.jsx
New mock UI components for study import workflows (wizard/panel/inline variants) and project views (editorial/dashboard/kanban/complete styles) with local state, mock data, and no external API calls.
Web Routing & Cleanup
packages/web/src/Routes.jsx, packages/web/src/components/mock/MockIndex.jsx
Updated mock routes to use lazy-loaded components under /mocks path; deleted legacy MockIndex.
Configuration
eslint.config.js, .claude/settings.json
Extended ESLint ignores to exclude mocks directories; added Claude settings file enabling frontend-design plugin.
Planning Documentation
packages/docs/audits/yjs-state-inspector-plan.md
Comprehensive 5-phase development plan for Yjs state inspection tooling covering shared infrastructure reorganization, dev-mode endpoints, CLI tools, frontend dev panel, and mock data generators.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

  • Rename some labels #176: Adds/updates local AMSTAR2 completeness logic in web package; directly overlaps with main PR's centralized isAMSTAR2Complete in shared module.
  • 137 checklist status improvements #138: Adds checklist domain helpers (isReconciledChecklist, getTodoChecklists, getCompletedChecklists, getNextStatusForCompletion) in web package that are identically implemented in main PR's shared domain.ts.
  • Robins i #50: Implements ROBINS-I checklist creation, scoring, answer extraction, and schema utilities in web package that the main PR centralizes and exposes from shared; directly code-level overlap.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 67.54% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the primary objective: moving checklist logic from web package to shared package for reusability.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/web/src/components/checklist/AMSTAR2Checklist/checklist.js (1)

30-70: CSV header/row column count mismatch.

The headers array has 8 columns but each row pushes 9 values. The critical field (line 62) is included in row data but missing from the headers array, causing column misalignment in the exported CSV.

🐛 Proposed fix
   const headers = [
     'Checklist Name',
     'Reviewer',
+    'Critical',
     'Question',
     'Question Text',
     'Column Label',
     'Option Text',
     'Selected',
     'Selected Answer',
   ];
🤖 Fix all issues with AI agents
In @eslint.config.js:
- Line 345: Replace the overly broad ignore glob '**/mocks/**' in
eslint.config.js with a more specific pattern so component mock files like
MockIndex.jsx still get linted; change the ignore entry to target only the UI
exploration folder (for example use 'packages/web/src/components/mocks/**' or an
equivalent project-specific path) to avoid excluding test mocks/fixtures while
still ignoring the intended temporary wireframes.

In @packages/docs/audits/yjs-state-inspector-plan.md:
- Around line 712-713: The file contains duplicated entries in the
"Questions/Decisions Needed" section: remove the duplicated lines "Do we want
the CLI tool to work with remote dev environments or just local?" and "Should
mock templates include realistic citation metadata or placeholder text?" (the
repeated block at lines 712-713) so only the original instances (lines ~656-657)
remain; simply delete the duplicate lines to eliminate redundancy.

In @packages/shared/src/checklists/amstar2/compare.ts:
- Around line 224-287: The multipart selection logic in
createReconciledChecklist can skip parts when selection is an AMSTAR2Question
object because the code only copies when selection[dataKey] exists; fix by (a)
tightening SelectionValue to explicitly type multipart selections (e.g., union
branches that require { q9a: AMSTAR2Question; q9b: AMSTAR2Question } and { q11a:
AMSTAR2Question; q11b: AMSTAR2Question }) and/or (b) adding runtime handling
inside createReconciledChecklist (and the isMultiPartQuestion branch) to detect
if selection is a single AMSTAR2Question object and, in that case, apply it to
all dataKeys for that multipart question (or otherwise fall back to
reviewer1/reviewer2), ensuring each dataKey (e.g., q9a/q9b, q11a/q11b) is always
assigned.

In @packages/shared/src/checklists/amstar2/schema.ts:
- Around line 21-29: There are two different symbols named AMSTAR2Question
causing confusion: the interface AMSTAR2Question in schema.ts (defines question
schema: info, text, columns, etc.) and the type AMSTAR2Question in types.ts
(aliases answer data). Rename the schema symbol to AMSTAR2QuestionSchema (or
similar), update its export in packages/shared/src/checklists/amstar2/schema.ts
and any re-exports (e.g., the module that does export * from './schema.js'), and
then update all imports that currently import the schema as AMSTAR2Question to
the new name; leave the answer type alias in types.ts unchanged so imports for
answer data remain AMSTAR2Question. Ensure tests/typescript compile after
renaming and adjust any references in documentation/comments.

In @packages/shared/src/checklists/amstar2/score.ts:
- Around line 25-55: scoreAMSTAR2Checklist currently skips entries when question
objects or their answers are missing/invalid, which can falsely yield a "High"
score; change the logic in scoreAMSTAR2Checklist so that if a consolidated entry
matching /^q\d+[a-z]*$/ has a missing question object, answers not an array, or
an empty answers array (inspect the questionData variable and
questionData.answers), the function returns 'Error' immediately (rather than
continue); keep using consolidateAnswers and getSelectedAnswer for normal flow
but treat these invalid payload cases as fatal input errors.

In @packages/shared/src/checklists/robins-i/answers.ts:
- Around line 173-183: The check in isQuestionnaireComplete incorrectly treats
missing entries as answered because `answers[key]?.answer !== null` is true when
answer is undefined; update the condition to ensure both null and undefined are
treated as missing (e.g., use a loose null check like `answers[key]?.answer !=
null` or explicitly verify the key exists and value is not null/undefined) when
iterating requiredKeys from getDomainQuestions so unanswered questions are not
considered complete.

In @packages/shared/src/checklists/types.ts:
- Around line 10-18: The types use two different discriminants which causes
inconsistent shapes: ChecklistMetadata currently has an optional type?:
'AMSTAR2' | 'ROBINS_I' while ROBINSIChecklist defines checklistType: 'ROBINS_I';
make the discriminant consistent by choosing one name and enforcing it across
implementations—preferably change ChecklistMetadata.type to a required field
(type: 'AMSTAR2' | 'ROBINS_I') and remove checklistType from ROBINSIChecklist
(or alternatively replace type usages with checklistType everywhere), then
update any consuming types/interfaces and type guards to use the single
discriminant (ChecklistMetadata.type or ROBINSIChecklist without checklistType)
so narrowing works correctly.

In @packages/web/src/components/mocks/AddStudiesInline.jsx:
- Around line 199-205: The onDrop handler currently only calls
setDragOver(false) and misses calling e.preventDefault(), so update the onDrop
prop (in the AddStudiesInline component) to accept the event parameter and call
e.preventDefault() before setDragOver(false); ensure the signature matches
onDrop={e => { e.preventDefault(); setDragOver(false); }} so the browser’s
default drop behavior is suppressed.

In @packages/web/src/components/mocks/MockIndex.jsx:
- Around line 185-197: The anchor in MockIndex.jsx uses
href='/mocks/robins-reconcile-section-b-question' but that route is not defined;
either add a route entry in Routes.jsx for path
'/mocks/robins-reconcile-section-b-question' that renders the new
RobinsReconcileSectionBQuestion mock component (import and register the
component in Routes.jsx), or remove/disable/change the <A href=...> link in
MockIndex.jsx until the component and route exist so the link no longer points
to a non-existent route.

In @packages/web/src/components/mocks/ProjectViewKanban.jsx:
- Line 349: Remove the stray empty string literal "" left in the JSX of
ProjectViewKanban (packages/web/src/components/mocks/ProjectViewKanban.jsx) —
delete the token and adjust surrounding commas or JSX children so the element
remains syntactically valid (no extra commas or empty children).
🧹 Nitpick comments (25)
packages/web/src/components/mocks/ProjectViewDashboard.jsx (2)

147-152: SVG gradient ID collision when multiple ProgressRing components render.

The id='gradient' is static, but multiple ProgressRing instances are rendered on the page. While browsers often handle this gracefully, it can cause inconsistent gradient rendering in some scenarios.

💡 Consider using unique gradient IDs
+import { createUniqueId } from 'solid-js';
+
 function ProgressRing(props) {
+  const gradientId = createUniqueId();
   const circumference = 2 * Math.PI * 18;
   const offset = circumference - (props.value / 100) * circumference;

   return (
     <div class='relative h-12 w-12'>
       <svg class='h-12 w-12 -rotate-90'>
         ...
         <circle
           ...
-          stroke='url(#gradient)'
+          stroke={`url(#${gradientId})`}
           ...
         />
         <defs>
-          <linearGradient id='gradient' x1='0%' y1='0%' x2='100%' y2='0%'>
+          <linearGradient id={gradientId} x1='0%' y1='0%' x2='100%' y2='0%'>
             ...
           </linearGradient>
         </defs>
       </svg>
       ...
     </div>
   );
 }

493-499: External font import blocks initial render.

Using @import inside a <style> tag for Google Fonts can delay first contentful paint. For mock components this is acceptable, but consider documenting this as a pattern to avoid in production.

packages/web/src/components/mocks/AddStudiesInline.jsx (1)

416-425: Remove commented-out code.

This commented-out "Add All" button should either be removed or implemented. Leaving dead code reduces readability.

packages/web/src/components/mocks/ProjectViewComplete.jsx (2)

645-659: Use For component instead of .map() for list rendering.

Per coding guidelines, SolidJS components should use For for rendering lists instead of Array.map().

♻️ Proposed fix
       <div class='grid grid-cols-4 gap-4'>
-        {[
-          { label: 'Total Studies', value: '24', change: '+3 this week' },
-          { label: 'High Confidence', value: '8', pct: '33%' },
-          { label: 'Moderate', value: '10', pct: '42%' },
-          { label: 'Low/Critically Low', value: '6', pct: '25%' },
-        ].map(stat => (
+        <For each={[
+          { label: 'Total Studies', value: '24', change: '+3 this week' },
+          { label: 'High Confidence', value: '8', pct: '33%' },
+          { label: 'Moderate', value: '10', pct: '42%' },
+          { label: 'Low/Critically Low', value: '6', pct: '25%' },
+        ]}>
+          {stat => (
           <div class='rounded-xl border border-slate-200 bg-white p-5'>
             ...
           </div>
-        ))}
+          )}
+        </For>
       </div>

The same pattern should be applied to lines 670-688 and 698-709.


317-317: Unused signal showUpload.

The showUpload signal is created but never used in the component.

🧹 Remove unused signal
 function StudiesTab() {
-  const [showUpload, setShowUpload] = createSignal(false);
-
   const unassignedStudies = createMemo(() => mockStudies.filter(s => s.assignedTo.length === 0));
packages/web/src/components/mocks/ProjectViewKanban.jsx (2)

232-234: Unused signal expanded in StudyCard.

The expanded signal is created but never toggled or read in the component logic.

🧹 Remove unused signal
 function StudyCard(props) {
-  const [expanded, setExpanded] = createSignal(false);
-
   return (

487-492: Remove the custom line-clamp-2 CSS definition—Tailwind CSS v4.1.18 provides the line-clamp-2 utility by default. The component is already using the Tailwind class on line 257; the custom CSS block at lines 487–492 duplicates this functionality and can be safely removed.

packages/web/src/components/mocks/ProjectViewEditorial.jsx (1)

243-268: Use For component instead of .map() for tab navigation.

Per coding guidelines, SolidJS components should use For for rendering lists instead of Array.map().

♻️ Proposed fix
+          const tabs = [
+            { id: 'all', label: 'All Studies', count: stats.total },
+            { id: 'todo', label: 'My Tasks', count: 3 },
+            { id: 'reconcile', label: 'Reconciliation', count: stats.reconciling },
+            { id: 'completed', label: 'Completed', count: stats.completed },
+          ];
+          
           <div class='flex gap-0'>
-            {[
-              { id: 'all', label: 'All Studies', count: stats.total },
-              { id: 'todo', label: 'My Tasks', count: 3 },
-              { id: 'reconcile', label: 'Reconciliation', count: stats.reconciling },
-              { id: 'completed', label: 'Completed', count: stats.completed },
-            ].map(tab => (
+            <For each={tabs}>
+              {tab => (
               <button
                 ...
               >
                 ...
               </button>
-            ))}
+              )}
+            </For>
           </div>
packages/web/src/components/mocks/AddStudiesWizard.jsx (2)

674-677: Hardcoded study count doesn't reflect actual data.

The button shows "Add 4 Studies" but the actual unique count from Step3ReviewDedupe could differ. Consider making this dynamic or adding a comment that this is intentionally static for the mock.

💡 Suggestion to make count dynamic

If you want the count to be dynamic, you'd need to lift the unique memo to the parent or pass a count prop. For a mock, a comment clarifying the static nature would also suffice.

-                    <button class='flex items-center gap-2 rounded-lg bg-violet-600 px-6 py-2 text-sm font-medium text-white transition-colors hover:bg-violet-700'>
-                      <FiCheck class='h-4 w-4' />
-                      Add 4 Studies
-                    </button>
+                    {/* Static count for mock - would be dynamic in real implementation */}
+                    <button class='flex items-center gap-2 rounded-lg bg-violet-600 px-6 py-2 text-sm font-medium text-white transition-colors hover:bg-violet-700'>
+                      <FiCheck class='h-4 w-4' />
+                      Add 4 Studies
+                    </button>

711-714: Avoid @import inside inline <style> tag for fonts.

Using @import for fonts inside a <style> block is render-blocking and can cause a flash of unstyled text (FOUT). For mock/demo components, consider relying on the app's existing font stack or moving font imports to the document <head>.

💡 Suggested fix

Remove the font import and rely on the system font stack that's already defined:

-      {/* Styles */}
-      <style>{`
-        @import url('https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700&display=swap');
-        body { font-family: 'Inter', -apple-system, BlinkMacSystemFont, sans-serif; }
-      `}</style>

The app likely already has Inter or a similar font configured at the root level. If Inter is required specifically for these mocks, add the <link> to the main HTML template instead.

packages/web/src/components/mocks/AddStudiesPanel.jsx (1)

586-589: Same font import issue - avoid @import in inline styles.

This has the same render-blocking concern as noted in AddStudiesWizard.jsx. Consider removing this and relying on the app's existing font configuration.

💡 Suggested fix
-      {/* Styles */}
-      <style>{`
-        @import url('https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700&display=swap');
-        body { font-family: 'Inter', -apple-system, BlinkMacSystemFont, sans-serif; }
-      `}</style>
packages/shared/src/checklists/amstar2/score.ts (2)

65-83: Use the same “final answer” derivation helper for completeness checks (avoid subtle drift).

isAMSTAR2Complete manually inspects the last column (Line 75-79). Consider reusing the existing answer-derivation helper (e.g., getSelectedAnswer/getFinalAnswer) so completeness and scoring stay aligned if label logic changes.


91-136: Avoid the second scoring pass + clarify whether breakdown keys should be consolidated (q9/q11) or part keys (q9a/q9b, q11a/q11b).

getScoreBreakdown already computes flaw lists, but then calls scoreAMSTAR2Checklist(checklist) (Line 134) which re-walks the checklist. Also, the returned criticalQuestions/nonCriticalQuestions are the consolidated keys produced by consolidateAnswers (likely q9/q11), which may not match UI expectations.

packages/shared/src/checklists/types.ts (1)

23-32: Rename AMSTAR2Question (answer state) to avoid collision with schema’s AMSTAR2Question (question definition).

This file exports AMSTAR2Question as an alias of AMSTAR2QuestionAnswer (Line 31), but packages/shared/src/checklists/amstar2/schema.ts also defines an AMSTAR2Question interface for question definitions. This is very easy to import wrong.

Suggested rename: AMSTAR2QuestionState / AMSTAR2QuestionAnswerState.

packages/shared/src/checklists/amstar2/compare.ts (1)

90-152: Consider surfacing missing question data as “disagreement/incomplete” instead of skipping it.

compareChecklists continues when parts or questions are missing (Line 116, 129), which reduces stats.total and can hide data issues. If missing data can happen, representing it explicitly in the result tends to be easier for UIs to handle.

packages/shared/src/checklists/amstar2/create.ts (1)

170-175: createEmptyQuestion helper is defined but unused internally.

The createEmptyQuestion helper function is exported but not used within createAMSTAR2Checklist. The checklist creation uses hard-coded structures instead.

This is fine if the helper is intended for external consumers, but consider using it internally to reduce the verbosity of the question definitions in createAMSTAR2Checklist:

♻️ Optional refactor example
-    q1: { answers: [[false, false, false, false], [false], [false, false]], critical: false },
+    q1: createEmptyQuestion(false, [4, 1, 2]),
packages/shared/src/checklists/amstar2/answers.ts (2)

75-90: Consolidation logic may not handle all answer combinations correctly.

The consolidation for q9a/q9b (and similarly q11a/q11b) doesn't explicitly handle all AMSTAR2 answer combinations. For instance, when one answer is "Partial Yes" and the other is "No MA", or when both are "Partial Yes", the logic falls through to the else branch and uses q9a by default.

Per AMSTAR2 methodology, consolidation should select the "lower" score. Consider making the precedence explicit:

  • No (lowest)
  • No MA
  • Partial Yes
  • Yes (highest)
♻️ Suggested explicit score ranking
+const ANSWER_RANK: Record<string, number> = {
+  'No': 0,
+  'No MA': 1,
+  'Partial Yes': 2,
+  'Yes': 3,
+};
+
+function getLowerScoreAnswer(
+  answerA: AnswerLabel,
+  answerB: AnswerLabel,
+  questionA: unknown,
+  questionB: unknown
+): unknown {
+  if (answerA === null || answerB === null) return questionA;
+  const rankA = ANSWER_RANK[answerA] ?? 3;
+  const rankB = ANSWER_RANK[answerB] ?? 3;
+  return rankA <= rankB ? questionA : questionB;
+}

Based on learnings, multi-part AMSTAR2 questions (q9a/b, q11a/b) require separate answer entries and careful consolidation.


120-122: getFinalAnswer is a thin wrapper with no added value.

This function simply delegates to getSelectedAnswer without any transformation. Consider whether this indirection is necessary or if consumers should call getSelectedAnswer directly.

packages/shared/src/checklists/robins-i/scoring.ts (1)

1005-1011: OVERALL_DISPLAY is missing a LOW entry.

The OVERALL_DISPLAY constant has LOW_EXCEPT_CONFOUNDING, MODERATE, SERIOUS, and CRITICAL, but no plain LOW entry. This appears intentional based on mapOverallJudgementToDisplay which maps both LOW and LOW_EXCEPT_CONFOUNDING to the same display string, but consider adding a comment explaining this design decision.

packages/shared/src/checklists/robins-i/answers.ts (1)

27-43: Type assertion for scoreAllDomains parameter.

Line 34-36 uses as unknown as Parameters<typeof scoreAllDomains>[0] for the type assertion. While this works, it bypasses type checking. Consider ensuring the ROBINSIChecklist type properly extends or is compatible with the ChecklistState interface expected by scoreAllDomains.

packages/shared/src/checklists/robins-i/schema.ts (1)

591-606: Clarify: Section B excluded from getDomainQuestions.

Line 594 explicitly excludes sectionB when retrieving domain questions. This appears intentional since Section B ("Decide whether to proceed") is a screening section rather than a bias domain, but the comment could clarify this distinction.

 export function getDomainQuestions(domainKey: string): Record<string, ROBINSQuestion> {
   const domain = ROBINS_I_CHECKLIST[domainKey as keyof typeof ROBINS_I_CHECKLIST];
-  if (!domain || domainKey === 'sectionB') return {};
+  // Section B is a screening section, not a bias domain - skip it
+  if (!domain || domainKey === 'sectionB') return {};
packages/docs/audits/yjs-state-inspector-plan.md (4)

37-99: Add language specifier to fenced code block.

The Yjs document structure diagram would benefit from a language specifier for better rendering and syntax highlighting.

-```
+```text
 Project (Y.Doc persisted in DO storage as 'yjs-state')

167-183: Add language specifier to fenced code block.

-```
+```text
 packages/shared/src/checklists/

317-324: Add language specifier to fenced code block.

-```
+```text
 DevPanel/

450-477: Add language specifier to fenced code block.

-```
+```text
 +------------------------------------------+

Comment thread eslint.config.js
'**/.vinxi/**',
'**/.output/**',
'**/coverage/**',
'**/mocks/**',
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the nature of mock files and the scope of the ignore pattern

echo "=== Mocks directories found ==="
fd -t d '^mocks$'

echo -e "\n=== Count of files in mocks directories ==="
fd -t f . '**/mocks/**' | wc -l

echo -e "\n=== Sample files from mocks directories ==="
fd -t f . '**/mocks/**' | head -20

echo -e "\n=== Check for component patterns in mock files ==="
rg -t js -t jsx 'export (default )?(function|const|class)' '**/mocks/**' | head -15

echo -e "\n=== Check eslint.config.js context around line 345 ==="
sed -n '340,350p' eslint.config.js

Repository: InfinityBowman/corates

Length of output: 837


🏁 Script executed:

#!/bin/bash
# Search for mock-related patterns and files more broadly

echo "=== All directories containing 'mock' in name ==="
fd -t d -i 'mock' | head -20

echo -e "\n=== Search for mock files anywhere (js/ts) ==="
fd -t f -i 'mock' -e js -e ts -e tsx -e jsx | head -20

echo -e "\n=== Check packages/workers test setup mentioned in learnings ==="
cat packages/workers/src/__tests__/setup.js 2>/dev/null | head -30

echo -e "\n=== Git status to see if mocks directory has staged/new files ==="
git status --short packages/web/src/components/mocks/ 2>/dev/null || echo "Directory appears clean"

echo -e "\n=== Look for MSW mocks or test fixtures ==="
find . -name '*mock*' -o -name '*fixture*' -o -name '*stub*' 2>/dev/null | grep -E '\.(js|ts|tsx|jsx)$' | head -20

Repository: InfinityBowman/corates

Length of output: 1272


🏁 Script executed:

#!/bin/bash
# Check the content and nature of MockIndex.jsx

echo "=== Content of MockIndex.jsx ==="
cat packages/web/src/components/mocks/MockIndex.jsx

echo -e "\n=== File size and line count ==="
wc -l packages/web/src/components/mocks/MockIndex.jsx

echo -e "\n=== Check git history for this file ==="
git log --oneline -5 packages/web/src/components/mocks/MockIndex.jsx 2>/dev/null || echo "File is new or untracked"

Repository: InfinityBowman/corates

Length of output: 9363


Use a more specific ignore pattern for mock components.

The pattern **/mocks/** ignores all files in any directory named "mocks" throughout the project. MockIndex.jsx is actual component code (202 lines of JSX with Tailwind styling), not generated files. Even though these are marked as "temporary wireframes," they're still component files that could benefit from linting to catch syntax errors, unused variables, or style violations.

Consider using packages/web/src/components/mocks/** instead of the broad glob, which would explicitly target only these UI exploration files while avoiding unintended matches against test mocks or fixtures elsewhere in the project.

🤖 Prompt for AI Agents
In @eslint.config.js at line 345, Replace the overly broad ignore glob
'**/mocks/**' in eslint.config.js with a more specific pattern so component mock
files like MockIndex.jsx still get linted; change the ignore entry to target
only the UI exploration folder (for example use
'packages/web/src/components/mocks/**' or an equivalent project-specific path)
to avoid excluding test mocks/fixtures while still ignoring the intended
temporary wireframes.

Comment on lines +712 to +713
3. Do we want the CLI tool to work with remote dev environments or just local?
4. Should mock templates include realistic citation metadata or placeholder text?
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove duplicate content.

Lines 712-713 appear to be accidentally duplicated from the "Questions/Decisions Needed" section (lines 656-657).

-3. Do we want the CLI tool to work with remote dev environments or just local?
-4. Should mock templates include realistic citation metadata or placeholder text?
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
3. Do we want the CLI tool to work with remote dev environments or just local?
4. Should mock templates include realistic citation metadata or placeholder text?
🤖 Prompt for AI Agents
In @packages/docs/audits/yjs-state-inspector-plan.md around lines 712 - 713, The
file contains duplicated entries in the "Questions/Decisions Needed" section:
remove the duplicated lines "Do we want the CLI tool to work with remote dev
environments or just local?" and "Should mock templates include realistic
citation metadata or placeholder text?" (the repeated block at lines 712-713) so
only the original instances (lines ~656-657) remain; simply delete the duplicate
lines to eliminate redundancy.

Comment on lines +224 to +287
type SelectionValue = 'reviewer1' | 'reviewer2' | AMSTAR2Question | Record<string, AMSTAR2Question>;

interface ReconciledMetadata {
name?: string;
reviewerName?: string;
createdAt?: string;
id?: string;
}

/**
* Create a merged/reconciled checklist from two source checklists.
*/
export function createReconciledChecklist(
checklist1: AMSTAR2Checklist,
checklist2: AMSTAR2Checklist,
selections: Record<string, SelectionValue>,
metadata: ReconciledMetadata = {},
): AMSTAR2Checklist & { sourceChecklists: string[] } {
const questionKeys = getQuestionKeys();

const reconciled: Record<string, unknown> = {
name: metadata.name || 'Reconciled Checklist',
reviewerName: metadata.reviewerName || 'Consensus',
createdAt: metadata.createdAt || new Date().toISOString().split('T')[0],
id: metadata.id || `reconciled-${Date.now()}`,
sourceChecklists: [checklist1.id, checklist2.id],
};

for (const key of questionKeys) {
const selection = selections[key];
const dataKeys = getDataKeysForQuestion(key);

if (isMultiPartQuestion(key)) {
for (const dataKey of dataKeys) {
if (!selection || selection === 'reviewer1') {
reconciled[dataKey] = JSON.parse(
JSON.stringify(checklist1[dataKey as keyof AMSTAR2Checklist]),
);
} else if (selection === 'reviewer2') {
reconciled[dataKey] = JSON.parse(
JSON.stringify(checklist2[dataKey as keyof AMSTAR2Checklist]),
);
} else if (
typeof selection === 'object' &&
(selection as Record<string, unknown>)[dataKey]
) {
reconciled[dataKey] = JSON.parse(
JSON.stringify((selection as Record<string, unknown>)[dataKey]),
);
}
}
} else {
if (!selection || selection === 'reviewer1') {
reconciled[key] = JSON.parse(JSON.stringify(checklist1[key as keyof AMSTAR2Checklist]));
} else if (selection === 'reviewer2') {
reconciled[key] = JSON.parse(JSON.stringify(checklist2[key as keyof AMSTAR2Checklist]));
} else if (typeof selection === 'object') {
reconciled[key] = JSON.parse(JSON.stringify(selection));
}
}
}

return reconciled as unknown as AMSTAR2Checklist & { sourceChecklists: string[] };
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix multipart selection handling: SelectionValue allows shapes that lead to partially-empty reconciled checklists.

For multipart keys (q9, q11), SelectionValue allows AMSTAR2Question, but the code only assigns a part when selection[dataKey] exists (Line 266-273). If a caller passes an AMSTAR2Question for q9, this will silently skip both q9a/q9b assignments.

Recommendation: use a stricter type for multipart selections (e.g. { q9a: AMSTAR2Question; q9b: AMSTAR2Question } / { q11a: ...; q11b: ... }) or explicitly handle AMSTAR2Question by applying it to both parts (if that’s intended).

One way to tighten types + runtime behavior
-type SelectionValue = 'reviewer1' | 'reviewer2' | AMSTAR2Question | Record<string, AMSTAR2Question>;
+type MultiPartSelection = Record<string, AMSTAR2Question>; // ideally narrow per key
+type SelectionValue = 'reviewer1' | 'reviewer2' | AMSTAR2Question | MultiPartSelection;

 ...
     if (isMultiPartQuestion(key)) {
       for (const dataKey of dataKeys) {
         if (!selection || selection === 'reviewer1') {
           reconciled[dataKey] = JSON.parse(
             JSON.stringify(checklist1[dataKey as keyof AMSTAR2Checklist]),
           );
         } else if (selection === 'reviewer2') {
           reconciled[dataKey] = JSON.parse(
             JSON.stringify(checklist2[dataKey as keyof AMSTAR2Checklist]),
           );
-        } else if (
-          typeof selection === 'object' &&
-          (selection as Record<string, unknown>)[dataKey]
-        ) {
+        } else if (typeof selection === 'object' && 'answers' in selection) {
+          // If caller passed a single answer object for a multi-part question, apply it to each part
+          reconciled[dataKey] = JSON.parse(JSON.stringify(selection));
+        } else if (typeof selection === 'object' && (selection as Record<string, unknown>)[dataKey]) {
           reconciled[dataKey] = JSON.parse(
             JSON.stringify((selection as Record<string, unknown>)[dataKey]),
           );
         }
       }
     } else {
🤖 Prompt for AI Agents
In @packages/shared/src/checklists/amstar2/compare.ts around lines 224 - 287,
The multipart selection logic in createReconciledChecklist can skip parts when
selection is an AMSTAR2Question object because the code only copies when
selection[dataKey] exists; fix by (a) tightening SelectionValue to explicitly
type multipart selections (e.g., union branches that require { q9a:
AMSTAR2Question; q9b: AMSTAR2Question } and { q11a: AMSTAR2Question; q11b:
AMSTAR2Question }) and/or (b) adding runtime handling inside
createReconciledChecklist (and the isMultiPartQuestion branch) to detect if
selection is a single AMSTAR2Question object and, in that case, apply it to all
dataKeys for that multipart question (or otherwise fall back to
reviewer1/reviewer2), ensuring each dataKey (e.g., q9a/q9b, q11a/q11b) is always
assigned.

Comment on lines +21 to +29
export interface AMSTAR2Question {
info: string;
text: string;
columns: AMSTAR2Column[];
subtitle?: string;
subtitle2?: string;
columns2?: AMSTAR2Column[];
options?: string[];
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# First, find the exact files mentioned
echo "=== Looking for schema.ts ==="
find . -name "schema.ts" -path "*/amstar2/*" | head -20

echo -e "\n=== Looking for types.ts ==="
find . -name "types.ts" -path "*/checklists/*" | head -20

Repository: InfinityBowman/corates

Length of output: 218


🏁 Script executed:

#!/bin/bash

# Check the actual content of schema.ts around lines 21-29
echo "=== Content of AMSTAR2 schema.ts ==="
fd "schema.ts" -path "*/amstar2/*" --exec cat -n {} \; | head -100

echo -e "\n=== Content of types.ts (checklists) ==="
fd "types.ts" -path "*/checklists/*" -not -path "*/node_modules/*" --exec cat -n {} \; | head -150

Repository: InfinityBowman/corates

Length of output: 499


🏁 Script executed:

#!/bin/bash

# Search for all AMSTAR2Question usages
echo "=== AMSTAR2Question usage across codebase ==="
rg "AMSTAR2Question" -n --type ts --type tsx -B1 -A1 | head -100

echo -e "\n=== Check imports of AMSTAR2Question ==="
rg "import.*AMSTAR2Question|from.*schema|from.*types" -n --type ts --type tsx | head -50

Repository: InfinityBowman/corates

Length of output: 226


🏁 Script executed:

#!/bin/bash

# Read schema.ts directly
echo "=== Content of packages/shared/src/checklists/amstar2/schema.ts ==="
cat -n ./packages/shared/src/checklists/amstar2/schema.ts | head -100

echo -e "\n=== Content of packages/shared/src/checklists/types.ts ==="
cat -n ./packages/shared/src/checklists/types.ts | head -50

Repository: InfinityBowman/corates

Length of output: 6142


🏁 Script executed:

#!/bin/bash

# Search for AMSTAR2Question usages with correct flags
echo "=== AMSTAR2Question usage ==="
rg "AMSTAR2Question" -n -B1 -A1 | head -100

echo -e "\n=== Import patterns ==="
rg "import.*AMSTAR2Question" -n | head -50

Repository: InfinityBowman/corates

Length of output: 7537


🏁 Script executed:

#!/bin/bash

# Check what's exported from both files
echo "=== Exports from schema.ts ==="
rg "^export.*AMSTAR2Question" packages/shared/src/checklists/amstar2/schema.ts -n

echo -e "\n=== Exports from types.ts ==="
rg "^export.*AMSTAR2Question" packages/shared/src/checklists/types.ts -n

echo -e "\n=== Check if types.ts imports from schema.ts ==="
rg "from.*schema" packages/shared/src/checklists/types.ts -n

Repository: InfinityBowman/corates

Length of output: 385


🏁 Script executed:

#!/bin/bash

# Check where schema.ts is imported/used
echo "=== Imports of schema.ts ==="
rg "from.*schema" --max-count=20

echo -e "\n=== Check if AMSTAR2Question from schema.ts is ever used ==="
rg "import.*AMSTAR2Question.*from.*schema" -n

echo -e "\n=== Check AMSTAR_CHECKLIST usage (which uses schema's AMSTAR2Question) ==="
rg "AMSTAR_CHECKLIST" -n | head -20

Repository: InfinityBowman/corates

Length of output: 18565


🏁 Script executed:

#!/bin/bash

# Check what's in checklist-map.js
echo "=== Content of checklist-map.js ==="
cat packages/web/src/components/checklist/AMSTAR2Checklist/checklist-map.js

echo -e "\n=== Check if schema.ts AMSTAR_CHECKLIST is actually exported and used ==="
rg "export.*AMSTAR_CHECKLIST|import.*AMSTAR_CHECKLIST.*from.*schema" -n

Repository: InfinityBowman/corates

Length of output: 1558


🏁 Script executed:

#!/bin/bash

# Verify the amstar2 export from @corates/shared
echo "=== Check @corates/shared amstar2 export ==="
find . -path "*/packages/shared/src/checklists/amstar2/index.ts" -exec cat {} \;

echo -e "\n=== Verify no cross-contamination of schema vs types ==="
rg "AMSTAR2Question" -B2 -A2 packages/shared/src/checklists/amstar2/create.ts | head -30

Repository: InfinityBowman/corates

Length of output: 961


🏁 Script executed:

#!/bin/bash

# Final verification - check the amstar2 index export
echo "=== Check what amstar2 module exports ==="
cat packages/shared/src/checklists/amstar2/index.ts

Repository: InfinityBowman/corates

Length of output: 536


🏁 Script executed:

#!/bin/bash

# Final check - what does amstar2 module actually export for AMSTAR2Question
echo "=== Simulate importing from @corates/shared amstar2 ==="
rg "export.*AMSTAR2Question|export type AMSTAR2Question|export interface AMSTAR2Question" packages/shared/src/checklists/amstar2/ -n

echo -e "\n=== Check if schema and types are both in export chain ==="
rg "export" packages/shared/src/checklists/amstar2/index.ts

Repository: InfinityBowman/corates

Length of output: 687


Rename one AMSTAR2Question to avoid naming confusion.

The codebase has two incompatible definitions with the same name:

  • packages/shared/src/checklists/amstar2/schema.ts (line 21): interface AMSTAR2Question describing question structure (info, text, columns)
  • packages/shared/src/checklists/types.ts (line 31): type AMSTAR2Question aliasing answer data structure (boolean matrix + critical flag)

The amstar2 module exports the schema interface via export * from './schema.js', while answer operations import the type alias from types.ts. This creates potential for developer confusion when selecting the correct import. Consider renaming the schema interface to AMSTAR2QuestionSchema or similar to clearly distinguish question structure from answer data.

🤖 Prompt for AI Agents
In @packages/shared/src/checklists/amstar2/schema.ts around lines 21 - 29, There
are two different symbols named AMSTAR2Question causing confusion: the interface
AMSTAR2Question in schema.ts (defines question schema: info, text, columns,
etc.) and the type AMSTAR2Question in types.ts (aliases answer data). Rename the
schema symbol to AMSTAR2QuestionSchema (or similar), update its export in
packages/shared/src/checklists/amstar2/schema.ts and any re-exports (e.g., the
module that does export * from './schema.js'), and then update all imports that
currently import the schema as AMSTAR2Question to the new name; leave the answer
type alias in types.ts unchanged so imports for answer data remain
AMSTAR2Question. Ensure tests/typescript compile after renaming and adjust any
references in documentation/comments.

Comment on lines +25 to +55
export function scoreAMSTAR2Checklist(checklist: AMSTAR2Checklist): AMSTAR2Score {
if (!checklist || typeof checklist !== 'object') return 'Error';

let criticalFlaws = 0;
let nonCriticalFlaws = 0;

const consolidated = consolidateAnswers(checklist);

for (const [question, obj] of Object.entries(consolidated)) {
if (!/^q\d+[a-z]*$/i.test(question)) continue;
const questionData = obj as AMSTAR2Question;
if (!questionData || !Array.isArray(questionData.answers)) continue;

const selected = getSelectedAnswer(questionData.answers, question);

// Only count as flaw if answer is missing or "No"
// "Yes", "Partial Yes", and "No MA" are not flaws
if (!selected || selected === 'No') {
if (questionData.critical) {
criticalFlaws++;
} else {
nonCriticalFlaws++;
}
}
}

if (criticalFlaws > 1) return 'Critically Low';
if (criticalFlaws === 1) return 'Low';
if (nonCriticalFlaws > 1) return 'Moderate';
return 'High';
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t silently ignore missing/invalid question objects during scoring (can produce incorrect “High”).

scoreAMSTAR2Checklist skips entries where questionData.answers isn’t an array (Line 36), which contradicts “missing answer is a flaw” (Line 40) and can yield optimistic scores when data is corrupted/incomplete.

Consider treating “missing question object / missing answers matrix / empty matrix” as 'Error' (or as a flaw) instead of continue.

Proposed tightening (treat invalid question payload as Error)
 export function scoreAMSTAR2Checklist(checklist: AMSTAR2Checklist): AMSTAR2Score {
   if (!checklist || typeof checklist !== 'object') return 'Error';
 
   let criticalFlaws = 0;
   let nonCriticalFlaws = 0;
 
   const consolidated = consolidateAnswers(checklist);
 
   for (const [question, obj] of Object.entries(consolidated)) {
     if (!/^q\d+[a-z]*$/i.test(question)) continue;
     const questionData = obj as AMSTAR2Question;
-    if (!questionData || !Array.isArray(questionData.answers)) continue;
+    if (!questionData || !Array.isArray(questionData.answers) || questionData.answers.length === 0) {
+      return 'Error';
+    }
 
     const selected = getSelectedAnswer(questionData.answers, question);
 
     // Only count as flaw if answer is missing or "No"
     // "Yes", "Partial Yes", and "No MA" are not flaws
     if (!selected || selected === 'No') {
       if (questionData.critical) {
         criticalFlaws++;
       } else {
         nonCriticalFlaws++;
       }
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function scoreAMSTAR2Checklist(checklist: AMSTAR2Checklist): AMSTAR2Score {
if (!checklist || typeof checklist !== 'object') return 'Error';
let criticalFlaws = 0;
let nonCriticalFlaws = 0;
const consolidated = consolidateAnswers(checklist);
for (const [question, obj] of Object.entries(consolidated)) {
if (!/^q\d+[a-z]*$/i.test(question)) continue;
const questionData = obj as AMSTAR2Question;
if (!questionData || !Array.isArray(questionData.answers)) continue;
const selected = getSelectedAnswer(questionData.answers, question);
// Only count as flaw if answer is missing or "No"
// "Yes", "Partial Yes", and "No MA" are not flaws
if (!selected || selected === 'No') {
if (questionData.critical) {
criticalFlaws++;
} else {
nonCriticalFlaws++;
}
}
}
if (criticalFlaws > 1) return 'Critically Low';
if (criticalFlaws === 1) return 'Low';
if (nonCriticalFlaws > 1) return 'Moderate';
return 'High';
}
export function scoreAMSTAR2Checklist(checklist: AMSTAR2Checklist): AMSTAR2Score {
if (!checklist || typeof checklist !== 'object') return 'Error';
let criticalFlaws = 0;
let nonCriticalFlaws = 0;
const consolidated = consolidateAnswers(checklist);
for (const [question, obj] of Object.entries(consolidated)) {
if (!/^q\d+[a-z]*$/i.test(question)) continue;
const questionData = obj as AMSTAR2Question;
if (!questionData || !Array.isArray(questionData.answers) || questionData.answers.length === 0) {
return 'Error';
}
const selected = getSelectedAnswer(questionData.answers, question);
// Only count as flaw if answer is missing or "No"
// "Yes", "Partial Yes", and "No MA" are not flaws
if (!selected || selected === 'No') {
if (questionData.critical) {
criticalFlaws++;
} else {
nonCriticalFlaws++;
}
}
}
if (criticalFlaws > 1) return 'Critically Low';
if (criticalFlaws === 1) return 'Low';
if (nonCriticalFlaws > 1) return 'Moderate';
return 'High';
}
🤖 Prompt for AI Agents
In @packages/shared/src/checklists/amstar2/score.ts around lines 25 - 55,
scoreAMSTAR2Checklist currently skips entries when question objects or their
answers are missing/invalid, which can falsely yield a "High" score; change the
logic in scoreAMSTAR2Checklist so that if a consolidated entry matching
/^q\d+[a-z]*$/ has a missing question object, answers not an array, or an empty
answers array (inspect the questionData variable and questionData.answers), the
function returns 'Error' immediately (rather than continue); keep using
consolidateAnswers and getSelectedAnswer for normal flow but treat these invalid
payload cases as fatal input errors.

Comment on lines +173 to +183
function isQuestionnaireComplete(
domainKey: string,
answers: Record<string, { answer: string | null }> | undefined,
): boolean {
if (!answers) return false;

const questions = getDomainQuestions(domainKey);
const requiredKeys = Object.keys(questions);

return requiredKeys.every(key => answers[key]?.answer !== null);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

isQuestionnaireComplete may incorrectly consider undefined answers as complete.

The condition answers[key]?.answer !== null returns true when the answer is undefined (because undefined !== null is true). This means questions without an answer entry would incorrectly be considered complete.

🐛 Proposed fix
-  return requiredKeys.every(key => answers[key]?.answer !== null);
+  return requiredKeys.every(key => answers[key]?.answer != null);

Using loose equality (!= null) checks for both null and undefined.

🤖 Prompt for AI Agents
In @packages/shared/src/checklists/robins-i/answers.ts around lines 173 - 183,
The check in isQuestionnaireComplete incorrectly treats missing entries as
answered because `answers[key]?.answer !== null` is true when answer is
undefined; update the condition to ensure both null and undefined are treated as
missing (e.g., use a loose null check like `answers[key]?.answer != null` or
explicitly verify the key exists and value is not null/undefined) when iterating
requiredKeys from getDomainQuestions so unanswered questions are not considered
complete.

Comment on lines +10 to +18
export interface ChecklistMetadata {
id: string;
name: string;
reviewerName: string;
createdAt: string;
assignedTo?: string | null;
status?: ChecklistStatus;
type?: 'AMSTAR2' | 'ROBINS_I';
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Unify the checklist type discriminator (type vs checklistType) to avoid downstream bugs.

ChecklistMetadata defines type?: 'AMSTAR2' | 'ROBINS_I' (Line 17), but ROBINSIChecklist adds checklistType: 'ROBINS_I' (Line 111) instead of making type required. This encourages inconsistent shapes and makes narrowing harder.

Suggested: make type a required discriminant on all checklist types (and remove/avoid checklistType), or consistently use checklistType everywhere.

Also applies to: 110-147

🤖 Prompt for AI Agents
In @packages/shared/src/checklists/types.ts around lines 10 - 18, The types use
two different discriminants which causes inconsistent shapes: ChecklistMetadata
currently has an optional type?: 'AMSTAR2' | 'ROBINS_I' while ROBINSIChecklist
defines checklistType: 'ROBINS_I'; make the discriminant consistent by choosing
one name and enforcing it across implementations—preferably change
ChecklistMetadata.type to a required field (type: 'AMSTAR2' | 'ROBINS_I') and
remove checklistType from ROBINSIChecklist (or alternatively replace type usages
with checklistType everywhere), then update any consuming types/interfaces and
type guards to use the single discriminant (ChecklistMetadata.type or
ROBINSIChecklist without checklistType) so narrowing works correctly.

Comment on lines +199 to +205
onDragOver={e => {
e.preventDefault();
setDragOver(true);
}}
onDragLeave={() => setDragOver(false)}
onDrop={() => setDragOver(false)}
>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing e.preventDefault() in drop handler.

The onDrop handler doesn't call e.preventDefault(), which means the browser may still attempt its default drop behavior (like navigating to the dropped file).

🐛 Proposed fix
-        onDrop={() => setDragOver(false)}
+        onDrop={e => {
+          e.preventDefault();
+          setDragOver(false);
+        }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
onDragOver={e => {
e.preventDefault();
setDragOver(true);
}}
onDragLeave={() => setDragOver(false)}
onDrop={() => setDragOver(false)}
>
onDragOver={e => {
e.preventDefault();
setDragOver(true);
}}
onDragLeave={() => setDragOver(false)}
onDrop={e => {
e.preventDefault();
setDragOver(false);
}}
>
🤖 Prompt for AI Agents
In @packages/web/src/components/mocks/AddStudiesInline.jsx around lines 199 -
205, The onDrop handler currently only calls setDragOver(false) and misses
calling e.preventDefault(), so update the onDrop prop (in the AddStudiesInline
component) to accept the event parameter and call e.preventDefault() before
setDragOver(false); ensure the signature matches onDrop={e => {
e.preventDefault(); setDragOver(false); }} so the browser’s default drop
behavior is suppressed.

Comment on lines +185 to +197
<A
href='/mocks/robins-reconcile-section-b-question'
class='block rounded-lg border border-gray-200 bg-white p-6 shadow-sm transition-shadow hover:shadow-md'
>
<h3 class='mb-2 text-lg font-semibold text-gray-900'>
ROBINS-I Section B Question Reconcile
</h3>
<p class='text-sm text-gray-600'>
Three-column compare view for a single Section B question with always-visible
comments.
</p>
</A>
</div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Link references non-existent route.

The link to /mocks/robins-reconcile-section-b-question points to a route that isn't defined in Routes.jsx. Either add the route or remove this link until the component is available.

🤖 Prompt for AI Agents
In @packages/web/src/components/mocks/MockIndex.jsx around lines 185 - 197, The
anchor in MockIndex.jsx uses href='/mocks/robins-reconcile-section-b-question'
but that route is not defined; either add a route entry in Routes.jsx for path
'/mocks/robins-reconcile-section-b-question' that renders the new
RobinsReconcileSectionBQuestion mock component (import and register the
component in Routes.jsx), or remove/disable/change the <A href=...> link in
MockIndex.jsx until the component and route exist so the link no longer points
to a non-existent route.

<div class='flex items-center justify-between'>
{/* Left: Back + Project Info */}
<div class='flex items-center gap-4'>
""
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove empty string literal.

There's an empty string literal "" that appears to be leftover from editing.

🐛 Proposed fix
             <div class='flex items-center gap-4'>
-                ""
               <button class='flex h-9 w-9 items-center justify-center rounded-lg border border-slate-200 bg-white text-slate-600 shadow-sm transition-all hover:bg-slate-50 hover:text-slate-900'>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
""
🤖 Prompt for AI Agents
In @packages/web/src/components/mocks/ProjectViewKanban.jsx at line 349, Remove
the stray empty string literal "" left in the JSX of ProjectViewKanban
(packages/web/src/components/mocks/ProjectViewKanban.jsx) — delete the token and
adjust surrounding commas or JSX children so the element remains syntactically
valid (no extra commas or empty children).

@InfinityBowman InfinityBowman merged commit bacdb7d into main Jan 9, 2026
2 of 4 checks passed
@InfinityBowman InfinityBowman deleted the 266-importexport-project-data branch January 9, 2026 20:20
@github-actions github-actions Bot restored the 266-importexport-project-data branch January 9, 2026 20:21
@coderabbitai coderabbitai Bot mentioned this pull request Jan 11, 2026
@InfinityBowman InfinityBowman deleted the 266-importexport-project-data branch January 11, 2026 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import/Export project data

2 participants