308 update design system#311
Conversation
… 308-update-design-system
… oauth provider. change name handling.
…yBowman/corates into 308-update-design-system
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ⛔ Deployment terminated View logs |
corates | 0b89624 | Commit Preview URL | Jan 19 2026, 07:19 AM |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR splits user names into Changes
Sequence Diagram(s)mermaid style Auth fill: rgba(63,81,181,0.5) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/workers/src/routes/admin/database.ts (1)
109-118: Schema mismatch:userDisplayNameshould beuserGivenName.The
UserAnalyticsSchemastill definesuserDisplayName(line 114), but thepdfsByUserRouteresponse at line 859 returnsuserGivenName. This breaks the OpenAPI contract and will cause validation failures.Proposed fix
const UserAnalyticsSchema = z .object({ userId: z.string(), userName: z.string().nullable(), userEmail: z.string().nullable(), - userDisplayName: z.string().nullable(), + userGivenName: z.string().nullable(), pdfCount: z.number(), totalStorage: z.number(), }) .openapi('UserAnalytics');packages/web/src/components/settings/pages/TwoFactorSetup.jsx (1)
197-201: Inconsistent color token: spinner still uses concreteborder-blue-600.The loading spinner uses a hardcoded color while the rest of the component has been migrated to semantic tokens. Update to use
border-primaryfor consistency with the design system update.Suggested fix
<Show when={loading() && !setupMode() && !disableMode()}> <div class='flex items-center justify-center py-4'> - <div class='h-6 w-6 animate-spin rounded-full border-2 border-blue-600 border-t-transparent' /> + <div class='h-6 w-6 animate-spin rounded-full border-2 border-primary border-t-transparent' /> </div> </Show>packages/workers/src/routes/database.ts (1)
26-33: Add familyName to the DbUser response for parity with the name split.Right now the response only exposes givenName, which makes it hard to reconstruct a full name and is inconsistent with the split used elsewhere.
Proposed fix
.object({ id: z.string(), username: z.string().nullable(), email: z.string(), givenName: z.string().nullable(), + familyName: z.string().nullable(), emailVerified: z.boolean(), createdAt: z.string(), })const results = await db .select({ id: user.id, username: user.username, email: user.email, givenName: user.givenName, + familyName: user.familyName, emailVerified: user.emailVerified, createdAt: user.createdAt, })Also applies to: 89-95
packages/workers/src/routes/orgs/pdfs.ts (1)
45-52: Expose familyName in PdfUploader for consistency with the split name model.Without familyName, consumers cannot render full names even when available.
Proposed fix
const PdfUploaderSchema = z .object({ id: z.string(), name: z.string().nullable(), email: z.string().nullable(), givenName: z.string().nullable(), + familyName: z.string().nullable(), })const results = await db .select({ id: mediaFiles.id, filename: mediaFiles.filename, originalName: mediaFiles.originalName, fileType: mediaFiles.fileType, fileSize: mediaFiles.fileSize, bucketKey: mediaFiles.bucketKey, createdAt: mediaFiles.createdAt, uploadedBy: mediaFiles.uploadedBy, uploadedByName: user.name, uploadedByEmail: user.email, uploadedByGivenName: user.givenName, + uploadedByFamilyName: user.familyName, })uploadedBy: row.uploadedBy ? { id: row.uploadedBy, name: row.uploadedByName, email: row.uploadedByEmail, givenName: row.uploadedByGivenName, + familyName: row.uploadedByFamilyName, } : null,Also applies to: 464-498
🤖 Fix all issues with AI agents
In `@packages/web/src/components/settings/pages/NotificationsSettings.jsx`:
- Line 69: The gradient CSS on the div in NotificationsSettings.jsx uses
hardcoded tokens 'from-slate-50 to-white'; replace them with the same semantic
token classes used earlier in this file (the gradient on line 27) so the div's
class string uses the semantic "from-..." and "to-..." token names instead of
literal color names; update the class value in the div element to match the
semantic tokens used elsewhere in the NotificationsSettings component.
- Line 27: The gradient uses hardcoded color tokens in the header div class
string ("border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white
px-6 py-4"), which breaks dark mode; update that element to use semantic tokens
instead (e.g., replace the "from-slate-50 to-white" parts with semantic token
names from your design system or swap to a single semantic background token) or,
if you must keep a gradient, add corresponding semantic gradient tokens to the
theme and reference them here; ensure the container still keeps
"border-border-subtle" and padding classes while removing concrete color tokens
so dark mode works correctly.
In `@packages/workers/src/lib/avatar-copy.ts`:
- Around line 42-49: The isExternalAvatarUrl function currently accepts URLs
regardless of scheme; update it to require HTTPS by checking parsed.protocol ===
'https:' (or reject if not) before performing the ALLOWED_AVATAR_DOMAINS
hostname checks so only https:// hosts on the allowlist are accepted; keep using
parsed.hostname and the existing domain matching (exact or endsWith) but
short-circuit to false for non-https schemes and invalid URLs.
In `@packages/workers/src/routes/admin/projects.ts`:
- Around line 383-386: The response schemas still use the old "*DisplayName"
property names while the handler payloads return "creatorGivenName",
"userGivenName", "uploaderGivenName", and "inviterGivenName" (e.g., the object
with creatorName, creatorGivenName, creatorEmail, createdAt), causing contract
mismatch; update the Zod/OpenAPI response schemas to use the new "*GivenName"
property names (or add a mapping layer that populates the old "*DisplayName"
aliases from the new fields) so the published contract matches what handlers
actually return (apply same change for the other occurrences referencing
userGivenName, uploaderGivenName, inviterGivenName).
🧹 Nitpick comments (18)
packages/workers/src/routes/billing/handlers/invoiceHandlers.ts (1)
196-208: Prefer full name composition for dunning emails.Using only
givenNamedropsfamilyNamein the email greeting. Consider selectingfamilyNameand composing a full name with fallback toname.Proposed change
- const billingUser = await db - .select({ id: user.id, email: user.email, name: user.name, givenName: user.givenName }) + const billingUser = await db + .select({ + id: user.id, + email: user.email, + name: user.name, + givenName: user.givenName, + familyName: user.familyName, + }) .from(user) .where(eq(user.stripeCustomerId, stripeCustomerId)) .get(); @@ - userName: billingUser.givenName || billingUser.name || null, + userName: + [billingUser.givenName, billingUser.familyName].filter(Boolean).join(' ') || + billingUser.name || + null,packages/web/src/components/settings/pages/TwoFactorSetup.jsx (1)
491-497: Hover effect may not be visible on copy button.The "Copy all codes" button uses
hover:bg-muted, but it's nested inside a container withbg-muted(line 481). The hover state won't produce a visible effect since it matches the parent background.Suggested fix
<button onClick={copyBackupCodes} - class='text-secondary-foreground hover:bg-muted mt-3 flex w-full items-center justify-center space-x-2 rounded-md px-3 py-2 text-sm transition' + class='text-secondary-foreground hover:bg-secondary mt-3 flex w-full items-center justify-center space-x-2 rounded-md px-3 py-2 text-sm transition' >packages/web/src/components/ui/select.tsx (1)
134-139: Consider removing redundant opacity.The chevron icon applies both
text-muted-foreground(already a lighter semantic color) andopacity-50, which may result in an overly faint icon. Iftext-muted-foregroundis defined appropriately in your design tokens, the additionalopacity-50might be unnecessary.Suggested change
- fallback={<BiRegularChevronDown class='text-muted-foreground h-4 w-4 opacity-50' />} + fallback={<BiRegularChevronDown class='text-muted-foreground h-4 w-4' />}packages/workers/src/commands/members/addMember.ts (1)
142-151: Confirm downstream schema accepts undefined for given/family names.If the DO schema expects
string | null(nullable but not optional), sendingundefinedcan fail validation. Consider normalizing tonullto keep a stable payload shape.Proposed fix
- givenName: userToAdd.givenName, - familyName: userToAdd.familyName, + givenName: userToAdd.givenName ?? null, + familyName: userToAdd.familyName ?? null,packages/workers/src/routes/admin/stripe-tools.ts (2)
48-55: Consider exposing familyName alongside givenNameIf consumers now rely on the split-name model, this response shape is incomplete without familyName. Adding it keeps the admin API consistent with the broader rename.
Proposed change
const LinkedUserSchema = z .object({ id: z.string(), email: z.string(), name: z.string(), givenName: z.string().nullable(), + familyName: z.string().nullable(), stripeCustomerId: z.string().optional(), }) .nullable() .openapi('LinkedUser');interface LinkedUser { id: string; email: string; name: string; givenName: string | null; + familyName: string | null; stripeCustomerId?: string | null; }Also applies to: 382-388
482-488: Include familyName in linked-user projectionsTo actually return familyName in the response, it needs to be selected from the database in both lookup paths.
Proposed change
const [userByStripe] = await db .select({ id: user.id, email: user.email, name: user.name, givenName: user.givenName, + familyName: user.familyName, })const [userByEmail] = await db .select({ id: user.id, email: user.email, name: user.name, givenName: user.givenName, + familyName: user.familyName, stripeCustomerId: user.stripeCustomerId, })Also applies to: 498-504
packages/web/src/components/settings/pages/SessionManagement.jsx (1)
148-150: Replace ternary JSX conditionals with Show.Use
<Show>instead of ternary rendering for SolidJS reactivity consistency. This applies to the "Active now" status and the two button label conditionals.Proposed refactor (example for status text)
- <span> - {props.isCurrent ? - 'Active now' - : formatRelativeTime(props.session.updatedAt || props.session.createdAt)} - </span> + <Show + when={props.isCurrent} + fallback={formatRelativeTime(props.session.updatedAt || props.session.createdAt)} + > + Active now + </Show>As per coding guidelines and learnings, prefer Show for conditional rendering.
Also applies to: 341-341, 382-382
packages/web/src/components/settings/pages/LinkedAccountsSection.jsx (1)
335-359: Use Show for DEV-only rendering to match SolidJS guidelines.This keeps conditional rendering consistent with the project rules.
As per coding guidelines.Proposed change
- {import.meta.env.DEV && ( - <div class='border-warning/30 bg-warning-subtle mt-4 rounded-lg border p-4'> - <p class='text-warning mb-2 text-xs font-semibold'>Dev Mode: Test Merge Flow</p> - <div class='flex gap-2'> - <button - onClick={() => { - setMergeConflictProvider('orcid'); - setShowMergeDialog(true); - }} - class='bg-warning hover:bg-warning/90 rounded px-3 py-1.5 text-xs font-medium text-white transition-colors' - > - Test ORCID Merge - </button> - <button - onClick={() => { - setMergeConflictProvider('google'); - setShowMergeDialog(true); - }} - class='bg-warning hover:bg-warning/90 rounded px-3 py-1.5 text-xs font-medium text-white transition-colors' - > - Test Google Merge - </button> - </div> - </div> - )} + <Show when={import.meta.env.DEV}> + <div class='border-warning/30 bg-warning-subtle mt-4 rounded-lg border p-4'> + <p class='text-warning mb-2 text-xs font-semibold'>Dev Mode: Test Merge Flow</p> + <div class='flex gap-2'> + <button + onClick={() => { + setMergeConflictProvider('orcid'); + setShowMergeDialog(true); + }} + class='bg-warning hover:bg-warning/90 rounded px-3 py-1.5 text-xs font-medium text-white transition-colors' + > + Test ORCID Merge + </button> + <button + onClick={() => { + setMergeConflictProvider('google'); + setShowMergeDialog(true); + }} + class='bg-warning hover:bg-warning/90 rounded px-3 py-1.5 text-xs font-medium text-white transition-colors' + > + Test Google Merge + </button> + </div> + </div> + </Show>packages/web/src/components/auth/CompleteProfile.jsx (3)
118-134: Trim OAuth-provided names before autofillWhitespace-only givenName or familyName can leave the fields empty while still setting hasAutofilledName, which blocks fallback. Consider trimming and only marking autofill when a trimmed value is present.
Proposed fix
- if (!firstName().trim() && !lastName().trim()) { - if (currentUser?.givenName || currentUser?.familyName) { - setFirstName(currentUser.givenName || ''); - setLastName(currentUser.familyName || ''); - setHasAutofilledName(true); - } else if (currentUser?.name) { + if (!firstName().trim() && !lastName().trim()) { + const oauthGivenName = (currentUser?.givenName ?? '').trim(); + const oauthFamilyName = (currentUser?.familyName ?? '').trim(); + if (oauthGivenName || oauthFamilyName) { + setFirstName(oauthGivenName); + setLastName(oauthFamilyName); + setHasAutofilledName(true); + } else if (currentUser?.name) {
157-159: Align form errors with createFormErrorSignalsThis form still uses a single string error signal. Consider migrating step validation to createFormErrorSignals so errors are field-aware and consistent with other forms. As per coding guidelines.
361-361: Use a semantic text token for the optional indicatorThis change introduces text-gray-400. If the design system now uses semantic tokens, switch this to the muted text token used elsewhere for consistency. Based on PR objectives.
packages/web/src/components/settings/pages/MergeAccountsDialog.jsx (1)
291-292: Prefer semantic foreground/background tokens for primary and destructive buttons.
Hard-codedtext-whiteandbg-red-600bypass the semantic palette and can drift in dark/high-contrast themes. Consider usingtext-primary-foregroundfor primary buttons andbg-destructive/text-destructive-foregroundfor the destructive action (if those tokens are available in the design system).Proposed change
- class='bg-primary hover:bg-primary/90 inline-flex items-center gap-2 rounded-lg px-4 py-2 text-sm font-medium text-white transition-colors' + class='bg-primary hover:bg-primary/90 inline-flex items-center gap-2 rounded-lg px-4 py-2 text-sm font-medium text-primary-foreground transition-colors' - class='bg-primary hover:bg-primary/90 inline-flex items-center gap-2 rounded-lg px-4 py-2 text-sm font-medium text-white transition-colors disabled:opacity-50' + class='bg-primary hover:bg-primary/90 inline-flex items-center gap-2 rounded-lg px-4 py-2 text-sm font-medium text-primary-foreground transition-colors disabled:opacity-50' - class='bg-primary hover:bg-primary/90 inline-flex items-center gap-2 rounded-lg px-4 py-2 text-sm font-medium text-white transition-colors disabled:opacity-50' + class='bg-primary hover:bg-primary/90 inline-flex items-center gap-2 rounded-lg px-4 py-2 text-sm font-medium text-primary-foreground transition-colors disabled:opacity-50' - class='focus:ring-ring inline-flex items-center gap-2 rounded-lg bg-red-600 px-4 py-2 text-sm font-medium text-white transition-colors hover:bg-red-700 focus:ring-2 focus:outline-none disabled:opacity-50' + class='focus:ring-ring inline-flex items-center gap-2 rounded-lg bg-destructive px-4 py-2 text-sm font-medium text-destructive-foreground transition-colors hover:bg-destructive/90 focus:ring-2 focus:outline-none disabled:opacity-50' - class='bg-primary hover:bg-primary/90 rounded-lg px-4 py-2 text-sm font-medium text-white transition-colors' + class='bg-primary hover:bg-primary/90 rounded-lg px-4 py-2 text-sm font-medium text-primary-foreground transition-colors'Also applies to: 337-338, 403-404, 469-470, 503-504
packages/web/src/components/settings/pages/ProfileInfoSection.jsx (1)
43-55: Normalize fallback name parsing for multi word given namesSplitting on a single space can introduce empty tokens and can mis-derive lastName when givenName has spaces. Consider trimming and splitting on whitespace, and when givenName exists, strip it from name before deriving the remainder.
Proposed update
const firstName = () => { const u = user(); - if (u?.givenName) return u.givenName; - const name = u?.name || ''; - return name.split(' ')[0] || ''; + if (u?.givenName) return u.givenName.trim(); + const name = (u?.name || '').trim(); + return name.split(/\s+/)[0] || ''; }; const lastName = () => { const u = user(); - if (u?.familyName) return u.familyName; - const name = u?.name || ''; - return name.split(' ').slice(1).join(' ') || ''; + if (u?.familyName) return u.familyName.trim(); + const name = (u?.name || '').trim(); + if (u?.givenName) { + const given = u.givenName.trim(); + if (given && name.startsWith(given)) { + return name.slice(given.length).trim(); + } + } + return name.split(/\s+/).slice(1).join(' ') || ''; };packages/web/src/primitives/avatarCache.js (1)
98-106: Consider handling non-relative URLs more explicitly.The current logic uses
imageUrldirectly if it doesn't start with/. While the docstring indicates only relative paths are expected, this could silently pass through unexpected URLs. Consider adding validation or a warning for non-relative URLs to catch potential misuse.Optional: Add explicit validation
export async function fetchAndCacheAvatar(userId, imageUrl) { if (!userId || !imageUrl) return null; try { + // All avatars should now be served from our API + if (!imageUrl.startsWith('/')) { + console.warn('Unexpected non-relative avatar URL:', imageUrl); + } // Build the full URL from relative path const fullUrl = imageUrl.startsWith('/') ? `${API_BASE}${imageUrl}` : imageUrl;packages/workers/src/durable-objects/dev-handlers.ts (1)
346-347: Consider extracting givenName/familyName from importer if available.The importer's
givenNameandfamilyNameare set tonulleven though the importer object could potentially carry this data. Consider updating theImportRequestinterface to include these fields and use them when available.Optional: Support givenName/familyName in importer
interface ImportRequest { json(): Promise<{ data?: ImportData; mode?: 'replace' | 'merge'; targetOrgId?: string; importer?: { userId?: string; name?: string | null; email?: string | null; + givenName?: string | null; + familyName?: string | null; image?: string | null; }; }>; }Then update the handler:
importerYMap.set('name', importer.name || null); importerYMap.set('email', importer.email || null); - importerYMap.set('givenName', null); - importerYMap.set('familyName', null); + importerYMap.set('givenName', importer.givenName || null); + importerYMap.set('familyName', importer.familyName || null); importerYMap.set('image', importer.image || null);packages/workers/src/routes/orgs/invitations.ts (1)
541-626: Consider composing inviterName with both givenName and familyName when available.This preserves a fuller display name in invitation emails while keeping existing fallbacks.
Proposed fix
- const inviter = await db - .select({ name: user.name, givenName: user.givenName, email: user.email }) + const inviter = await db + .select({ + name: user.name, + givenName: user.givenName, + familyName: user.familyName, + email: user.email, + }) .from(user) .where(eq(user.id, authUser.id)) .get();- const inviterName = inviter?.givenName || inviter?.name || inviter?.email || 'Someone'; + const fullGivenFamily = + inviter?.givenName && inviter?.familyName ? + `${inviter.givenName} ${inviter.familyName}` : + inviter?.givenName; + const inviterName = fullGivenFamily || inviter?.name || inviter?.email || 'Someone';packages/workers/src/routes/members.ts (1)
474-476: Consider using full inviter name when available.Right now the inviter display name uses only givenName; if familyName is present it gets dropped. You can compose a full name first and then fall back to name or email.
Proposed refactor
- const inviter = await db - .select({ name: user.name, givenName: user.givenName, email: user.email }) + const inviter = await db + .select({ + name: user.name, + givenName: user.givenName, + familyName: user.familyName, + email: user.email, + }) .from(user) .where(eq(user.id, authUser.id)) .get(); @@ - const inviterName = inviter?.givenName || inviter?.name || inviter?.email || 'Someone'; + const inviterName = + (inviter?.givenName && inviter?.familyName + ? `${inviter.givenName} ${inviter.familyName}` + : inviter?.givenName) || + inviter?.name || + inviter?.email || + 'Someone';Also applies to: 558-560
packages/workers/src/auth/config.ts (1)
427-437: Add email fallback for display names in outbound emails.If givenName, name, and username are all missing, the templates may receive undefined. Using email as a final fallback keeps the greeting stable.
Proposed refactor
- user.givenName || user.name || user.username, + user.givenName || user.name || user.username || user.email, @@ - user.givenName || user.name || user.username, + user.givenName || user.name || user.username || user.email,Also applies to: 460-470
| <div class='mb-6 overflow-hidden rounded-xl border border-slate-200/60 bg-white shadow-sm transition-shadow duration-200 hover:shadow-md'> | ||
| <div class='border-b border-slate-100 bg-gradient-to-r from-slate-50 to-white px-6 py-4'> | ||
| <div class='border-border bg-card mb-6 overflow-hidden rounded-xl border shadow-sm transition-shadow duration-200 hover:shadow-md'> | ||
| <div class='border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6 py-4'> |
There was a problem hiding this comment.
Inconsistent color tokens in gradient.
The gradient still uses concrete color tokens (from-slate-50 to-white) while the rest of the component has been migrated to semantic tokens. This will break dark mode support and is inconsistent with the design system update.
Consider using semantic tokens for the gradient background, or replacing with a non-gradient background that uses semantic tokens.
Suggested fix
- <div class='border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6 py-4'>
+ <div class='border-border-subtle border-b bg-card-subtle px-6 py-4'>Or if you need to preserve the gradient effect, define semantic gradient tokens in your theme.
📝 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.
| <div class='border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6 py-4'> | |
| <div class='border-border-subtle border-b bg-card-subtle px-6 py-4'> |
🤖 Prompt for AI Agents
In `@packages/web/src/components/settings/pages/NotificationsSettings.jsx` at line
27, The gradient uses hardcoded color tokens in the header div class string
("border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6
py-4"), which breaks dark mode; update that element to use semantic tokens
instead (e.g., replace the "from-slate-50 to-white" parts with semantic token
names from your design system or swap to a single semantic background token) or,
if you must keep a gradient, add corresponding semantic gradient tokens to the
theme and reference them here; ensure the container still keeps
"border-border-subtle" and padding classes while removing concrete color tokens
so dark mode works correctly.
| <div class='mb-6 overflow-hidden rounded-xl border border-slate-200/60 bg-white shadow-sm transition-shadow duration-200 hover:shadow-md'> | ||
| <div class='border-b border-slate-100 bg-gradient-to-r from-slate-50 to-white px-6 py-4'> | ||
| <div class='border-border bg-card mb-6 overflow-hidden rounded-xl border shadow-sm transition-shadow duration-200 hover:shadow-md'> | ||
| <div class='border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6 py-4'> |
There was a problem hiding this comment.
Same gradient token inconsistency.
This line has the same issue as line 27 - uses from-slate-50 to-white instead of semantic tokens.
Suggested fix
- <div class='border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6 py-4'>
+ <div class='border-border-subtle border-b bg-card-subtle px-6 py-4'>📝 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.
| <div class='border-border-subtle border-b bg-gradient-to-r from-slate-50 to-white px-6 py-4'> | |
| <div class='border-border-subtle border-b bg-card-subtle px-6 py-4'> |
🤖 Prompt for AI Agents
In `@packages/web/src/components/settings/pages/NotificationsSettings.jsx` at line
69, The gradient CSS on the div in NotificationsSettings.jsx uses hardcoded
tokens 'from-slate-50 to-white'; replace them with the same semantic token
classes used earlier in this file (the gradient on line 27) so the div's class
string uses the semantic "from-..." and "to-..." token names instead of literal
color names; update the class value in the div element to match the semantic
tokens used elsewhere in the NotificationsSettings component.
There was a problem hiding this comment.
Actionable comments posted: 2
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/project/reconcile-tab/ReconcileStudyRow.jsx (1)
82-88: Guard against non-Element targets before calling closest.Line 85 can throw if the click target is a Text node. Add an Element check before calling closest to prevent runtime errors when clicking on text.
Proposed fix
const handleRowClick = e => { if (!hasPdfs()) return; const target = e.target; + if (!(target instanceof Element)) return; const interactive = target.closest('button, [role="button"], [data-selectable]'); if (interactive) return; setExpanded(!expanded()); };
🤖 Fix all issues with AI agents
In `@packages/web/src/components/project/add-studies/DoiLookupSection.jsx`:
- Line 230: The class string on the anchor elements in DoiLookupSection.jsx uses
a redundant hover text color (e.g., "text-primary hover:text-primary inline-flex
h-6 w-6 items-center justify-center rounded transition-colors
hover:bg-blue-50"); update both occurrences (the one containing that class and
the similar one around line 284) to either remove "hover:text-primary" or
replace it with a subtle variant like "hover:text-primary/80" so the hover state
provides visible feedback while keeping the rest of the classes intact.
In `@packages/workers/src/lib/avatar-copy.ts`:
- Around line 20-36: Replace the local AVATAR_COPY_ERRORS and ad-hoc error
strings with the shared domain error utilities: use createDomainError from
`@corates/shared` and the shared error categories (e.g., VALIDATION_ERRORS for
input issues and SYSTEM_ERRORS or FILE_ERRORS for fetch/upload failures) inside
the avatar copy flow (see AVATAR_COPY_ERRORS and AvatarCopyResult); return or
attach the created domain error (instead of raw strings) on
AvatarCopyResult.error/errorCode, and map specific failure cases (invalid
MIME/type → VALIDATION_ERRORS, fetch/upload failures → SYSTEM_ERRORS or
FILE_ERRORS) so callers receive standardized domain errors; update all related
logic in the avatar-copy functions (including the error-returning branches
around lines 76-181) to construct and propagate createDomainError(...) with the
appropriate shared constant.
🧹 Nitpick comments (14)
packages/web/src/components/project/ContactPrompt.jsx (1)
37-50: Design token applied correctly; consider semantic tokens for remaining colors.The
bg-cardtoken aligns with the design system migration. The hardcoded blue colors (border-blue-200,text-blue-800,text-blue-600,bg-blue-600, etc.) may be intentional for this informational/CTA component, but could be candidates for semantic tokens (e.g.,text-info,bg-primary,border-info) if consistency with the new design system is desired.packages/web/src/components/auth/SocialAuthButtons.jsx (1)
71-72: Consider extracting shared button styles to reduce duplication.The
baseClassdefinition here is identical toGoogleButton(line 12). Extracting this to a shared constant would reduce maintenance burden and ensure consistency if styles need to change.Suggested refactor
+const socialButtonBaseClass = + 'border border-border hover:bg-muted text-secondary-foreground font-semibold rounded-lg sm:rounded-xl transition disabled:opacity-50 disabled:cursor-not-allowed flex items-center justify-center'; + export function GoogleButton(props) { - const baseClass = - 'border border-border hover:bg-muted text-secondary-foreground font-semibold rounded-lg sm:rounded-xl transition disabled:opacity-50 disabled:cursor-not-allowed flex items-center justify-center'; + const baseClass = socialButtonBaseClass; // ... } export function OrcidButton(props) { - const baseClass = - 'border border-border hover:bg-muted text-secondary-foreground font-semibold rounded-lg sm:rounded-xl transition disabled:opacity-50 disabled:cursor-not-allowed flex items-center justify-center'; + const baseClass = socialButtonBaseClass; // ... }packages/web/src/components/sidebar/Sidebar.jsx (3)
273-275: Inconsistent design token migration — hardcoded blue colors remain.The PR migrates most colors to semantic tokens, but active/selected states and action links still use hardcoded Tailwind blue values (
bg-blue-100 text-blue-700,text-blue-600 hover:text-blue-700). Consider migrating these to semantic tokens likebg-primary/10 text-primaryor introducing dedicatedactive/selectedvariants for consistency.Example: Migrate active state colors
class={`flex w-full items-center gap-2 rounded-lg px-3 py-2 text-sm font-medium transition-colors ${ isCurrentPath(getProjectsPath()) || isCurrentPath('/dashboard') ? - 'bg-blue-100 text-blue-700' + 'bg-primary/10 text-primary' : 'text-secondary-foreground hover:bg-secondary' }`}<button onClick={() => navigate(getProjectsPath())} - class='mt-1 text-xs text-blue-600 hover:text-blue-700' + class='mt-1 text-xs text-primary hover:text-primary/80' >Also applies to: 297-299, 343-344, 386-387
493-493: Resize handle also uses hardcoded blue.The resize handle hover color
hover:bg-blue-400should be migrated to a semantic token (e.g.,hover:bg-primary/60or a dedicated resize/accent token) for consistency with the design system.
515-515: Mobile overlay styling matches desktop — consider extracting shared content.The mobile portal correctly mirrors the desktop design token updates. However, there's substantial code duplication between the desktop sidebar content (lines 266-408) and mobile content (lines 540-677). Consider extracting the shared content into a
SidebarContentsub-component to reduce duplication and ensure future styling changes stay synchronized.Also applies to: 526-532, 540-677
packages/web/src/components/dashboard/LocalAppraisalCard.jsx (1)
102-102: Consider using destructive design tokens for consistency.The delete button hover uses hardcoded color values (
hover:bg-red-50 hover:text-red-500) while the rest of the component uses design system tokens. Based on the AI summary,global.cssintroduces tokens likecolor-destructive-subtle. Usinghover:bg-destructive-subtle hover:text-destructivewould maintain consistency with the design system migration.Suggested change for design token consistency
<button type='button' onClick={e => { e.stopPropagation(); props.onDelete?.(props.checklist?.id); }} - class='text-muted-foreground/50 rounded-lg p-2 opacity-0 transition-all group-hover:opacity-100 hover:bg-red-50 hover:text-red-500' + class='text-muted-foreground/50 rounded-lg p-2 opacity-0 transition-all group-hover:opacity-100 hover:bg-destructive-subtle hover:text-destructive' title='Delete appraisal' >packages/web/src/components/auth/CheckEmail.jsx (1)
168-172: Consider: Hardcoded accent colors may benefit from design tokens.The success message uses hardcoded colors (
border-green-200 bg-green-50 text-green-600) while the rest of the component has migrated to semantic tokens. Similarly, the blue accent colors for the spinner and icon (lines 134, 147-148, 157) remain hardcoded.If the design system includes semantic tokens for success/primary accent states (e.g.,
bg-success,text-primary), consider updating these for consistency in a follow-up.packages/web/src/components/project/add-studies/AddStudiesForm.jsx (2)
180-183: Consider migrating hardcoded blue colors to design tokens.The drag overlay uses
bg-card(design token) but retains hardcoded colors (bg-blue-500/10,border-blue-500,text-blue-600). For consistency with the design system migration, consider using semantic tokens:-<div class='pointer-events-none fixed inset-0 z-50 flex items-center justify-center bg-blue-500/10'> - <div class='bg-card rounded-xl border-2 border-dashed border-blue-500 p-8 shadow-lg'> - <p class='text-lg font-medium text-blue-600'>Drop PDFs to add studies</p> +<div class='pointer-events-none fixed inset-0 z-50 flex items-center justify-center bg-primary/10'> + <div class='bg-card rounded-xl border-2 border-dashed border-primary p-8 shadow-lg'> + <p class='text-lg font-medium text-primary'>Drop PDFs to add studies</p>
334-345: Consider migrating hover colors to design tokens.Similar to the drag overlay, the initial drop zone uses design tokens for base styling but hardcoded colors for hover states:
-class='border-border cursor-pointer rounded-lg border-2 border-dashed p-8 text-center transition-colors hover:border-blue-500 hover:bg-blue-50/50' +class='border-border cursor-pointer rounded-lg border-2 border-dashed p-8 text-center transition-colors hover:border-primary hover:bg-primary/5'This would maintain consistency with the design system migration.
packages/web/src/components/dashboard/LocalAppraisalsSection.jsx (2)
39-50: Prefer text-primary-foreground for primary button labelsUsing text-white hard-codes contrast and can break theming if primary is light. A semantic foreground token keeps contrast consistent.
Proposed change
- class='bg-primary hover:bg-primary/90 flex items-center gap-1.5 rounded-lg px-4 py-2 text-sm font-medium text-white transition-colors' + class='bg-primary hover:bg-primary/90 flex items-center gap-1.5 rounded-lg px-4 py-2 text-sm font-medium text-primary-foreground transition-colors'
67-77: Use text-primary-foreground for the Sign In buttonThis avoids hard-coded white text and keeps contrast consistent across themes.
Proposed change
- class='bg-primary hover:bg-primary/90 rounded-lg px-4 py-2 text-sm font-medium text-white transition-colors' + class='bg-primary hover:bg-primary/90 rounded-lg px-4 py-2 text-sm font-medium text-primary-foreground transition-colors'packages/web/src/components/auth/TwoFactorVerify.jsx (1)
68-80: Replace ternary-rendered text with Solid ShowProject guidelines prefer Solid’s Show for conditional rendering. Please swap the ternary-rendered text in the header and label to Show to stay consistent with the codebase conventions.
Proposed fix
-import { createSignal } from 'solid-js'; +import { createSignal, Show } from 'solid-js';- <p class='text-muted-foreground text-sm'> - {useBackupCode() ? - 'Enter one of your backup codes' - : 'Enter the code from your authenticator app'} - </p> + <p class='text-muted-foreground text-sm'> + <Show when={useBackupCode()} fallback='Enter the code from your authenticator app'> + Enter one of your backup codes + </Show> + </p> ... - <label class='text-secondary-foreground mb-2 block text-sm font-medium' for='2fa-code'> - {useBackupCode() ? 'Backup Code' : 'Verification Code'} - </label> + <label class='text-secondary-foreground mb-2 block text-sm font-medium' for='2fa-code'> + <Show when={useBackupCode()} fallback='Verification Code'> + Backup Code + </Show> + </label>packages/web/src/components/dashboard/ProjectCard.jsx (1)
133-141: Design tokens applied correctly, minor shadow color inconsistency.The container styling correctly uses semantic tokens (
border-border/60,bg-card,hover:border-border). However,hover:shadow-stone-200/50still uses a hardcoded color while other elements migrated to tokens. This is a minor inconsistency that could be addressed for full design system alignment.packages/workers/src/lib/avatar-copy.ts (1)
112-133: Guard against oversized responses when Content-Length is missing.If Content-Length is absent, arrayBuffer() reads the entire body before size checks, which can spike memory. Consider streaming the response with a hard MAX_AVATAR_SIZE limit before buffering.
Proposed adjustment
- const arrayBuffer = await response.arrayBuffer(); + const arrayBuffer = await readArrayBufferWithLimit(response, MAX_AVATAR_SIZE);async function readArrayBufferWithLimit(response: Response, maxBytes: number) { const reader = response.body?.getReader(); if (!reader) throw new Error('Missing response body'); const chunks: Uint8Array[] = []; let received = 0; while (true) { const { done, value } = await reader.read(); if (done) break; if (value) { received += value.byteLength; if (received > maxBytes) { throw new Error(`Avatar too large: ${received} bytes (max: ${maxBytes})`); } chunks.push(value); } } const buffer = new Uint8Array(received); let offset = 0; for (const chunk of chunks) { buffer.set(chunk, offset); offset += chunk.byteLength; } return buffer.buffer; }
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/web/src/components/admin/ProjectDetail.jsx (1)
210-216: Use orgSlug in admin org links to match URL convention.The links currently point to
/admin/orgs/${orgId}but the frontend URL convention requires orgSlug. This component already hasprojectData().project.orgSlug, so use that in both locations to keep routes consistent.Proposed fix
- href={`/admin/orgs/${projectData().project.orgId}`} + href={`/admin/orgs/${projectData().project.orgSlug}`}- href={`/admin/orgs/${projectData().project.orgId}`} + href={`/admin/orgs/${projectData().project.orgSlug}`}As per coding guidelines, prefer human-readable org slugs in frontend URLs.
Also applies to: 264-270
packages/web/src/components/ui/tooltip.tsx (1)
95-104: Hardcoded arrow background color may cause theme inconsistency.Line 99 uses hardcoded
#111827for--arrow-backgroundwhile the classes now use theme tokens (bg-foreground). If the foreground color differs from this hex value in other themes, the arrow background won't match the tooltip content.Consider using a CSS variable reference or removing
--arrow-backgroundif Ark UI derives it from the element's background class.Suggested fix
style={{ '--arrow-size': '8px', '--arrow-background': '#111827' }} + // TODO(agent): Replace hardcoded `#111827` with a CSS variable reference + // that matches the foreground token, or verify if Ark UI derives this from bg-foreground classIf your Tailwind config exposes the foreground color as a CSS variable (e.g.,
--color-foreground), you could use:style={{ '--arrow-size': '8px', '--arrow-background': 'var(--color-foreground)' }}packages/web/src/components/admin/DatabaseViewer.jsx (1)
246-255: Duplicate hover class detected.Line 248 has
bg-secondary hover:bg-secondarywhich applies the same background on hover as the base state, effectively making the hover state invisible.Suggested fix
<button onClick={() => rowsQuery.refetch()} - class='bg-secondary hover:bg-secondary flex items-center gap-1 rounded px-3 py-1 text-sm disabled:opacity-50' + class='bg-secondary hover:bg-muted flex items-center gap-1 rounded px-3 py-1 text-sm disabled:opacity-50' disabled={rowsQuery.isFetching} >
🤖 Fix all issues with AI agents
In `@packages/web/src/components/admin/SubscriptionDialog.jsx`:
- Around line 206-211: The Cancel button in SubscriptionDialog.jsx uses the
redundant class "hover:bg-secondary" that duplicates the base "bg-secondary" so
the hover state doesn't change; update the button's class list on the <button>
(the Cancel button with onClick={() => props.onOpenChange?.(false)}) by removing
the duplicate "hover:bg-secondary" and replace it with a visible hover variant
(for example "hover:bg-secondary-foreground" or "hover:bg-secondary/90"
depending on your design tokens) so the hover state is distinguishable from the
base state.
In `@packages/web/src/components/admin/UserDetail.jsx`:
- Around line 270-277: The UI uses userData().user.name directly in the
UserAvatar name prop and in the <h1> heading which can render blank for legacy
users; update both usages (UserAvatar's name prop and the h1 text) to provide a
safe fallback such as userData().user.name || userData().user.email || 'Unknown
User' so the avatar initials and header always display something meaningful.
In `@packages/web/src/components/admin/UserTable.jsx`:
- Around line 55-62: The avatar fallback and displayed name are inconsistent:
UserAvatar uses user.name directly while the link shows {user.name ||
'Unknown'}, which can leave avatar initials blank when name is missing; update
the UserAvatar usage (component call) to use the same fallback expression (e.g.,
use the computed displayName variable or user.name || 'Unknown') so the avatar
initials match the displayed name and reference the UserAvatar component and the
A link rendering that uses {user.name || 'Unknown'} to locate the change.
In `@packages/web/src/components/project/add-studies/PdfUploadSection.jsx`:
- Around line 15-17: The pendingPdfs helper assumes studies().uploadedPdfs
exists and will throw if studies() is undefined or uploadedPdfs is missing;
update pendingPdfs to guard by using a safe fallback (e.g., optional chaining or
defaulting to an empty array) when reading studies().uploadedPdfs so the filter
only runs on an array; reference the pendingPdfs function and the studies() call
to locate and update the expression to something that safely handles a missing
or loading uploadedPdfs.
In `@packages/web/src/components/project/add-studies/StagedStudiesSection.jsx`:
- Around line 11-69: The remove button in StagedStudiesSection (the button that
calls studies().removeStagedStudy(study) and renders BiRegularTrash) is
icon-only and needs an explicit accessible name; add an aria-label attribute to
that button (e.g., "Remove study" or preferably include the study title like
"Remove study {study.title}") so screen readers can announce its purpose; ensure
the aria-label is stable and unique per study.
In `@packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx`:
- Around line 249-314: MemberPercentRow is defined inside ReviewerAssignment
which recreates it on each render causing showCustom signal resets, lost focus
and extra re-renders; extract MemberPercentRow to a top-level component (or
separate file) so it is not redefined per render, keep its internal
createSignal(showCustom) inside the extracted component, accept rowProps
(member, percent, onChange) as props, and ensure it either imports/defines the
presets array (or receives presets as a prop) so it no longer closes over
ReviewerAssignment scope; update any usage in ReviewerAssignment to render
<MemberPercentRow ... /> (or the equivalent JSX) and export/import the extracted
component if moved to another file.
In
`@packages/web/src/components/project/reconcile-tab/rob2-reconcile/NavbarDomainPill.jsx`:
- Around line 55-60: In NavbarDomainPill.jsx the computed class string stored in
base uses identical classes for expanded hover state (currently "bg-border
hover:bg-border"), which removes hover feedback; update the branch that checks
props.isExpanded to use a darker hover variant (for example replace the
hover:bg-border with hover:bg-muted or hover:bg-gray-300) so the expanded state
still shows a visual change on hover; ensure you modify the code that builds the
base variable inside the component (the props.isExpanded branch) and keep the
non-expanded branch as-is.
In
`@packages/web/src/components/project/reconcile-tab/rob2-reconcile/ROB2SummaryView.jsx`:
- Around line 141-143: The buttons in ROB2SummaryView.jsx used for navigation
(the element with onClick calling props.onGoToPage?.(getItemIndex(item)) and the
other buttons around lines referenced 223-244) are missing explicit types and
may default to type="submit" inside forms; update those button elements to
include type="button" so they don't trigger form submission—locate the JSX
<button> elements (e.g., the one using getItemIndex(item) and other non-submit
buttons) and add type="button" to each.
🧹 Nitpick comments (22)
packages/web/src/components/ui/toast.tsx (1)
137-150: Default case correctly migrated to design tokens.The default styling now uses
border-border bg-cardwhich aligns with the design system.Note: The state-specific styles (success/error/warning/loading) and
ToastActionTrigger(line 129) still use hardcoded colors. If you want full design token consistency, consider whether these should migrate to semantic tokens like--color-success,--color-destructive, etc. However, keeping explicit semantic colors for feedback states is also a valid approach.packages/web/src/components/checklist/ROBINSIChecklist/SectionC.jsx (1)
75-77: Redundant hover class has no visual effect.The
hover:border-borderis identical to the defaultborder-border, so it has no effect. Consider either:
- Removing the redundant hover class
- Adding a distinct hover state for better affordance (e.g.,
hover:border-primaryorhover:bg-accent)Option 1: Remove redundant hover class
- class={`flex items-center gap-3 rounded-lg border-2 p-3 transition-all duration-200 ${props.sectionCState?.isPerProtocol === option.value ? 'border-blue-500 bg-blue-50' : 'border-border bg-card hover:border-border'} ${props.disabled ? 'cursor-not-allowed opacity-60' : 'cursor-pointer'} `} + class={`flex items-center gap-3 rounded-lg border-2 p-3 transition-all duration-200 ${props.sectionCState?.isPerProtocol === option.value ? 'border-blue-500 bg-blue-50' : 'border-border bg-card'} ${props.disabled ? 'cursor-not-allowed opacity-60' : 'cursor-pointer'} `}Option 2: Add meaningful hover state
- class={`flex items-center gap-3 rounded-lg border-2 p-3 transition-all duration-200 ${props.sectionCState?.isPerProtocol === option.value ? 'border-blue-500 bg-blue-50' : 'border-border bg-card hover:border-border'} ${props.disabled ? 'cursor-not-allowed opacity-60' : 'cursor-pointer'} `} + class={`flex items-center gap-3 rounded-lg border-2 p-3 transition-all duration-200 ${props.sectionCState?.isPerProtocol === option.value ? 'border-blue-500 bg-blue-50' : 'border-border bg-card hover:bg-accent'} ${props.disabled ? 'cursor-not-allowed opacity-60' : 'cursor-pointer'} `}packages/web/src/components/checklist/ROBINSIChecklist/SectionD.jsx (1)
44-54: Incomplete design token migration and no-op hover state.Two observations:
Hardcoded colors: The checked state still uses hardcoded
border-blue-500 bg-blue-50(line 45) andtext-blue-600(line 52), while other styles were migrated to design tokens. Consider using semantic tokens likeborder-primary bg-primary/10for consistency.No-op hover:
hover:border-border(line 45) provides no visual feedback since it's identical to the baseborder-border. Considerhover:border-muted-foregroundor similar for an actual hover effect.Suggested diff for consistency
<label - class={`flex items-start gap-3 rounded-lg border p-3 transition-all duration-200 ${isSourceChecked(source) ? 'border-blue-500 bg-blue-50' : 'border-border bg-card hover:border-border'} ${props.disabled ? 'cursor-not-allowed opacity-60' : 'cursor-pointer'} `} + class={`flex items-start gap-3 rounded-lg border p-3 transition-all duration-200 ${isSourceChecked(source) ? 'border-primary bg-primary/10' : 'border-border bg-card hover:border-muted-foreground'} ${props.disabled ? 'cursor-not-allowed opacity-60' : 'cursor-pointer'} `} > <input type='checkbox' checked={isSourceChecked(source)} disabled={props.disabled} onChange={() => !props.disabled && handleSourceToggle(source)} - class='border-border focus:ring-primary mt-0.5 h-4 w-4 rounded text-blue-600' + class='border-border focus:ring-primary mt-0.5 h-4 w-4 rounded text-primary' />packages/web/src/components/checklist/LocalAppraisalsPanel.jsx (1)
121-121: Redundanthover:border-borderclass.The
hover:border-borderis redundant sinceborder-borderis already applied as the base class. Consider removing it or specifying a different hover border color for visual feedback.Suggested fix
- <div class='group border-border bg-card hover:border-border relative rounded-lg border p-6 shadow-sm transition-all duration-200 hover:shadow-md'> + <div class='group border-border bg-card relative rounded-lg border p-6 shadow-sm transition-all duration-200 hover:shadow-md'>packages/web/src/components/project/reconcile-tab/amstar2-reconcile/NotesCompareSection.jsx (1)
169-192: Consider using design tokens for accent colors.The final note section header correctly uses
text-secondary-foreground, but several accent colors remain hardcoded:
- Line 169:
bg-green-50/50- Line 175:
text-green-600 hover:text-green-800- Line 191:
focusRingColor='green-500'Similarly, the copy buttons (lines 123, 150) use hardcoded
text-blue-600 hover:text-blue-800.If your design system includes semantic tokens for success/primary actions (e.g.,
bg-success-muted,text-primary), consider using them for consistency. Otherwise, this is fine to leave as-is if hardcoded accent colors are intentional.packages/web/src/components/admin/billing-observability/AdminBillingLedgerPage.jsx (3)
113-120: Prefer<Show>for conditional rendering.Line 115 uses
{entry.processedAt && (...)}which should use the<Show>component per SolidJS conventions. Based on learnings, this maintains consistency with the rest of the codebase.Suggested fix
<div class='text-muted-foreground whitespace-nowrap'> <div>{formatDate(entry.receivedAt)}</div> - {entry.processedAt && ( + <Show when={entry.processedAt}> <div class='text-muted-foreground/70 text-xs'> Processed: {formatDate(entry.processedAt)} </div> - )} + </Show> </div>
211-281: Use<Show>for Stripe ID conditionals.Lines 212, 238, and 264 use the
{entry.stripeXxxId && (...)}pattern. For consistency with SolidJS best practices and the<Show>usage elsewhere in this file, convert these to<Show>components.Suggested fix
<div class='space-y-1'> - {entry.stripeCustomerId && ( + <Show when={entry.stripeCustomerId}> <div class='flex items-center space-x-1'> ... </div> - )} - {entry.stripeSubscriptionId && ( + </Show> + <Show when={entry.stripeSubscriptionId}> <div class='flex items-center space-x-1'> ... </div> - )} - {entry.stripeCheckoutSessionId && ( + </Show> + <Show when={entry.stripeCheckoutSessionId}> <div class='flex items-center space-x-1'> ... </div> - )} + </Show> </div>
91-101: Consider extractinghandleCopyto a shared utility.This function is identical to the one in
StripeToolsPage.jsx. Consider extracting it to a shared admin utility to reduce duplication.packages/web/src/components/admin/ui/AdminDataTable.jsx (1)
203-218: Consider extracting duplicated button styling.The Previous and Next buttons share identical class strings. Extracting to a constant would improve maintainability and reduce duplication.
♻️ Optional: Extract button class to constant
+const paginationButtonClass = + 'border-border bg-card text-secondary-foreground hover:bg-muted rounded-lg border px-3 py-1.5 text-sm font-medium transition-colors focus:ring-2 focus:ring-blue-500 focus:outline-none disabled:cursor-not-allowed disabled:opacity-50'; + // In the JSX: <button type='button' - class='border-border bg-card text-secondary-foreground hover:bg-muted rounded-lg border px-3 py-1.5 text-sm font-medium transition-colors focus:ring-2 focus:ring-blue-500 focus:outline-none disabled:cursor-not-allowed disabled:opacity-50' + class={paginationButtonClass} disabled={!table.getCanPreviousPage()} onClick={() => table.previousPage()} > Previous </button> <button type='button' - class='border-border bg-card text-secondary-foreground hover:bg-muted rounded-lg border px-3 py-1.5 text-sm font-medium transition-colors focus:ring-2 focus:ring-blue-500 focus:outline-none disabled:cursor-not-allowed disabled:opacity-50' + class={paginationButtonClass} disabled={!table.getCanNextPage()} onClick={() => table.nextPage()} > Next </button>packages/web/src/components/checklist/ROB2Checklist/OverallSection.jsx (1)
156-160: Minor: No-op hover class.The unselected button state has
hover:border-borderbut the base already specifiesborder-border, making the hover effect a no-op. This doesn't cause issues but could be cleaned up for clarity.Suggested cleanup
isSelected() ? 'border-blue-400 bg-blue-100 text-blue-800' - : 'border-border bg-muted text-muted-foreground hover:border-border' + : 'border-border bg-muted text-muted-foreground hover:bg-muted/80'Consider either removing the no-op hover or adding a meaningful hover effect (like
hover:bg-muted/80for subtle feedback).packages/web/src/components/admin/OrgBillingSummary.jsx (1)
62-71: Replace the access mode ternary with Show.SolidJS guidance prefers Show for conditional rendering.
As per coding guidelines, use Show for conditional rendering.Proposed change
<p class='text-foreground mt-1 text-lg font-medium'> - {accessMode() === 'full' ? - <span class='inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800'> - Full Access - </span> - : <span class='inline-flex items-center rounded-full bg-yellow-100 px-2.5 py-0.5 text-xs font-medium text-yellow-800'> - Read Only - </span> - } + <Show + when={accessMode() === 'full'} + fallback={ + <span class='inline-flex items-center rounded-full bg-yellow-100 px-2.5 py-0.5 text-xs font-medium text-yellow-800'> + Read Only + </span> + } + > + <span class='inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800'> + Full Access + </span> + </Show> </p>packages/web/src/components/checklist/AMSTAR2Checklist/AMSTAR2Checklist.jsx (2)
769-800: Consider using a design token for input accent color.Lines 779 and 794 retain
text-blue-600for the radio/checkbox checked state while other colors use design tokens. For full theming consistency, consider usingtext-primaryor a similar token.Suggested change
- class='border-border focus:ring-primary h-3.5 w-3.5 cursor-pointer text-blue-600' + class='border-border focus:ring-primary h-3.5 w-3.5 cursor-pointer text-primary'- class='border-border focus:ring-primary h-3 w-3 shrink-0 text-blue-600' + class='border-border focus:ring-primary h-3 w-3 shrink-0 text-primary'
845-851: Outer container background not migrated to design token.Line 846 retains
bg-blue-50while child components were migrated tobg-card. If this is intentional for AMSTAR2-specific styling, consider documenting why. Otherwise, consider using a design token likebg-mutedorbg-backgroundfor theming consistency.packages/web/src/components/project/reconcile-tab/amstar2-reconcile/Footer.jsx (1)
28-31: Consider using primary design tokens for the enabled save state.
The enabled classes still hardcode blue, which may bypass theming. Using primary tokens keeps the button consistent with the new system.Proposed example (if primary token utilities exist)
- 'focus:ring-primary bg-blue-600 text-white shadow hover:bg-blue-700 focus:ring-2' + 'focus:ring-primary bg-primary text-primary-foreground shadow hover:bg-primary/90 focus:ring-2'packages/web/src/components/admin/GrantList.jsx (1)
64-82: Prefer Show for conditional rendering in SolidJS. Replace the ternary badge and the inline && block with Show to match repo conventions.Proposed refactor
- {grant.revokedAt ? - <span class='inline-flex items-center rounded-full bg-red-100 px-2.5 py-0.5 text-xs font-medium text-red-800'> - Revoked - </span> - : <span class='inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800'> - Active - </span> - } + <Show + when={grant.revokedAt} + fallback={ + <span class='inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800'> + Active + </span> + } + > + <span class='inline-flex items-center rounded-full bg-red-100 px-2.5 py-0.5 text-xs font-medium text-red-800'> + Revoked + </span> + </Show>- {grant.revokedAt && <p>Revoked: {formatDate(grant.revokedAt)}</p>} + <Show when={grant.revokedAt}> + <p>Revoked: {formatDate(grant.revokedAt)}</p> + </Show>Based on learnings, use Show for conditional rendering in SolidJS.
packages/web/src/components/admin/billing-observability/OrgBillingReconcilePanel.jsx (1)
75-307: Prefer Show for conditional rendering in SolidJS. Replace ternary and && conditions (refresh button, checkStripe note, Stripe comparison) with Show to align with repo conventions.Proposed refactor example
- {reconcileQuery.isFetching ? - <FiLoader class='h-4 w-4 animate-spin' /> - : 'Refresh'} + <Show + when={reconcileQuery.isFetching} + fallback='Refresh' + > + <FiLoader class='h-4 w-4 animate-spin' /> + </Show>Based on learnings, use Show for conditional rendering in SolidJS.
packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx (2)
79-89: Consider specifying radix forparseInt.Using
parseIntwithout an explicit radix can lead to unexpected behavior with certain input strings (e.g., strings with leading zeros in older engines).Suggested fix
const updatePool1Percent = (userId, value) => { - const val = Math.max(0, Math.min(100, parseInt(value) || 0)); + const val = Math.max(0, Math.min(100, parseInt(value, 10) || 0)); setPool1Percents(prev => ({ ...prev, [userId]: val })); setShowPreview(false); }; const updatePool2Percent = (userId, value) => { - const val = Math.max(0, Math.min(100, parseInt(value) || 0)); + const val = Math.max(0, Math.min(100, parseInt(value, 10) || 0)); setPool2Percents(prev => ({ ...prev, [userId]: val })); setShowPreview(false); };
308-308:autofocusattribute won't work for dynamically shown elements.The
autofocusHTML attribute only triggers focus on initial page load. For dynamically revealed inputs, use arefwith programmatic focus.Suggested approach
<input ref={el => showCustom() && el?.focus()} type='number' // ... rest of props />Or use SolidJS's
onMountpattern if the component is extracted.packages/web/src/components/admin/OrgQuickActions.jsx (1)
42-57: Consider updating shared button tokens for consistency.The secondary button styling here uses the new design tokens (
border-border,bg-card,text-secondary-foreground,hover:bg-muted), butpackages/web/src/components/admin/styles/admin-tokens.js(lines 250-265) still definesbutton.secondarywith gray-based colors (border-gray-200,bg-white,text-gray-700).If this is an incremental migration, this is fine. Otherwise, consider updating
admin-tokens.jsto maintain a single source of truth for button styling.packages/web/src/primitives/useAddStudies/index.js (1)
39-52: Consider extracting shared filtering logic to reduce duplication.The filtering logic here duplicates
getStudiesToSubmit(lines 130-141). Both filter refs byselectedRefIdsand lookups byselectedLookupIdswithpdfAvailable. Consider extracting a helper:♻️ Suggested refactor
// Helper to get filtered sources const getSelectedSources = () => { const selectedRefs = refOps.importedRefs().filter(r => refOps.selectedRefIds().has(r._id)); const selectedLookups = lookupOps .lookupRefs() .filter(r => lookupOps.selectedLookupIds().has(r._id) && r.pdfAvailable); return { selectedRefs, selectedLookups }; }; // Then use in both places const stagedStudiesPreview = createMemo(() => { const { selectedRefs, selectedLookups } = getSelectedSources(); return buildDeduplicatedStudies({ uploadedPdfs: pdfOps.uploadedPdfs, selectedRefs, selectedLookups, driveFiles: driveOps.selectedDriveFiles(), }); }); function getStudiesToSubmit() { const { selectedRefs, selectedLookups } = getSelectedSources(); return buildDeduplicatedStudies({ uploadedPdfs: pdfOps.uploadedPdfs, selectedRefs, selectedLookups, driveFiles: driveOps.selectedDriveFiles(), }); }packages/workers/src/lib/avatar-copy.ts (1)
151-174: Verify the bucket choice for avatar storage.Line 153 uses
env.PDF_BUCKETfor avatars. Please confirm this is intended (lifecycle rules, access controls, and naming all align), or consider a dedicated bucket or clearer naming to avoid accidental policy conflicts.packages/web/src/components/project/add-studies/AddStudiesForm.jsx (1)
222-257: Consider extracting the tabs header to reduce duplicationThe tabs list and count logic are duplicated in multiple branches. A small TabsHeader component would keep styling and count logic in one place. As per coding guidelines, keep files small and modular.
| <button | ||
| onClick={() => props.onOpenChange?.(false)} | ||
| class='rounded-lg bg-gray-100 px-4 py-2 text-sm font-medium text-gray-700 hover:bg-gray-200' | ||
| class='bg-secondary text-secondary-foreground hover:bg-secondary rounded-lg px-4 py-2 text-sm font-medium' | ||
| > | ||
| Cancel | ||
| </button> |
There was a problem hiding this comment.
Duplicate hover class on Cancel button.
Similar to DatabaseViewer, line 208 has bg-secondary hover:bg-secondary which makes the hover state indistinguishable from the base state.
Suggested fix
<button
onClick={() => props.onOpenChange?.(false)}
- class='bg-secondary text-secondary-foreground hover:bg-secondary rounded-lg px-4 py-2 text-sm font-medium'
+ class='bg-secondary text-secondary-foreground hover:bg-muted rounded-lg px-4 py-2 text-sm font-medium'
>
Cancel
</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.
| <button | |
| onClick={() => props.onOpenChange?.(false)} | |
| class='rounded-lg bg-gray-100 px-4 py-2 text-sm font-medium text-gray-700 hover:bg-gray-200' | |
| class='bg-secondary text-secondary-foreground hover:bg-secondary rounded-lg px-4 py-2 text-sm font-medium' | |
| > | |
| Cancel | |
| </button> | |
| <button | |
| onClick={() => props.onOpenChange?.(false)} | |
| class='bg-secondary text-secondary-foreground hover:bg-muted rounded-lg px-4 py-2 text-sm font-medium' | |
| > | |
| Cancel | |
| </button> |
🤖 Prompt for AI Agents
In `@packages/web/src/components/admin/SubscriptionDialog.jsx` around lines 206 -
211, The Cancel button in SubscriptionDialog.jsx uses the redundant class
"hover:bg-secondary" that duplicates the base "bg-secondary" so the hover state
doesn't change; update the button's class list on the <button> (the Cancel
button with onClick={() => props.onOpenChange?.(false)}) by removing the
duplicate "hover:bg-secondary" and replace it with a visible hover variant (for
example "hover:bg-secondary-foreground" or "hover:bg-secondary/90" depending on
your design tokens) so the hover state is distinguishable from the base state.
| <UserAvatar | ||
| src={userData().user.avatarUrl || userData().user.image} | ||
| name={userData().user.displayName || userData().user.name} | ||
| name={userData().user.name} | ||
| class='h-16 w-16 text-xl' | ||
| /> | ||
| <div> | ||
| <h1 class='text-2xl font-bold text-gray-900'> | ||
| {userData().user.displayName || userData().user.name} | ||
| </h1> | ||
| <p class='text-gray-500'>{userData().user.email}</p> | ||
| <h1 class='text-foreground text-2xl font-bold'>{userData().user.name}</h1> | ||
| <p class='text-muted-foreground'>{userData().user.email}</p> |
There was a problem hiding this comment.
Add a safe fallback for missing user name.
Line 272 and Line 276 use user.name without a fallback. If legacy data is incomplete, the header and avatar initials can be blank. Consider falling back to email or a placeholder.
Proposed fix
- <UserAvatar
- src={userData().user.avatarUrl || userData().user.image}
- name={userData().user.name}
- class='h-16 w-16 text-xl'
- />
+ <UserAvatar
+ src={userData().user.avatarUrl || userData().user.image}
+ name={userData().user.name || userData().user.email || 'Unknown'}
+ class='h-16 w-16 text-xl'
+ />
...
- <h1 class='text-foreground text-2xl font-bold'>{userData().user.name}</h1>
+ <h1 class='text-foreground text-2xl font-bold'>
+ {userData().user.name || userData().user.email || 'Unknown'}
+ </h1>📝 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.
| <UserAvatar | |
| src={userData().user.avatarUrl || userData().user.image} | |
| name={userData().user.displayName || userData().user.name} | |
| name={userData().user.name} | |
| class='h-16 w-16 text-xl' | |
| /> | |
| <div> | |
| <h1 class='text-2xl font-bold text-gray-900'> | |
| {userData().user.displayName || userData().user.name} | |
| </h1> | |
| <p class='text-gray-500'>{userData().user.email}</p> | |
| <h1 class='text-foreground text-2xl font-bold'>{userData().user.name}</h1> | |
| <p class='text-muted-foreground'>{userData().user.email}</p> | |
| <UserAvatar | |
| src={userData().user.avatarUrl || userData().user.image} | |
| name={userData().user.name || userData().user.email || 'Unknown'} | |
| class='h-16 w-16 text-xl' | |
| /> | |
| <div> | |
| <h1 class='text-foreground text-2xl font-bold'> | |
| {userData().user.name || userData().user.email || 'Unknown'} | |
| </h1> | |
| <p class='text-muted-foreground'>{userData().user.email}</p> |
🤖 Prompt for AI Agents
In `@packages/web/src/components/admin/UserDetail.jsx` around lines 270 - 277, The
UI uses userData().user.name directly in the UserAvatar name prop and in the
<h1> heading which can render blank for legacy users; update both usages
(UserAvatar's name prop and the h1 text) to provide a safe fallback such as
userData().user.name || userData().user.email || 'Unknown User' so the avatar
initials and header always display something meaningful.
| <UserAvatar src={user.avatarUrl || user.image} name={user.name} class='h-8 w-8' /> | ||
| <div> | ||
| <A | ||
| href={`/admin/users/${user.id}`} | ||
| class='font-medium text-blue-600 hover:text-blue-700 hover:underline' | ||
| onClick={e => e.stopPropagation()} | ||
| > | ||
| {user.displayName || user.name || 'Unknown'} | ||
| {user.name || 'Unknown'} |
There was a problem hiding this comment.
Align UserAvatar fallback with the displayed name.
Line 55 uses user.name directly while Line 62 falls back to Unknown. If name is empty, the avatar initials may be blank. Consider using the same fallback.
Proposed fix
- <UserAvatar src={user.avatarUrl || user.image} name={user.name} class='h-8 w-8' />
+ <UserAvatar
+ src={user.avatarUrl || user.image}
+ name={user.name || 'Unknown'}
+ class='h-8 w-8'
+ />📝 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.
| <UserAvatar src={user.avatarUrl || user.image} name={user.name} class='h-8 w-8' /> | |
| <div> | |
| <A | |
| href={`/admin/users/${user.id}`} | |
| class='font-medium text-blue-600 hover:text-blue-700 hover:underline' | |
| onClick={e => e.stopPropagation()} | |
| > | |
| {user.displayName || user.name || 'Unknown'} | |
| {user.name || 'Unknown'} | |
| <UserAvatar | |
| src={user.avatarUrl || user.image} | |
| name={user.name || 'Unknown'} | |
| class='h-8 w-8' | |
| /> | |
| <div> | |
| <A | |
| href={`/admin/users/${user.id}`} | |
| class='font-medium text-blue-600 hover:text-blue-700 hover:underline' | |
| onClick={e => e.stopPropagation()} | |
| > | |
| {user.name || 'Unknown'} |
🤖 Prompt for AI Agents
In `@packages/web/src/components/admin/UserTable.jsx` around lines 55 - 62, The
avatar fallback and displayed name are inconsistent: UserAvatar uses user.name
directly while the link shows {user.name || 'Unknown'}, which can leave avatar
initials blank when name is missing; update the UserAvatar usage (component
call) to use the same fallback expression (e.g., use the computed displayName
variable or user.name || 'Unknown') so the avatar initials match the displayed
name and reference the UserAvatar component and the A link rendering that uses
{user.name || 'Unknown'} to locate the change.
| // Show PDFs that are still extracting or have errors (need user attention) | ||
| const pendingPdfs = () => studies().uploadedPdfs.filter(pdf => pdf.extracting || pdf.error); | ||
|
|
There was a problem hiding this comment.
Guard against missing uploadedPdfs.
Line 16 assumes studies().uploadedPdfs is always defined. If studies() is still loading or does not yet contain the array, this will throw.
Proposed fix
- const pendingPdfs = () => studies().uploadedPdfs.filter(pdf => pdf.extracting || pdf.error);
+ const pendingPdfs = () =>
+ (studies()?.uploadedPdfs || []).filter(pdf => pdf.extracting || pdf.error);📝 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.
| // Show PDFs that are still extracting or have errors (need user attention) | |
| const pendingPdfs = () => studies().uploadedPdfs.filter(pdf => pdf.extracting || pdf.error); | |
| // Show PDFs that are still extracting or have errors (need user attention) | |
| const pendingPdfs = () => | |
| (studies()?.uploadedPdfs || []).filter(pdf => pdf.extracting || pdf.error); |
🤖 Prompt for AI Agents
In `@packages/web/src/components/project/add-studies/PdfUploadSection.jsx` around
lines 15 - 17, The pendingPdfs helper assumes studies().uploadedPdfs exists and
will throw if studies() is undefined or uploadedPdfs is missing; update
pendingPdfs to guard by using a safe fallback (e.g., optional chaining or
defaulting to an empty array) when reading studies().uploadedPdfs so the filter
only runs on an array; reference the pendingPdfs function and the studies() call
to locate and update the expression to something that safely handles a missing
or loading uploadedPdfs.
| export default function StagedStudiesSection(props) { | ||
| const studies = () => props.studies; | ||
| const stagedStudies = () => studies().stagedStudiesPreview(); | ||
|
|
||
| return ( | ||
| <Show when={stagedStudies().length > 0}> | ||
| <div class='border-border mt-4 border-t pt-4'> | ||
| <div class='mb-3 flex items-center justify-between'> | ||
| <h4 class='text-secondary-foreground text-sm font-medium'> | ||
| Staged Studies ({stagedStudies().length}) | ||
| </h4> | ||
| </div> | ||
|
|
||
| <div class='space-y-2'> | ||
| <For each={stagedStudies()}> | ||
| {study => ( | ||
| <div class='border-border bg-muted flex items-center gap-3 rounded-lg border p-3'> | ||
| <div class='text-muted-foreground shrink-0'> | ||
| <Show | ||
| when={study.pdfData || study.googleDriveFileId} | ||
| fallback={<FiFile class='h-5 w-5' />} | ||
| > | ||
| <CgFileDocument class='h-5 w-5' /> | ||
| </Show> | ||
| </div> | ||
|
|
||
| <div class='min-w-0 flex-1'> | ||
| <p class='text-foreground truncate text-sm font-medium'>{study.title}</p> | ||
| <div class='text-muted-foreground flex items-center gap-2 text-xs'> | ||
| <Show when={study.firstAuthor || study.publicationYear}> | ||
| <span> | ||
| <Show when={study.firstAuthor}> | ||
| {study.firstAuthor} | ||
| <Show when={study.publicationYear}>, </Show> | ||
| </Show> | ||
| <Show when={study.publicationYear}>{study.publicationYear}</Show> | ||
| </span> | ||
| </Show> | ||
| <Show when={study.pdfData}> | ||
| <span class='bg-primary/10 text-primary rounded px-1.5 py-0.5 text-xs'> | ||
| </span> | ||
| </Show> | ||
| <Show when={study.googleDriveFileId && !study.pdfData}> | ||
| <span class='bg-primary/10 text-primary rounded px-1.5 py-0.5 text-xs'> | ||
| Drive | ||
| </span> | ||
| </Show> | ||
| </div> | ||
| </div> | ||
|
|
||
| <button | ||
| type='button' | ||
| onClick={() => studies().removeStagedStudy(study)} | ||
| class='text-muted-foreground/70 focus:ring-primary shrink-0 rounded p-1.5 transition-colors hover:bg-red-50 hover:text-red-600 focus:ring-2 focus:outline-none' | ||
| title='Remove study' | ||
| > | ||
| <BiRegularTrash class='h-4 w-4' /> | ||
| </button> |
There was a problem hiding this comment.
Add an aria-label to the icon-only remove button
Title alone is not sufficient for screen readers. Add an explicit label to keep the button accessible.
Proposed change
<button
type='button'
+ aria-label='Remove staged study'
onClick={() => studies().removeStagedStudy(study)}
class='text-muted-foreground/70 focus:ring-primary shrink-0 rounded p-1.5 transition-colors hover:bg-red-50 hover:text-red-600 focus:ring-2 focus:outline-none'
title='Remove study'
>🤖 Prompt for AI Agents
In `@packages/web/src/components/project/add-studies/StagedStudiesSection.jsx`
around lines 11 - 69, The remove button in StagedStudiesSection (the button that
calls studies().removeStagedStudy(study) and renders BiRegularTrash) is
icon-only and needs an explicit accessible name; add an aria-label attribute to
that button (e.g., "Remove study" or preferably include the study title like
"Remove study {study.title}") so screen readers can announce its purpose; ensure
the aria-label is stable and unique per study.
| if (props.isExpanded) { | ||
| // When expanded, label is slightly darker to stand out | ||
| base += 'bg-gray-200 hover:bg-gray-300 '; | ||
| base += 'bg-border hover:bg-border '; | ||
| } else { | ||
| base += 'hover:bg-gray-200 '; | ||
| base += 'hover:bg-border '; | ||
| } |
There was a problem hiding this comment.
No hover feedback when expanded due to identical bg-border classes.
Line 57 uses bg-border hover:bg-border, which means the hover state is visually identical to the base state. Previously this was bg-gray-200 hover:bg-gray-300, providing distinct hover feedback.
Consider using a darker variant for hover, e.g., hover:bg-muted or hover:bg-accent:
Suggested fix
if (props.isExpanded) {
// When expanded, label is slightly darker to stand out
- base += 'bg-border hover:bg-border ';
+ base += 'bg-border hover:bg-muted ';
} else {
base += 'hover:bg-border ';
}📝 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.
| if (props.isExpanded) { | |
| // When expanded, label is slightly darker to stand out | |
| base += 'bg-gray-200 hover:bg-gray-300 '; | |
| base += 'bg-border hover:bg-border '; | |
| } else { | |
| base += 'hover:bg-gray-200 '; | |
| base += 'hover:bg-border '; | |
| } | |
| if (props.isExpanded) { | |
| // When expanded, label is slightly darker to stand out | |
| base += 'bg-border hover:bg-muted '; | |
| } else { | |
| base += 'hover:bg-border '; | |
| } |
🤖 Prompt for AI Agents
In
`@packages/web/src/components/project/reconcile-tab/rob2-reconcile/NavbarDomainPill.jsx`
around lines 55 - 60, In NavbarDomainPill.jsx the computed class string stored
in base uses identical classes for expanded hover state (currently "bg-border
hover:bg-border"), which removes hover feedback; update the branch that checks
props.isExpanded to use a darker hover variant (for example replace the
hover:bg-border with hover:bg-muted or hover:bg-gray-300) so the expanded state
still shows a visual change on hover; ensure you modify the code that builds the
base variable inside the component (the props.isExpanded branch) and keep the
non-expanded branch as-is.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/components/project/add-studies/AddStudiesForm.jsx (1)
84-87: Close toggle becomes a no-op once studies are staged.
isExpanded()stays true whenstudies.hasAnyStudies()is true, so the Close button appears active but cannot collapse. Either allow collapse when studies exist or disable/hide the toggle to avoid misleading UX.Proposed fix (disable toggle when it cannot collapse)
+ const canToggle = () => !studies.hasAnyStudies(); <button type='button' - onClick={() => setExpanded(!expanded())} + onClick={() => canToggle() && setExpanded(!expanded())} + disabled={!canToggle()} class='flex items-center gap-2 rounded-lg px-3 py-2 text-sm font-medium transition-colors' classList={{ 'bg-primary text-white': isExpanded(), 'bg-primary-subtle text-primary hover:bg-primary/20': !isExpanded(), }} >Also applies to: 206-219
🤖 Fix all issues with AI agents
In `@packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx`:
- Around line 74-97: The validation currently allows totals of 99-101% but the
UI text says 99-100% and resetToEven can produce sums that are invalid; keep the
isPoolValid check as-is (>=99 && <=101) and update the warning/copy to say
"99-101%" to match, and change resetToEven to compute an integer even
distribution that sums exactly to 100 for each pool: for each pool use count =
number of members in that pool, base = Math.floor(100 / count), remainder = 100
- base*count, then assign base+1 to the first "remainder" member IDs and base to
the rest via setPool1Percents and setPool2Percents (and still call
setShowPreview(false)); reference functions/vars: isPoolValid, resetToEven,
setPool1Percents, setPool2Percents, pool1Total/pool2Total and the member ID
lists used when building percents.
♻️ Duplicate comments (1)
packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx (1)
249-314: Consider extracting MemberPercentRow to avoid redefinition on each render.This component is recreated on every parent update, which can reset internal state and cause avoidable re-renders. Moving it out improves stability and aligns with SolidJS best practices.
🧹 Nitpick comments (2)
packages/web/src/components/project/add-studies/AddStudiesForm.jsx (2)
226-318: Extract the repeated tabs + content block to avoid drift.The Tabs list, tab counts, and tab content are duplicated three times. Consider extracting a shared render function or subcomponent so future changes remain consistent across all layouts.
Also applies to: 330-408, 418-505
22-31: Consider consolidating props into a store/config object.This component takes many props and mixes state, persistence, and submit behaviors. A store or a single config object would reduce prop-drilling and align with the local-props-only guideline. As per coding guidelines, prefer external stores for shared state.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
🧰 Additional context used
📓 Path-based instructions (14)
packages/web/src/**/*.{js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/form-state.mdc)
packages/web/src/**/*.{js,jsx}: Save form state to IndexedDB before initiating OAuth redirects (Google Drive, ORCID) using saveFormState() with form type ('createProject' or 'addStudies') and serializable state only
Only save serializable data to IndexedDB—exclude File objects, ArrayBuffers, functions, and other non-serializable objects from form state persistence
Restore form state after OAuth redirects by checking URL restore params with getRestoreParamsFromUrl(), retrieving saved state with getFormState(), restoring to form, clearing saved state with clearFormState(), and clearing URL params with clearRestoreParamsFromUrl()
For temporary File object storage during OAuth flows (e.g., pending PDFs), use projectStore.setPendingProjectData() instead of IndexedDB, as File objects cannot be serialized
Add restore parameters to the URL after OAuth redirects in the format '?restore=&projectId=' to signal form state restoration on mount
Clear URL restore parameters after form state restoration by calling clearRestoreParamsFromUrl() to prevent stale restoration attempts on subsequent navigation
Form state persistence library functions (saveFormState, getFormState, clearFormState, getRestoreParamsFromUrl, clearRestoreParamsFromUrl) are implemented in packages/web/src/lib/formStatePersistence.js and should be imported from@/lib/formStatePersistence.js
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/web/src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/pdf-handling.mdc)
packages/web/src/**/*.{js,jsx,ts,tsx}: Always useuploadPdffrom@api/pdf-api.jsfor uploading PDF files. Pass object with projectId, studyId, tag ('primary' or 'supplementary'), and fileName
Validate PDF files on frontend by checking file.type includes 'pdf' and file.size is within MAX_PDF_SIZE limit (full validation is done on backend)
UsecachePdfandgetCachedPdffrom@primitives/pdfCache.jsfor caching PDF data in IndexedDB
Implement PDF caching strategy: check cache first, download from server if not cached, then cache the downloaded PDF
Store only PDF metadata in Yjs (id, name, size, tag, uploadedAt, uploadedBy), never store PDF binary data in Yjs as it is too large
UseimportFromGoogleDrivefrom@api/google-drive.jsfor importing PDFs from Google Drive. Pass fileId, projectId, studyId, and tag
Save form state usingsaveFormStatefrom@/lib/formStatePersistence.jsbefore triggering OAuth redirects for Google Drive
UseaddPdfToStudyoperation fromuseProjecthook to add PDFs to a study, passing id, name, size, tag, uploadedAt, and uploadedBy
UseremovePdfFromStudyoperation fromuseProjecthook to remove PDFs from a study
UsepdfPreviewStorefrom@/stores/pdfPreviewStore.jsfor managing PDF preview state (openPreview, closePreview, getPreview methods)
UsedownloadPdffrom@api/pdf-api.jsto download PDFs from server, then cache the result usingcachePdf
Always cache PDFs after download and check cache before downloading to avoid redundant downloads
Use PDF operations fromuseProjecthook instead of bypassing through direct API calls
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/pdf-handling.mdc)
Use
PdfViewercomponent from@/components/checklist-ui/pdf/PdfViewer.jsxfor displaying PDFs, passing pdfData as ArrayBuffer, fileName, readOnly, and onPageChange
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
{packages/web/**,packages/landing/**}/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/solidjs.mdc)
{packages/web/**,packages/landing/**}/**/*.{jsx,tsx}: NEVER destructure props in SolidJS components - access props directly or wrap in functions to maintain reactivity
Use external stores in packages/web/src/stores/ for shared/cross-feature state instead of prop-drilling
Keep SolidJS components lean and focused on rendering - move business logic to stores, primitives, or utilities
Create reusable logic in primitives (hooks) in packages/web/src/primitives/ and import them into components
Use Solid's Show component for conditional rendering instead of ternary operators
Use Solid's For component for rendering lists instead of Array.map()
Use the children helper when manipulating props.children in SolidJS componentsUse Show and For components for conditional and list rendering in SolidJS
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
{packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/solidjs.mdc)
{packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx}: Use createSignal for simple reactive values in SolidJS components
Use createStore for complex objects and arrays that need granular reactivity in SolidJS
Use createMemo for computed/derived values that depend on reactive state in SolidJS
Always clean up SolidJS effects that create subscriptions or timers using onCleanup
Import stores directly in components and use store read/write action pattern - read from store, write via actions store
Prefer derived state with createMemo or signals over effects whenever possible
Use local createSignal or createStore for local component state; use external stores for shared/cross-feature state
{packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx}: NEVER destructure props in SolidJS components - it breaks reactivity. Access props directly (e.g., props.name) or wrap in a function (e.g., () => props.name)
Store imports should access store data directly from external stores (packages/web/src/stores/) rather than receiving store data via props
Use createSignal for simple, reactive values in SolidJS
Use createStore for complex objects and nested state in SolidJS, with nested updates using setState pattern (e.g., setState('items', items => [...items, newItem]))
Use createMemo for derived/computed values in SolidJS to maintain reactivity
Use local createSignal or createStore for local component state
Components should receive at most 1-5 props for local configuration only
Move business logic to stores, utilities, or SolidJS primitives rather than keeping it in component code
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/yjs-sync.mdc)
**/*.{js,jsx,ts,tsx}: Use theuseProjecthook for managing Yjs connections with reference counting instead of creating Y.Doc instances directly
Never create Y.Doc instances directly; always use the connection registry managed by useProject
Access Y.Doc connection state via projectStore.getConnectionState(projectId) instead of checking connection state directly
Read Yjs-synced data from projectStore (using getStudies, getChecklist, etc.) rather than accessing Y.Doc maps/arrays directly
Use operation functions from useProject or projectActionsStore for writing data instead of directly modifying Y.Doc
Don't store Y.Doc references in component state; always retrieve the connection through useProject
Handle connection cleanup via onCleanup() in Solid.js components or equivalent cleanup patterns, calling disconnect() when the component unmounts
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/web/src/**
📄 CodeRabbit inference engine (.cursor/rules/organizations.mdc)
packages/web/src/**: Use human-readable slug in frontend URLs: /orgs/:orgSlug/...
Use orgId (UUID) for API calls, not orgSlug, when making fetch requests to backend endpoints
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/web/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
packages/web/**/*.{js,ts,jsx,tsx}: UseapiFetchfor all frontend API calls instead of raw fetch to automatically handle JSON parsing, errors, and toast notifications
UsehandleFetchErrorfor wrapping legacy raw fetch calls in frontend code (legacy pattern, prefer apiFetch)
Use error utility functions likeisErrorCodefrom error-utils to check specific error types instead of direct string comparisons
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/web/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
Use
createFormErrorSignalsfor form error handling in frontend forms to manage field-level and global error states
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/{web,workers}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
Never throw string literals; always throw Error objects or return domain errors from API routes
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
{packages/web/src/stores/**/*.{js,jsx,ts,tsx},packages/web/src/**/*.{js,jsx,ts,tsx}}
📄 CodeRabbit inference engine (.github/instructions/solidjs.instructions.md)
Shared or cross-feature state should use external stores located in packages/web/src/stores/
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/corates.mdc)
**/*.{js,ts,jsx,tsx}: Prefer modern ES6+ syntax and features
Comments should explain why something is being done, not what the code is doing
Reserve comments for explaining intent, context, workarounds, assumptions, edge cases, or limitations
Use TODO(agent): prefix for incomplete work or flagged items with brief description and relevant doc references
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use emojis or unicode symbols anywhere in code, comments, documentation, plan files, commit messages, or examples
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
packages/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/**/*.{ts,tsx,js,jsx}: Prefer modern ES6+ syntax and features
Prefer config files over hardcoding values
Keep files small, focused, and modular - extract large files into sub-modules or separate utilities
Each file should handle one coherent responsibility
Comments should explain WHY something is being done or provide context, not repeat what the code is saying
Do NOT narrate what code is doing, don't duplicate function/variable names in comments, and don't leave stale comments that contradict the code
Use the Agent TODO convention// TODO(agent): Brief descriptionfor incomplete work, flagging items for future attention, known limitations, and documentation section referencesUse TODO(agent) convention with brief description and relevant doc section references for incomplete work or flagged issues
Files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
🧠 Learnings (28)
📚 Learning: 2025-12-27T03:02:05.951Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/pdf-handling.mdc:0-0
Timestamp: 2025-12-27T03:02:05.951Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use `addPdfToStudy` operation from `useProject` hook to add PDFs to a study, passing id, name, size, tag, uploadedAt, and uploadedBy
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:05.951Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/pdf-handling.mdc:0-0
Timestamp: 2025-12-27T03:02:05.951Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use `removePdfFromStudy` operation from `useProject` hook to remove PDFs from a study
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:14.854Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/reconciliation.mdc:0-0
Timestamp: 2025-12-27T03:02:14.854Z
Learning: Applies to {packages/web/src/components/checklist-ui/compare/**,packages/web/src/AMSTAR2/checklist-compare.js} : Use compareChecklists utility from checklist-compare.js for comparing reviewer checklists
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
📚 Learning: 2025-12-27T03:01:06.933Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/checklist-operations.mdc:0-0
Timestamp: 2025-12-27T03:01:06.933Z
Learning: Applies to packages/web/src/components/checklist-ui/**,packages/web/src/AMSTAR2/**,packages/web/src/ROBINS-I/**,packages/web/src/lib/checklist-domain.js,packages/web/src/primitives/useProject/checklists.js : Use checklist operations from useProject hook (createChecklist, updateChecklistAnswer, getChecklistData) instead of manually updating checklist structure
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:50.087Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/yjs-sync.mdc:0-0
Timestamp: 2025-12-27T03:02:50.087Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Read Yjs-synced data from projectStore (using getStudies, getChecklist, etc.) rather than accessing Y.Doc maps/arrays directly
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:14.854Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/reconciliation.mdc:0-0
Timestamp: 2025-12-27T03:02:14.854Z
Learning: Applies to {packages/web/src/components/checklist-ui/compare/**,packages/web/src/lib/checklist-domain.js} : Store reconciliation progress metadata (IDs and optional currentPage) in the study's Y.Map reconciliation property
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:50.087Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/yjs-sync.mdc:0-0
Timestamp: 2025-12-27T03:02:50.087Z
Learning: Applies to packages/workers/src/durable-objects/ProjectDoc.js, packages/web/src/primitives/useProject/** : Y.Doc structure in ProjectDoc follows a specific hierarchy: meta (Map), members (Map), reviews (Map containing studies with checklists, pdfs, and reconciliation)
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:05.951Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/pdf-handling.mdc:0-0
Timestamp: 2025-12-27T03:02:05.951Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx} : Use `PdfViewer` component from `@/components/checklist-ui/pdf/PdfViewer.jsx` for displaying PDFs, passing pdfData as ArrayBuffer, fileName, readOnly, and onPageChange
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:01:06.933Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/checklist-operations.mdc:0-0
Timestamp: 2025-12-27T03:01:06.933Z
Learning: Applies to packages/web/src/components/checklist-ui/**,packages/web/src/primitives/useProject/checklists.js : Store question notes as Y.Text objects obtained from useProject hook via getQuestionNote operation to enable collaborative editing
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:26.947Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/solidjs.mdc:0-0
Timestamp: 2025-12-27T03:02:26.947Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx} : Use local createSignal or createStore for local component state; use external stores for shared/cross-feature state
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2026-01-17T00:25:12.507Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/instructions/solidjs.instructions.md:0-0
Timestamp: 2026-01-17T00:25:12.507Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{jsx,tsx} : Use Show and For components for conditional and list rendering in SolidJS
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:26.947Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/solidjs.mdc:0-0
Timestamp: 2025-12-27T03:02:26.947Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{jsx,tsx} : Keep SolidJS components lean and focused on rendering - move business logic to stores, primitives, or utilities
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:26.947Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/solidjs.mdc:0-0
Timestamp: 2025-12-27T03:02:26.947Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx} : Use createSignal for simple reactive values in SolidJS components
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:01:54.727Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/form-state.mdc:0-0
Timestamp: 2025-12-27T03:01:54.727Z
Learning: Applies to packages/web/src/**/*.{js,jsx} : Save form state to IndexedDB before initiating OAuth redirects (Google Drive, ORCID) using saveFormState() with form type ('createProject' or 'addStudies') and serializable state only
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:50.087Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/yjs-sync.mdc:0-0
Timestamp: 2025-12-27T03:02:50.087Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use operation functions from useProject or projectActionsStore for writing data instead of directly modifying Y.Doc
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:05.951Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/pdf-handling.mdc:0-0
Timestamp: 2025-12-27T03:02:05.951Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Use `importFromGoogleDrive` from `api/google-drive.js` for importing PDFs from Google Drive. Pass fileId, projectId, studyId, and tag
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:05.951Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/pdf-handling.mdc:0-0
Timestamp: 2025-12-27T03:02:05.951Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Save form state using `saveFormState` from `@/lib/formStatePersistence.js` before triggering OAuth redirects for Google Drive
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:01:35.601Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/durable-objects.mdc:0-0
Timestamp: 2025-12-27T03:01:35.601Z
Learning: Applies to packages/workers/src/durable-objects/**/ProjectDoc.{js,ts} : Check user membership in project before allowing WebSocket connection in ProjectDoc
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:50.087Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/yjs-sync.mdc:0-0
Timestamp: 2025-12-27T03:02:50.087Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Access Y.Doc connection state via projectStore.getConnectionState(projectId) instead of checking connection state directly
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:50.087Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/yjs-sync.mdc:0-0
Timestamp: 2025-12-27T03:02:50.087Z
Learning: Applies to packages/web/src/primitives/useProject/**, packages/web/src/stores/projectStore.js : When access is denied (user removed or project deleted), the connection automatically triggers cleanup: IndexedDB data cleared, connection closed, store cleared, and user redirected
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx
📚 Learning: 2025-12-27T03:02:26.947Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/solidjs.mdc:0-0
Timestamp: 2025-12-27T03:02:26.947Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{jsx,tsx} : Use Solid's Show component for conditional rendering instead of ternary operators
Applied to files:
packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsxpackages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:01:54.727Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/form-state.mdc:0-0
Timestamp: 2025-12-27T03:01:54.727Z
Learning: Applies to packages/web/src/components/project-ui/**/*.{js,jsx} : Form state should include serializable metadata when handling files (name, size, type) and use the store's pendingPdfs pattern for actual File objects that persist across redirects
Applied to files:
packages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:05.951Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/pdf-handling.mdc:0-0
Timestamp: 2025-12-27T03:02:05.951Z
Learning: Applies to packages/web/src/**/*.{js,jsx,ts,tsx} : Always use `uploadPdf` from `api/pdf-api.js` for uploading PDF files. Pass object with projectId, studyId, tag ('primary' or 'supplementary'), and fileName
Applied to files:
packages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2026-01-17T00:25:12.507Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/instructions/solidjs.instructions.md:0-0
Timestamp: 2026-01-17T00:25:12.507Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx} : Use createSignal for simple, reactive values in SolidJS
Applied to files:
packages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2026-01-17T16:09:36.904Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-17T16:09:36.904Z
Learning: Applies to packages/{web,landing}/src/**/*.{ts,tsx} : Use `createStore` for complex state objects in SolidJS
Applied to files:
packages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2026-01-17T00:25:12.507Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/instructions/solidjs.instructions.md:0-0
Timestamp: 2026-01-17T00:25:12.507Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx} : Use local createSignal or createStore for local component state
Applied to files:
packages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:26.947Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/solidjs.mdc:0-0
Timestamp: 2025-12-27T03:02:26.947Z
Learning: Applies to {packages/web/**,packages/landing/**}/**/*.{js,jsx,ts,tsx} : Use createStore for complex objects and arrays that need granular reactivity in SolidJS
Applied to files:
packages/web/src/components/project/add-studies/AddStudiesForm.jsx
📚 Learning: 2025-12-27T03:02:14.854Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursor/rules/reconciliation.mdc:0-0
Timestamp: 2025-12-27T03:02:14.854Z
Learning: Applies to packages/web/src/components/checklist-ui/compare/** : Auto-fill final answer with reviewer1's answer when reviewers agree and no final answer exists
Applied to files:
packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx
🧬 Code graph analysis (1)
packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx (2)
packages/web/src/components/ui/toast.tsx (1)
showToast(234-234)packages/web/src/components/ui/collapsible.tsx (3)
Collapsible(113-113)CollapsibleContent(113-113)CollapsibleTrigger(113-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (7)
packages/web/src/components/project/add-studies/AddStudiesForm.jsx (1)
65-74: Ensure saved OAuth state is serializable before persistence.
handleSaveFormStateforwardsexternalStateunchanged, which could break IndexedDB persistence if it containsFileobjects or other non-serializable data. Verify that the caller properly filters non-serializable values before callingsaveFormStatefrom@/lib/formStatePersistence.jswith the correct form type ('addStudies').packages/web/src/components/project/all-studies-tab/AllStudiesTab.jsx (3)
7-78: Reviewer assignment data wiring looks solid.Pulling members from the store and memoizing unassigned studies keeps the derived state reactive and straightforward.
146-155: Conditional ReviewerAssignment rendering is clean.The visibility guard reads well and keeps the UI focused on owners with unassigned studies.
158-178: Loading and empty-state tweaks look consistent.The updated styling and copy read well and align with the rest of the tab.
packages/web/src/components/project/overview-tab/ReviewerAssignment.jsx (3)
19-63: Derived member and percentage state is well structured.The memoized member list and percent mapping read cleanly and should be easy to maintain.
320-599: Collapsible layout and preview table wiring look cohesive.The panel structure and preview flow are easy to follow and well-organized.
204-229: Confirm reviewer updates are synchronous before counting success.
handleApplyassumesonAssignReviewersis synchronous. If it returns a Promise, the try/catch will not catch failures andsuccessCountwill be optimistic. Verify whetheronAssignReviewers(which likely delegates toprojectActionsStore.study.update) is async, and if so, consider making the handler async and awaiting updates, or usePromise.allSettled()to properly handle concurrent assignment operations and accurately count successes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Summary by CodeRabbit
New Features
UI/Style
Changes
✏️ Tip: You can customize this high-level summary in your review settings.