338 multiple appraisals per study per user#352
Merged
Conversation
…github.com/InfinityBowman/corates into 338-multiple-appraisals-per-study-per-user
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 26. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
📝 WalkthroughWalkthroughThis PR introduces outcome management to the project review system, enabling checklists to be associated with specific outcomes (for ROB2 and ROBINS_I types). It adds outcome CRUD operations, updates data models and domain logic to group checklists by outcome, and extends UI across multiple tabs (todo, reconcile, completed) to support multi-outcome workflows for creating, reviewing, and completing outcome-specific checklists. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant OutcomeUI as Outcome Manager<br/>(UI)
participant ProjectStore as Project Store<br/>(Outcomes Ops)
participant ChecklistUI as Checklist Form<br/>(UI)
participant ChecklistOps as Checklist Ops<br/>(Domain)
participant ReconciliationOps as Reconciliation Ops<br/>(Domain)
User->>OutcomeUI: Create Outcome (name)
OutcomeUI->>ProjectStore: createOutcome(name, userId)
ProjectStore->>ProjectStore: Generate UUID, store in meta.outcomes
ProjectStore-->>OutcomeUI: Return outcomeId
OutcomeUI-->>User: Display outcome in list
User->>ChecklistUI: Select type (ROB2/ROBINS_I)
ChecklistUI->>ProjectStore: getOutcomes()
ProjectStore-->>ChecklistUI: Filter available outcomes
ChecklistUI->>ChecklistUI: Show outcome selector
User->>ChecklistUI: Select outcome & assign reviewer
ChecklistUI->>ChecklistOps: createChecklist(studyId, type, reviewer, outcomeId)
ChecklistOps->>ChecklistOps: Validate outcomeId required
ChecklistOps->>ProjectStore: Store checklist with outcomeId
ChecklistOps-->>ChecklistUI: Return checklistId
ChecklistUI-->>User: Checklist created
User->>ReconciliationOps: Open Reconcile tab
ReconciliationOps->>ReconciliationOps: Group checklists by outcome
ReconciliationOps->>ReconciliationOps: Filter pairs with 2 checklists
ReconciliationOps-->>User: Display outcomes needing reconciliation
User->>ReconciliationOps: Start reconciliation for outcome
ReconciliationOps->>ReconciliationOps: saveReconciliationProgress(studyId, outcomeId, type, data)
ReconciliationOps->>ProjectStore: Store in reconciliations[outcomeKey]
ReconciliationOps-->>User: Track reconciliation progress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 | 3f9bf44 | Commit Preview URL | Feb 02 2026, 04:54 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: 4
🤖 Fix all issues with AI agents
In `@packages/web/src/components/project/completed-tab/CompletedOutcomeRow.jsx`:
- Around line 25-87: The Open button can call props.onOpenChecklist with
undefined when there is no finalized checklist; guard it by checking
finalizedChecklist()?.id before enabling or invoking the action. Update the Open
button rendering/handler in CompletedOutcomeRow.jsx to (a) compute const
checklistId = finalizedChecklist()?.id and use that for both the button disabled
state and the onClick only if truthy, or (b) hide the button when checklistId is
falsy; adjust the onClick to call props.onOpenChecklist(checklistId) only when
checklistId exists so onOpenChecklist never receives undefined.
In `@packages/web/src/components/project/completed-tab/CompletedStudyRow.jsx`:
- Around line 189-200: The button assumes a checklist exists but
firstGroup().checklists[0] can be undefined; update the render guard and click
handler in CompletedStudyRow.jsx to ensure a checklist exists before calling
props.onOpenChecklist: change the Show condition to require firstGroup() &&
firstGroup().checklists?.length > 0 (in addition to !hasMultipleOutcomes()), and
use optional chaining when reading the id (e.g., get the firstChecklist =
firstGroup().checklists?.[0] and only call
props.onOpenChecklist(firstChecklist?.id) if present) so the button neither
throws nor calls with undefined.
In `@packages/web/src/components/project/outcomes/OutcomeManager.jsx`:
- Around line 81-88: In OutcomeManager.jsx the delete failure branch in
handleDelete calls showToast.error twice with the same message, causing
duplicate toasts; remove the redundant showToast.error call so only a single
showToast.error('Cannot delete outcome', result?.error || 'Outcome is in use by
checklists') is executed (locate the duplicate lines inside the handleDelete
function and delete the extra invocation).
In `@packages/web/src/components/project/todo-tab/TodoStudyRow.jsx`:
- Around line 69-98: The canAddMore memo currently inspects all checklists in
the study; change it to consider only checklists assigned to the current user by
filtering checklists() by assignedTo === currentUserId before computing
hasAmstar2 and usedOutcomesByType. Specifically, in the canAddMore callback
filter const userChecklists = checklists().filter(c => c.assignedTo ===
currentUserId) (or equivalent) and then compute hasAmstar2 and build
usedOutcomesByType from that filtered array while leaving outcomes() unchanged;
retain the existing checks for AMSTAR2 and ROB2/ROBINS_I but scoped to the
filtered userChecklists.
🧹 Nitpick comments (12)
packages/web/src/components/checklist/ROB2Checklist/OverallSection.jsx (1)
28-31: Consider removing redundant memo.
effectiveJudgementsimply returnscalculatedDisplayJudgement()without any transformation. This could be inlined at usage sites (lines 126, 132, 146) to reduce indirection.However, keeping it may be intentional for symmetry with the ROBINS-I version where
effectiveJudgementcontains conditional logic for manual mode.packages/shared/src/checklists/types.ts (1)
36-38: Consider importingrequiresOutcomefrom shared types instead of duplicating.This function is duplicated in
packages/web/src/primitives/useProject/checklists/index.js(lines 51-53) with identical logic. The frontendChecklistForm.jsxalso has similar inline logic. Consider having all consumers import this helper from the shared package to ensure consistency if the list of outcome-requiring types changes.♻️ Suggested approach
In
packages/web/src/primitives/useProject/checklists/index.js, import and use the shared helper:+import { requiresOutcome } from '@corates/shared'; import { createChecklistOfType, CHECKLIST_TYPES } from '@/checklist-registry';Then remove the local
requiresOutcomefunction (lines 46-53) and use the imported version.packages/web/src/primitives/useProject/checklists/index.js (2)
46-53: Duplicate helper - consider importing from shared package.As noted in the
types.tsreview, thisrequiresOutcomefunction duplicates the one in@corates/shared. Import from the shared package for single-source-of-truth.
172-191: Consider validating outcome existence before checklist creation.The ROBINS-I auto-fill silently handles missing outcome data, but if
outcomeIdreferences a non-existent outcome, the checklist is still created with an invalidoutcomeId. Consider validating outcome existence early (around line 72) and returning an error if the outcome doesn't exist.♻️ Optional early validation
// Validate outcome requirement for ROB2 and ROBINS_I if (requiresOutcome(type) && !outcomeId) { console.error(`[createChecklist] ${type} requires an outcomeId`); return null; } + // Validate outcome exists if provided + if (outcomeId) { + const metaMap = ydoc.getMap('meta'); + const outcomesMap = metaMap.get('outcomes'); + if (!outcomesMap || !outcomesMap.has(outcomeId)) { + console.error('[createChecklist] Outcome not found:', outcomeId); + return null; + } + }packages/web/src/stores/projectActionsStore/checklists.js (1)
29-43: Import and reuserequiresOutcomefrom shared types.The logic on line 31 (
type === 'ROB2' || type === 'ROBINS_I') duplicates therequiresOutcomehelper already defined inpackages/shared/src/checklists/types.ts. Reusing the shared helper ensures consistency if the list of outcome-requiring types changes.Proposed fix
+import { requiresOutcome } from '@shared/checklists/types'; // In the create function: if (!checklistId) { - // Could be duplicate or missing outcome - const requiresOutcome = type === 'ROB2' || type === 'ROBINS_I'; - if (requiresOutcome && !outcomeId) { + if (requiresOutcome(type) && !outcomeId) { showToast.error('Addition Failed', `${type} requires an outcome to be selected`); - } else if (requiresOutcome) { + } else if (requiresOutcome(type)) { showToast.error(packages/web/src/stores/projectActionsStore/outcomes.js (1)
17-20: Consider reusinggetCurrentUserIdfrom the parent store.The
getCurrentUserIdhelper duplicates the same logic found inpackages/web/src/stores/projectActionsStore/index.js(lines 107-110). Consider passinggetCurrentUserIdas a parameter tocreateOutcomeActions(similar to howcreateStudyActionsreceives it) to avoid duplication and ensure consistency.Proposed refactor
-export function createOutcomeActions(getActiveConnection) { - /** - * Get current user ID from auth store - */ - function getCurrentUserId() { - const auth = useBetterAuth(); - return auth.user()?.id || null; - } +export function createOutcomeActions(getActiveConnection, getCurrentUserId) {Then in
index.js:- const outcome = createOutcomeActions(getActiveConnection); + const outcome = createOutcomeActions(getActiveConnection, getCurrentUserId);packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx (1)
147-152: Minor: Comment is slightly misleading.The comment says "Always visible for owners" but
OutcomeManageris rendered for all users whenhasData()is true. The ownership check happens internally withinOutcomeManager(per the component'scanEdit()logic). Consider clarifying the comment.Suggested comment update
- {/* Outcome Management - Always visible for owners */} + {/* Outcome Management - Visible when data loaded; editing restricted to owners internally */} <Show when={hasData()}>packages/web/src/components/project/outcomes/OutcomeManager.jsx (1)
145-153: Avoid inline ternary rendering for the outcome count label.
Consider deriving a label via createMemo (or Show blocks) to keep conditional rendering consistent and easier to scan. As per coding guidelines: Use Solid's Show component for conditional rendering instead of ternary operators.Proposed refactor
const outcomes = createMemo(() => meta()?.outcomes || []); + const outcomeCountLabel = createMemo(() => { + const count = outcomes().length; + if (count === 0) return 'None defined'; + if (count === 1) return '1 outcome'; + return `${count} outcomes`; + }); @@ <div class='min-w-0 flex-1'> <span class='text-foreground font-medium'>Outcomes</span> <span class='text-muted-foreground ml-2 text-sm'> - {outcomes().length === 0 ? - 'None defined' - : outcomes().length === 1 ? - '1 outcome' - : `${outcomes().length} outcomes`} + {outcomeCountLabel()} </span> </div>packages/web/src/components/project/todo-tab/TodoStudyRow.jsx (1)
32-46: Consider reducing the prop surface for TodoStudyRow.
The component now accepts many props and callbacks; moving shared state and actions into a store or context would reduce prop drilling. As per coding guidelines: Components should receive at most 1-5 props for local configuration only.packages/web/src/lib/checklist-domain.js (1)
109-160: Consider extracting shared grouping logic to reduce duplication.The grouping logic in
shouldShowInTab(lines 123-134) duplicates the pattern ingetReconciliationChecklistsByOutcome(lines 311-327). This could lead to divergence if the grouping criteria change.♻️ Suggested refactor
case 'reconcile': { // Must have both reviewers assigned (single reviewer studies never go to reconcile tab) if (!study.reviewer1 || !study.reviewer2) return false; - const checklists = study.checklists || []; - - // Get reviewer-completed checklists grouped by outcome - const awaitingReconcile = checklists.filter( - c => !isReconciledChecklist(c) && c.status === CHECKLIST_STATUS.REVIEWER_COMPLETED, - ); - - if (awaitingReconcile.length === 0) return false; - - // Group by outcomeId (or type for AMSTAR2) - const groups = new Map(); - for (const checklist of awaitingReconcile) { - const groupKey = checklist.outcomeId || `type:${checklist.type}`; - if (!groups.has(groupKey)) { - groups.set(groupKey, { - checklists: [], - outcomeId: checklist.outcomeId, - type: checklist.type, - }); - } - groups.get(groupKey).checklists.push(checklist); - } + const groups = getReconciliationChecklistsByOutcome(study); + if (groups.length === 0) return false; + + const checklists = study.checklists || []; // Check if any group has a pair ready for reconciliation...packages/web/src/primitives/useProject/outcomes.js (1)
151-190: Consider reusingisOutcomeInUseindeleteOutcometo reduce duplication.The referential integrity check in
deleteOutcome(lines 157-171) duplicates the logic inisOutcomeInUse(lines 197-214).♻️ Suggested refactor
function deleteOutcome(outcomeId) { try { const ydoc = getYDoc(); if (!ydoc) return { success: false, error: 'No connection' }; - // Check if outcome is in use by any checklist - const studiesMap = ydoc.getMap('reviews'); - for (const [, studyYMap] of studiesMap.entries()) { - const checklistsMap = studyYMap.get('checklists'); - if (!checklistsMap) continue; - - for (const [, checklistYMap] of checklistsMap.entries()) { - const checklistOutcomeId = checklistYMap.get('outcomeId'); - if (checklistOutcomeId === outcomeId) { - return { - success: false, - error: 'Cannot delete outcome that is in use by checklists', - }; - } - } - } + if (isOutcomeInUse(outcomeId)) { + return { + success: false, + error: 'Cannot delete outcome that is in use by checklists', + }; + } const metaMap = ydoc.getMap('meta');packages/web/src/primitives/useProject/reconciliation.js (1)
222-226: Clarify the legacy clearing condition logic.The condition
if (!outcomeId || type === 'AMSTAR2')clears the legacy map when either:
outcomeIdis null/undefined, ORtypeis 'AMSTAR2'This means clearing a non-AMSTAR2 outcome with a specific outcomeId won't touch legacy data, which is correct. However, clearing any AMSTAR2 outcome (even with an outcomeId) will also clear legacy data, which may be unexpected if AMSTAR2 starts supporting outcomes.
Consider making this more explicit:
♻️ Suggested clarification
- // Also clear legacy format if clearing the default outcome - if (!outcomeId || type === 'AMSTAR2') { + // Also clear legacy format if this is the default AMSTAR2 case (no outcomeId) + // Legacy data only existed for AMSTAR2 without outcomes + if (!outcomeId && type === 'AMSTAR2') { studyYMap.delete('reconciliation'); }
Comment on lines
+25
to
+87
| // Get the first finalized checklist (the reconciled one) | ||
| const finalizedChecklist = () => outcomeGroup().checklists[0]; | ||
|
|
||
| // Get outcome name for display | ||
| const outcomeName = () => { | ||
| const outcomeId = outcomeGroup().outcomeId; | ||
| if (!outcomeId) return null; | ||
| return props.getOutcomeName?.(outcomeId) || 'Unknown Outcome'; | ||
| }; | ||
|
|
||
| // Get reconciliation progress for this outcome | ||
| const reconciliationProgress = () => { | ||
| return props.getReconciliationProgress?.(outcomeGroup().outcomeId, outcomeGroup().type); | ||
| }; | ||
|
|
||
| // Check if we have previous reviewers to show | ||
| const hasPreviousReviewers = () => { | ||
| const progress = reconciliationProgress(); | ||
| return !!(progress?.checklist1Id && progress?.checklist2Id); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <div class='bg-muted/50 flex items-center justify-between rounded-lg p-3'> | ||
| <div class='flex items-center gap-3'> | ||
| {/* Outcome badge */} | ||
| <Show when={outcomeName()}> | ||
| <span class='bg-secondary text-secondary-foreground rounded-full px-2 py-0.5 text-xs font-medium'> | ||
| {outcomeName()} | ||
| </span> | ||
| </Show> | ||
|
|
||
| {/* Checklist type */} | ||
| <span class='text-foreground text-sm font-medium'> | ||
| {getChecklistMetadata(outcomeGroup().type)?.name || outcomeGroup().type} | ||
| </span> | ||
|
|
||
| {/* Status badge */} | ||
| <span | ||
| class={`rounded-full px-2 py-0.5 text-xs font-medium ${getStatusStyle(finalizedChecklist()?.status)}`} | ||
| > | ||
| {getStatusLabel(finalizedChecklist()?.status)} | ||
| </span> | ||
| </div> | ||
|
|
||
| <div class='flex items-center gap-2'> | ||
| {/* View Previous button (for dual-reviewer studies) */} | ||
| <Show when={hasPreviousReviewers()}> | ||
| <button | ||
| onClick={() => setShowPreviousReviewers(true)} | ||
| class='bg-secondary text-secondary-foreground hover:bg-secondary/80 rounded-lg px-3 py-1.5 text-sm font-medium transition-colors' | ||
| > | ||
| View Previous | ||
| </button> | ||
| </Show> | ||
|
|
||
| {/* Open button */} | ||
| <button | ||
| onClick={() => props.onOpenChecklist?.(finalizedChecklist()?.id)} | ||
| class='bg-primary hover:bg-primary/90 focus:ring-primary rounded-lg px-3 py-1.5 text-sm font-medium text-white transition-colors focus:ring-2 focus:outline-none' | ||
| > | ||
| Open | ||
| </button> |
There was a problem hiding this comment.
Guard the Open action when no finalized checklist is available.
If outcomeGroup.checklists is empty during sync or data inconsistencies, onOpenChecklist will receive undefined. Consider disabling or hiding the button when there is no checklist id.
Proposed fix
- const finalizedChecklist = () => outcomeGroup().checklists[0];
+ const finalizedChecklist = () => outcomeGroup().checklists[0];
+ const finalizedChecklistId = () => finalizedChecklist()?.id;
@@
- <button
- onClick={() => props.onOpenChecklist?.(finalizedChecklist()?.id)}
+ <button
+ onClick={() => finalizedChecklistId() && props.onOpenChecklist?.(finalizedChecklistId())}
+ disabled={!finalizedChecklistId()}
class='bg-primary hover:bg-primary/90 focus:ring-primary rounded-lg px-3 py-1.5 text-sm font-medium text-white transition-colors focus:ring-2 focus:outline-none'
>
Open
</button>📝 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
| // Get the first finalized checklist (the reconciled one) | |
| const finalizedChecklist = () => outcomeGroup().checklists[0]; | |
| // Get outcome name for display | |
| const outcomeName = () => { | |
| const outcomeId = outcomeGroup().outcomeId; | |
| if (!outcomeId) return null; | |
| return props.getOutcomeName?.(outcomeId) || 'Unknown Outcome'; | |
| }; | |
| // Get reconciliation progress for this outcome | |
| const reconciliationProgress = () => { | |
| return props.getReconciliationProgress?.(outcomeGroup().outcomeId, outcomeGroup().type); | |
| }; | |
| // Check if we have previous reviewers to show | |
| const hasPreviousReviewers = () => { | |
| const progress = reconciliationProgress(); | |
| return !!(progress?.checklist1Id && progress?.checklist2Id); | |
| }; | |
| return ( | |
| <> | |
| <div class='bg-muted/50 flex items-center justify-between rounded-lg p-3'> | |
| <div class='flex items-center gap-3'> | |
| {/* Outcome badge */} | |
| <Show when={outcomeName()}> | |
| <span class='bg-secondary text-secondary-foreground rounded-full px-2 py-0.5 text-xs font-medium'> | |
| {outcomeName()} | |
| </span> | |
| </Show> | |
| {/* Checklist type */} | |
| <span class='text-foreground text-sm font-medium'> | |
| {getChecklistMetadata(outcomeGroup().type)?.name || outcomeGroup().type} | |
| </span> | |
| {/* Status badge */} | |
| <span | |
| class={`rounded-full px-2 py-0.5 text-xs font-medium ${getStatusStyle(finalizedChecklist()?.status)}`} | |
| > | |
| {getStatusLabel(finalizedChecklist()?.status)} | |
| </span> | |
| </div> | |
| <div class='flex items-center gap-2'> | |
| {/* View Previous button (for dual-reviewer studies) */} | |
| <Show when={hasPreviousReviewers()}> | |
| <button | |
| onClick={() => setShowPreviousReviewers(true)} | |
| class='bg-secondary text-secondary-foreground hover:bg-secondary/80 rounded-lg px-3 py-1.5 text-sm font-medium transition-colors' | |
| > | |
| View Previous | |
| </button> | |
| </Show> | |
| {/* Open button */} | |
| <button | |
| onClick={() => props.onOpenChecklist?.(finalizedChecklist()?.id)} | |
| class='bg-primary hover:bg-primary/90 focus:ring-primary rounded-lg px-3 py-1.5 text-sm font-medium text-white transition-colors focus:ring-2 focus:outline-none' | |
| > | |
| Open | |
| </button> | |
| // Get the first finalized checklist (the reconciled one) | |
| const finalizedChecklist = () => outcomeGroup().checklists[0]; | |
| const finalizedChecklistId = () => finalizedChecklist()?.id; | |
| // Get outcome name for display | |
| const outcomeName = () => { | |
| const outcomeId = outcomeGroup().outcomeId; | |
| if (!outcomeId) return null; | |
| return props.getOutcomeName?.(outcomeId) || 'Unknown Outcome'; | |
| }; | |
| // Get reconciliation progress for this outcome | |
| const reconciliationProgress = () => { | |
| return props.getReconciliationProgress?.(outcomeGroup().outcomeId, outcomeGroup().type); | |
| }; | |
| // Check if we have previous reviewers to show | |
| const hasPreviousReviewers = () => { | |
| const progress = reconciliationProgress(); | |
| return !!(progress?.checklist1Id && progress?.checklist2Id); | |
| }; | |
| return ( | |
| <> | |
| <div class='bg-muted/50 flex items-center justify-between rounded-lg p-3'> | |
| <div class='flex items-center gap-3'> | |
| {/* Outcome badge */} | |
| <Show when={outcomeName()}> | |
| <span class='bg-secondary text-secondary-foreground rounded-full px-2 py-0.5 text-xs font-medium'> | |
| {outcomeName()} | |
| </span> | |
| </Show> | |
| {/* Checklist type */} | |
| <span class='text-foreground text-sm font-medium'> | |
| {getChecklistMetadata(outcomeGroup().type)?.name || outcomeGroup().type} | |
| </span> | |
| {/* Status badge */} | |
| <span | |
| class={`rounded-full px-2 py-0.5 text-xs font-medium ${getStatusStyle(finalizedChecklist()?.status)}`} | |
| > | |
| {getStatusLabel(finalizedChecklist()?.status)} | |
| </span> | |
| </div> | |
| <div class='flex items-center gap-2'> | |
| {/* View Previous button (for dual-reviewer studies) */} | |
| <Show when={hasPreviousReviewers()}> | |
| <button | |
| onClick={() => setShowPreviousReviewers(true)} | |
| class='bg-secondary text-secondary-foreground hover:bg-secondary/80 rounded-lg px-3 py-1.5 text-sm font-medium transition-colors' | |
| > | |
| View Previous | |
| </button> | |
| </Show> | |
| {/* Open button */} | |
| <button | |
| onClick={() => finalizedChecklistId() && props.onOpenChecklist?.(finalizedChecklistId())} | |
| disabled={!finalizedChecklistId()} | |
| class='bg-primary hover:bg-primary/90 focus:ring-primary rounded-lg px-3 py-1.5 text-sm font-medium text-white transition-colors focus:ring-2 focus:outline-none' | |
| > | |
| Open | |
| </button> |
🤖 Prompt for AI Agents
In `@packages/web/src/components/project/completed-tab/CompletedOutcomeRow.jsx`
around lines 25 - 87, The Open button can call props.onOpenChecklist with
undefined when there is no finalized checklist; guard it by checking
finalizedChecklist()?.id before enabling or invoking the action. Update the Open
button rendering/handler in CompletedOutcomeRow.jsx to (a) compute const
checklistId = finalizedChecklist()?.id and use that for both the button disabled
state and the onClick only if truthy, or (b) hide the button when checklistId is
falsy; adjust the onClick to call props.onOpenChecklist(checklistId) only when
checklistId exists so onOpenChecklist never receives undefined.
Comment on lines
+189
to
200
| {/* Open checklist button (single outcome mode) */} | ||
| <Show when={!hasMultipleOutcomes() && firstGroup()}> | ||
| <button | ||
| onClick={e => { | ||
| e.stopPropagation(); | ||
| props.onOpenChecklist?.(completedChecklists()[0].id); | ||
| props.onOpenChecklist?.(firstGroup().checklists[0].id); | ||
| }} | ||
| class='bg-primary hover:bg-primary/90 focus:ring-primary shrink-0 rounded-lg px-4 py-1.5 text-sm font-medium text-white transition-colors focus:ring-2 focus:outline-none' | ||
| > | ||
| Open | ||
| </button> | ||
| </Show> |
There was a problem hiding this comment.
Potential issue: firstGroup().checklists[0] may be undefined if the group has no checklists.
The Show when guard ensures firstGroup() exists, but doesn't guarantee checklists[0] is defined. While getCompletedChecklistsByOutcome only creates groups for completed checklists, adding a defensive check would be safer.
🛡️ Suggested defensive check
- <Show when={!hasMultipleOutcomes() && firstGroup()}>
+ <Show when={!hasMultipleOutcomes() && firstGroup()?.checklists?.[0]}>
<button
onClick={e => {
e.stopPropagation();
props.onOpenChecklist?.(firstGroup().checklists[0].id);
}}🤖 Prompt for AI Agents
In `@packages/web/src/components/project/completed-tab/CompletedStudyRow.jsx`
around lines 189 - 200, The button assumes a checklist exists but
firstGroup().checklists[0] can be undefined; update the render guard and click
handler in CompletedStudyRow.jsx to ensure a checklist exists before calling
props.onOpenChecklist: change the Show condition to require firstGroup() &&
firstGroup().checklists?.length > 0 (in addition to !hasMultipleOutcomes()), and
use optional chaining when reading the id (e.g., get the firstChecklist =
firstGroup().checklists?.[0] and only call
props.onOpenChecklist(firstChecklist?.id) if present) so the button neither
throws nor calls with undefined.
Comment on lines
+81
to
+88
| showToast.error( | ||
| 'Cannot delete outcome', | ||
| result?.error || 'Outcome is in use by checklists', | ||
| ); | ||
| showToast.error( | ||
| 'Cannot delete outcome', | ||
| result?.error || 'Outcome is in use by checklists', | ||
| ); |
There was a problem hiding this comment.
Remove duplicate error toast on delete failure.
handleDelete currently shows the same error twice, which will duplicate the toast.
Proposed fix
} else {
showToast.error(
'Cannot delete outcome',
result?.error || 'Outcome is in use by checklists',
);
- showToast.error(
- 'Cannot delete outcome',
- result?.error || 'Outcome is in use by checklists',
- );
}📝 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
| showToast.error( | |
| 'Cannot delete outcome', | |
| result?.error || 'Outcome is in use by checklists', | |
| ); | |
| showToast.error( | |
| 'Cannot delete outcome', | |
| result?.error || 'Outcome is in use by checklists', | |
| ); | |
| } else { | |
| showToast.error( | |
| 'Cannot delete outcome', | |
| result?.error || 'Outcome is in use by checklists', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/components/project/outcomes/OutcomeManager.jsx` around lines
81 - 88, In OutcomeManager.jsx the delete failure branch in handleDelete calls
showToast.error twice with the same message, causing duplicate toasts; remove
the redundant showToast.error call so only a single showToast.error('Cannot
delete outcome', result?.error || 'Outcome is in use by checklists') is executed
(locate the duplicate lines inside the handleDelete function and delete the
extra invocation).
Comment on lines
+69
to
+98
| // Check if user can add more checklists (has unused outcomes for ROB2/ROBINS_I, or no AMSTAR2 yet) | ||
| const canAddMore = createMemo(() => { | ||
| const userChecklists = checklists(); | ||
| const projectOutcomes = outcomes(); | ||
|
|
||
| // For AMSTAR2: can add if user doesn't have one yet | ||
| const hasAmstar2 = userChecklists.some(c => c.type === CHECKLIST_TYPES.AMSTAR2); | ||
| if (!hasAmstar2) return true; | ||
|
|
||
| // For ROB2/ROBINS_I: can add if there are unused outcomes | ||
| if (projectOutcomes.length === 0) return false; | ||
|
|
||
| const usedOutcomesByType = {}; | ||
| for (const checklist of userChecklists) { | ||
| if (checklist.outcomeId) { | ||
| if (!usedOutcomesByType[checklist.type]) { | ||
| usedOutcomesByType[checklist.type] = new Set(); | ||
| } | ||
| usedOutcomesByType[checklist.type].add(checklist.outcomeId); | ||
| } | ||
| } | ||
|
|
||
| // Check if any outcome is available for ROB2 or ROBINS_I | ||
| for (const outcome of projectOutcomes) { | ||
| const rob2Used = usedOutcomesByType[CHECKLIST_TYPES.ROB2]?.has(outcome.id); | ||
| const robinsUsed = usedOutcomesByType[CHECKLIST_TYPES.ROBINS_I]?.has(outcome.id); | ||
| if (!rob2Used || !robinsUsed) return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Scope canAddMore to the current user to avoid blocking other reviewers.
The current logic considers all checklists in the study, so another reviewer can consume all outcomes or add AMSTAR2 and prevent the current user from adding their own checklist. Filter by assignedTo/currentUserId when computing hasAmstar2 and usedOutcomesByType.
Proposed fix
const canAddMore = createMemo(() => {
- const userChecklists = checklists();
+ const userId = props.currentUserId;
+ if (!userId) return false;
+ const userChecklists = checklists().filter(c => c.assignedTo === userId);
const projectOutcomes = outcomes();
// For AMSTAR2: can add if user doesn't have one yet
const hasAmstar2 = userChecklists.some(c => c.type === CHECKLIST_TYPES.AMSTAR2);
if (!hasAmstar2) return true;
@@
- for (const checklist of userChecklists) {
+ for (const checklist of userChecklists) {
if (checklist.outcomeId) {
if (!usedOutcomesByType[checklist.type]) {
usedOutcomesByType[checklist.type] = new Set();
}
usedOutcomesByType[checklist.type].add(checklist.outcomeId);
}
}🤖 Prompt for AI Agents
In `@packages/web/src/components/project/todo-tab/TodoStudyRow.jsx` around lines
69 - 98, The canAddMore memo currently inspects all checklists in the study;
change it to consider only checklists assigned to the current user by filtering
checklists() by assignedTo === currentUserId before computing hasAmstar2 and
usedOutcomesByType. Specifically, in the canAddMore callback filter const
userChecklists = checklists().filter(c => c.assignedTo === currentUserId) (or
equivalent) and then compute hasAmstar2 and build usedOutcomesByType from that
filtered array while leaving outcomes() unchanged; retain the existing checks
for AMSTAR2 and ROB2/ROBINS_I but scoped to the filtered userChecklists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores