Dev#201
Conversation
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
…wman/corates into 199-add-organizations
backend migration to organizations completed
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces a comprehensive multi-tenant organization model to CoRATES, adding organization entities with membership hierarchies, org-scoped API routes with new authorization middleware, organization context primitives for the frontend, database schema changes with migrations, and extensive documentation covering organization patterns, routing, and API design. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant AuthServer
participant DB as D1 Database
participant BootstrapLogic as Bootstrap Logic
User->>Browser: Login
Browser->>AuthServer: Authenticate
AuthServer->>AuthServer: Validate credentials
AuthServer->>DB: Check user exists
DB-->>AuthServer: User confirmed
AuthServer->>BootstrapLogic: Trigger after-auth hook
BootstrapLogic->>DB: Check: user has orgs?
DB-->>BootstrapLogic: No orgs found
BootstrapLogic->>DB: Create personal organization
DB-->>BootstrapLogic: org created (id, slug)
BootstrapLogic->>DB: Create organization membership<br/>(user as owner)
DB-->>BootstrapLogic: membership created
BootstrapLogic->>DB: Set activeOrganizationId<br/>in session
DB-->>BootstrapLogic: session updated
AuthServer-->>Browser: Auth success + session
Browser->>Browser: Redirect to /orgs/{slug}
User->>User: Logged in with org
sequenceDiagram
participant Owner
participant Browser
participant APIServer
participant EmailService
participant Invitee as Invited User
participant Email
participant AuthService
participant DB
Owner->>Browser: Create project invitation
Browser->>APIServer: POST /api/orgs/{orgId}/projects/{projectId}/invitations<br/>{email, role, orgRole}
APIServer->>APIServer: requireOrgMembership (owner)
APIServer->>APIServer: requireProjectAccess (owner)
APIServer->>DB: Check existing invitation
APIServer->>DB: Create invitation record<br/>(orgId, projectId, email, roles, token)
APIServer->>EmailService: Queue invitation email<br/>(magic-link with token)
EmailService->>Email: Send magic-link
Email-->>Invitee: Click magic-link
Invitee->>AuthService: Accept invitation (token)
AuthService->>DB: Verify token & expiry
AuthService->>DB: Check org membership
AuthService->>DB: Create org membership<br/>(if missing)
AuthService->>DB: Create project membership
AuthService->>DB: Mark invitation accepted
AuthService-->>Invitee: Redirect to project
Invitee->>Invitee: Access project with<br/>org+project roles
sequenceDiagram
participant Client
participant Middleware
participant AuthCheck as Auth Check
participant OrgCheck as Org Membership
participant ProjectCheck as Project Access
participant EntitlementCheck
participant Validation
participant Handler
Client->>Middleware: Request to protected route
Middleware->>AuthCheck: Verify session/token
AuthCheck-->>Middleware: User ID extracted
Middleware->>OrgCheck: requireOrgMembership(minRole?)
OrgCheck->>OrgCheck: Extract orgId from params
OrgCheck->>OrgCheck: Query DB: user org member?
OrgCheck-->>Middleware: Attach org context<br/>(orgId, orgRole, org)
Middleware->>ProjectCheck: requireProjectAccess(minRole?)
ProjectCheck->>ProjectCheck: Use org context
ProjectCheck->>ProjectCheck: Query DB: project in org?
ProjectCheck->>ProjectCheck: Query DB: user project member?
ProjectCheck-->>Middleware: Attach project context<br/>(projectId, projectRole)
Middleware->>EntitlementCheck: Verify entitlement/quota
EntitlementCheck-->>Middleware: OK
Middleware->>Validation: validateRequest(schema)
Validation-->>Middleware: OK
Middleware->>Handler: Invoke route handler
Handler-->>Client: 200 + response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
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/api/google-drive.js (1)
49-63: Fix missingorgIdargument in GoogleDrivePickerModal.The function signature change to include
orgIdis correctly updated in two of three call sites. However,GoogleDrivePickerModal.jsx:31still callsimportFromGoogleDrive(file.id, projectId(), studyId)without passing the requiredorgIdparameter.Additionally, the component needs to accept
orgIdas a prop (currently missing from JSDoc lines 12-18), andAllStudiesTab.jsxneeds to destructureorgIdfromuseProjectContext()and pass it to the modal.
🧹 Nitpick comments (29)
packages/workers/src/routes/billing/index.js (2)
55-57: Good tier normalization with defensive validation.The tier normalization and validation logic is well-implemented. Consider applying similar validation to the default response (lines 38-45) for consistency, where
TIER_INFO.freeis hardcoded instead of usingTIER_INFO[DEFAULT_SUBSCRIPTION_TIER].Optional consistency improvement
return c.json({ tier: DEFAULT_SUBSCRIPTION_TIER, status: DEFAULT_SUBSCRIPTION_STATUS, - tierInfo: TIER_INFO.free, + tierInfo: TIER_INFO[DEFAULT_SUBSCRIPTION_TIER] ?? TIER_INFO.free, stripeSubscriptionId: null, currentPeriodEnd: null, cancelAtPeriodEnd: false, });
60-66: Response construction looks good.The response correctly uses the normalized
validTierand convertedcurrentPeriodEnd. The nullish coalescing operator on line 62 (?? TIER_INFO.free) is redundant sincevalidTieris guaranteed to be a valid key inTIER_INFO, but it doesn't hurt as defensive coding.packages/web/src/components/org/CreateOrgPage.jsx (1)
24-37: Consider simplifying slug generation logic.The slug generation works correctly but uses multiple chained
.replace()calls. While this is functional, you could consider combining some operations for better readability.Proposed simplification
- const generatedSlug = newName - .toLowerCase() - .replace(/\s+/g, '-') - .replace(/[^a-z0-9-]/g, '') - .replace(/-+/g, '-') - .replace(/^-|-$/g, ''); + const generatedSlug = newName + .toLowerCase() + .replace(/\s+/g, '-') // spaces to hyphens + .replace(/[^a-z0-9-]/g, '') // keep only alphanumeric and hyphens + .replace(/-{2,}/g, '-') // collapse multiple hyphens + .replace(/^-+|-+$/g, ''); // trim hyphens from start/endThe changes are minimal but add clarity with comments and slightly more efficient regex patterns (
-{2,}instead of-+and^-+|-+$to be more explicit about trimming multiple hyphens).packages/web/src/primitives/useRecentsNav.js (1)
58-109: Consider adding comments to clarify nested regex matching logic.The path parsing logic is correct but uses nested regex matches that progressively check for more specific routes. While functional, adding brief comments could improve readability.
Proposed clarification
function parsePathToRecentItem(path) { - // Match project pages: /orgs/:slug/projects/:projectId + // Try to match org-scoped project routes (most specific match wins) const projectMatch = path.match(/^\/orgs\/([^/]+)\/projects\/([^/]+)/); if (projectMatch) { const [, orgSlug, projectId] = projectMatch; - // Check if it's a study page: /orgs/:slug/projects/:projectId/studies/:studyId + // Check if path also includes a study segment const studyMatch = path.match(/^\/orgs\/([^/]+)\/projects\/([^/]+)\/studies\/([^/]+)/); if (studyMatch) { - const [, , , studyId] = studyMatch; - // Check if it's a checklist page + const [, , , studyId] = studyMatch; // Skip orgSlug, projectId captures + // Check if path also includes a checklist segment const checklistMatch = path.match( /^\/orgs\/([^/]+)\/projects\/([^/]+)\/studies\/([^/]+)\/checklists\/([^/]+)/, );This makes the progressive matching strategy more explicit.
packages/web/src/components/sidebar/Sidebar.jsx (1)
458-640: Consider extracting duplicated sidebar content into a shared component.The mobile overlay Portal correctly implements a slide-in panel with backdrop and proper z-indexing. However, the sidebar content (lines 497-636) is duplicated from the desktop expanded content (lines 250-391).
While the current implementation works correctly, extracting the common sidebar body into a shared component would:
- Reduce maintenance burden (changes only need to be made in one place)
- Follow DRY principles
- Reduce bundle size
Example approach:
// Extract to SidebarContent.jsx function SidebarContent(props) { return ( <> {/* Dashboard/Workspace Link */} <div class='p-2 pt-3'>...</div> {/* Recents Section */} <Show when={props.recents().length > 0}>...</Show> {/* Cloud Projects Section */} <Show when={props.isLoggedIn() && props.orgSlug()}>...</Show> {/* Local Checklists Section */} <div class='px-3 pt-6 pb-2'>...</div> </> ); } // Then use in both desktop and mobile contexts <SidebarContent {...sharedProps} />packages/workers/src/middleware/requireOrg.js (1)
236-240: Clarify role hierarchy comments.The comments use
>which typically implies "greater than," but the arrays are ordered from lowest to highest privilege. Consider clarifying to avoid confusion:Suggested clarification
-// Org role hierarchy: owner > admin > member +// Org role hierarchy (lowest to highest): member < admin < owner const ORG_ROLE_HIERARCHY = ['member', 'admin', 'owner']; -// Project role hierarchy: viewer > member > collaborator > owner +// Project role hierarchy (lowest to highest): viewer < member < collaborator < owner const PROJECT_ROLE_HIERARCHY = ['viewer', 'member', 'collaborator', 'owner'];packages/workers/src/index.js (1)
192-205: Consider using domain error pattern for consistency.The legacy endpoint handler correctly returns 410 Gone to indicate the endpoints have moved. However, the error response format differs from the domain error pattern used elsewhere in the codebase.
Optional: Align with domain error pattern
For consistency with error handling patterns in the codebase, consider using a domain error:
+import { createDomainError, SYSTEM_ERRORS } from '@corates/shared'; + -const legacyGoneHandler = c => - c.json( - { - error: 'ENDPOINT_MOVED', - message: 'This endpoint has been moved. Use /api/orgs/:orgId/projects/... instead.', - statusCode: 410, - }, - 410, - ); +const legacyGoneHandler = c => { + const error = createDomainError( + SYSTEM_ERRORS.ENDPOINT_MOVED || 'ENDPOINT_MOVED', + 'This endpoint has been moved. Use /api/orgs/:orgId/projects/... instead.', + { statusCode: 410 } + ); + return c.json(error, 410); +};Note: This assumes ENDPOINT_MOVED exists in SYSTEM_ERRORS or can be added. The current implementation is acceptable for a simple deprecation notice.
packages/workers/src/config/validation.js (1)
82-90: Consider extracting org role enum to constants.The new
invitationSchemas.createcorrectly validates invitation data with appropriate field constraints. The schema follows established patterns and reusesPROJECT_ROLESas recommended.Optional: Extract org role enum for consistency
For consistency with the
PROJECT_ROLESpattern and to enable reuse across the codebase, consider extracting the org role enum to constants:In
packages/workers/src/config/constants.js:+export const ORG_ROLES = ['member', 'admin', 'owner'];Then update the schema:
-import { PROJECT_ROLES } from './constants.js'; +import { PROJECT_ROLES, ORG_ROLES } from './constants.js'; export const invitationSchemas = { create: z.object({ email: z.string().email('Invalid email address'), role: z.enum(PROJECT_ROLES, { error: `Role must be one of: ${PROJECT_ROLES.join(', ')}`, }), - orgRole: z.enum(['member', 'admin', 'owner'], { - error: "Org role must be one of: 'member', 'admin', 'owner'", + orgRole: z.enum(ORG_ROLES, { + error: `Org role must be one of: ${ORG_ROLES.join(', ')}`, }), }),Based on coding guidelines for validation schemas.
packages/web/src/components/org/OrgRedirect.jsx (1)
44-68: Consider: Brief content flash during authenticated redirect.When logged in with orgs, the outer
Showresolves to true (not loading) before the effect navigates away. This may briefly render an empty container. Consider adding a condition to avoid rendering when navigation is imminent.Optional: Add logged-in-with-orgs check to prevent flash
> - <Show when={!isLoggedIn()}> + <Show when={!isLoggedIn() && !hasNoOrgs()}> <div class='text-center'> <h2 class='mb-2 text-xl font-semibold text-gray-900'>Welcome to CoRATES</h2>Alternatively, you could add a
navigatingsignal that's set before navigation to suppress the empty state.packages/workers/src/auth/config.js (1)
344-344: Consider slug uniqueness edge case.The slug is generated as
${userName}-${orgId.slice(0,8)}. While the UUID suffix makes collisions unlikely, theslugcolumn has auniqueconstraint (per schema). If a collision somehow occurs (e.g., same 8-char UUID prefix), the insert will fail silently due to the try-catch, leaving the user without an org.Consider adding retry logic or using the full UUID for guaranteed uniqueness.
packages/docs/architecture/diagrams/06-api-routes.md (1)
147-155: Add language specifier to fenced code block.The fenced code block should have a language specified for proper rendering and accessibility.
Proposed fix
-``` +```text requireAuth -> requireOrgMembership(minRole) -> requireProjectAccess(minRole) -> handler</details> </blockquote></details> <details> <summary>packages/workers/src/routes/orgs/index.js (3)</summary><blockquote> `47-56`: **Use `VALIDATION_ERRORS` instead of `AUTH_ERRORS.FORBIDDEN` for input validation.** The error for missing `name` field is a validation concern, not an authorization issue. Using `AUTH_ERRORS.FORBIDDEN` with status 403 is semantically incorrect for missing input. As per coding guidelines, use validation middleware or appropriate validation errors. <details> <summary>Proposed fix</summary> ```diff if (!body.name?.trim()) { - const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { - reason: 'name_required', + const error = createDomainError(VALIDATION_ERRORS.FIELD_REQUIRED, { + field: 'name', }); return c.json(error, error.statusCode); }Also add
VALIDATION_ERRORSto the import on line 24:-import { createDomainError, AUTH_ERRORS, SYSTEM_ERRORS } from '@corates/shared'; +import { createDomainError, AUTH_ERRORS, SYSTEM_ERRORS, VALIDATION_ERRORS } from '@corates/shared';
231-243: UseVALIDATION_ERRORS.FIELD_REQUIREDfor missinguserId.Consistent with the previous comment, missing required fields should use validation errors.
Proposed fix
if (!userId) { - const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { - reason: 'user_id_required', + const error = createDomainError(VALIDATION_ERRORS.FIELD_REQUIRED, { + field: 'userId', }); return c.json(error, error.statusCode); }
281-290: UseVALIDATION_ERRORS.FIELD_REQUIREDfor missingrole.Proposed fix
if (!role) { - const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { - reason: 'role_required', + const error = createDomainError(VALIDATION_ERRORS.FIELD_REQUIRED, { + field: 'role', }); return c.json(error, error.statusCode); }packages/workers/migrations/0001_nice_blonde_phantom.sql (2)
14-22: Consider adding unique constraint on(userId, organizationId)to prevent duplicate memberships.The
membertable allows a user to be added multiple times to the same organization. A unique constraint would enforce data integrity at the database level.Proposed addition after line 22
--> statement-breakpoint CREATE UNIQUE INDEX `member_user_org_unique` ON `member` (`userId`, `organizationId`);
34-36: Migration may fail on existing data due to NOT NULL constraint.Adding
orgIdasNOT NULLto existing tables (project_invitations,projects) will fail if these tables contain existing rows without a default value or data migration.If this is a greenfield deployment, this is fine. Otherwise, consider a multi-step migration:
- Add column as nullable
- Backfill existing rows with a default org
- Alter to NOT NULL
Would you like me to help create a safe multi-step migration for existing data?
packages/web/src/stores/projectActionsStore/members.js (1)
29-50: Consider handling org-specific error codes.The fetch call now uses org-scoped endpoints, but error handling only checks for generic errors. Based on the test file
packages/workers/src/routes/__tests__/org-auth.test.js, the backend may return org-specific error codes likePROJECT_NOT_IN_ORG,AUTH_FORBIDDEN(withnot_org_memberreason), etc.Consider checking for org-specific error codes and providing more specific error messages:
Suggested enhancement for org-specific error handling
try { const response = await fetch( `${API_BASE}/api/orgs/${orgId}/projects/${projectId}/members/${memberId}`, { method: 'DELETE', credentials: 'include', }, ); if (!response.ok) { const data = await response.json(); + // Handle org-specific error codes + if (data.code === 'PROJECT_NOT_IN_ORG') { + throw new Error('Project does not belong to the current organization'); + } + if (data.code === 'AUTH_FORBIDDEN' && data.details?.reason === 'not_org_member') { + throw new Error('You are no longer a member of this organization'); + } throw new Error(data.error || 'Failed to remove member'); }packages/web/src/components/org/OrgProjectsPage.jsx (1)
100-127: Consider usinghandleFetchErrorfrom error-utils.Per coding guidelines, frontend fetch calls should use
handleFetchErrorwith optionalshowToastparameter. The current implementation manually parses errors and uses a dynamic import.Suggested refactor using handleFetchError
const handleDeleteProject = async targetProjectId => { const confirmed = await confirmDialog.open({ title: 'Delete Project', description: 'Are you sure you want to delete this entire project? This action cannot be undone.', confirmText: 'Delete Project', variant: 'danger', }); if (!confirmed) return; try { const response = await fetch(`${API_BASE}/api/orgs/${orgId()}/projects/${targetProjectId}`, { method: 'DELETE', credentials: 'include', }); if (!response.ok) { - const data = await response.json().catch(() => ({})); - throw new Error(data.error || 'Failed to delete project'); + await handleFetchError(response, { showToast: true, toastTitle: 'Delete Failed' }); + return; } queryClient.invalidateQueries({ queryKey: queryKeys.projects.byOrg(orgId()) }); showToast.success('Project Deleted', 'The project has been deleted successfully'); } catch (err) { - const { handleError } = await import('@/lib/error-utils.js'); - await handleError(err, { toastTitle: 'Delete Failed' }); + console.error('Delete project error:', err); } };This requires importing
handleFetchErrorfrom@/lib/error-utils.jsat the top of the file.packages/docs/guides/organizations.md (1)
20-34: Consider adding language identifier to fenced code blocks.The markdownlint hints flag blocks at lines 20, 150, 274, and 406 for missing language specifiers. For ASCII diagrams like this entity hierarchy, you can use
textorplaintextas the language identifier to silence the lint warning while preserving formatting.Example fix
-``` +```text Organization | +-- Projects (owned by org)packages/web/src/components/Navbar.jsx (1)
99-157: Workspace switcher missing keyboard accessibility.The workspace switcher dropdown is only dismissible via mouse clicks. Consider adding keyboard support for accessibility (e.g., Escape key to close, arrow key navigation).
Suggested enhancement for keyboard accessibility
+ // Add keyboard handler for dropdown + const handleOrgKeyDown = (e) => { + if (e.key === 'Escape') { + setShowOrgMenu(false); + } + }; <div class='relative' ref={orgMenuRef}> <button onClick={() => setShowOrgMenu(!showOrgMenu())} + onKeyDown={handleOrgKeyDown} + aria-expanded={showOrgMenu()} + aria-haspopup="listbox" class='flex h-8 items-center gap-2 rounded-lg bg-white/10 px-3 text-sm font-medium transition hover:bg-white/20' >packages/web/src/stores/projectActionsStore/project.js (1)
58-77: Missing null check for orgId in deleteById.If neither
targetOrgIdnorgetActiveOrgId()returns a value, the API call will have an undefined segment in the URL. Consider adding validation.Suggested fix
async function deleteById(targetProjectId, targetOrgId) { const orgId = targetOrgId || getActiveOrgId(); + if (!orgId) { + throw new Error('Organization ID is required for deletion'); + } try { const response = await fetch(`${API_BASE}/api/orgs/${orgId}/projects/${targetProjectId}`, {packages/workers/src/routes/orgs/members.js (2)
142-153: Potential race condition in ensureOrgMembership followed by project member insert.The
ensureOrgMembershipcall and subsequentprojectMembers.insertare not atomic. Under concurrent requests, this could lead to duplicate membership attempts or inconsistent state.Consider using
db.batch()to make these operations atomic, as per the coding guidelines.Suggested approach
- // Ensure org membership first (combined flow) - await ensureOrgMembership(db, orgId, userToAdd.id); - - // Add project membership - const now = new Date(); - await db.insert(projectMembers).values({ - id: crypto.randomUUID(), - projectId, - userId: userToAdd.id, - role, - joinedAt: now, - }); + // Ensure org membership and add project membership atomically + const now = new Date(); + const projectMemberId = crypto.randomUUID(); + + // Check existing org membership + const existingOrgMembership = await db + .select({ id: member.id }) + .from(member) + .where(and(eq(member.organizationId, orgId), eq(member.userId, userToAdd.id))) + .get(); + + if (!existingOrgMembership) { + // Need to add both memberships + await db.batch([ + db.insert(member).values({ + id: crypto.randomUUID(), + userId: userToAdd.id, + organizationId: orgId, + role: 'member', + createdAt: now, + }), + db.insert(projectMembers).values({ + id: projectMemberId, + projectId, + userId: userToAdd.id, + role, + joinedAt: now, + }), + ]); + } else { + // Only add project membership + await db.insert(projectMembers).values({ + id: projectMemberId, + projectId, + userId: userToAdd.id, + role, + joinedAt: now, + }); + }
507-620: Consider extracting email sending logic.The
handleInvitationfunction is quite long (~115 lines) with email template generation and magic link creation embedded. Consider extracting the email-related logic into a separate utility for better maintainability.packages/web/src/api/pdf-api.js (1)
54-81: Consider adding parameter validation.The
uploadPdffunction doesn't validate thatorgId,projectId, orstudyIdare provided, which could lead to malformed URLs with undefined segments.Suggested validation
export async function uploadPdf(orgId, projectId, studyId, file, fileName = null) { + if (!orgId || !projectId || !studyId) { + throw new Error('orgId, projectId, and studyId are required'); + } const url = buildPdfBaseUrl(orgId, projectId, studyId);packages/web/src/stores/projectActionsStore/index.js (1)
168-170: Consider consistent naming for public getters.The public API exposes
getActiveProjectIdwhich maps togetActiveProjectIdOrNull, andgetActiveOrgIdwhich maps togetActiveOrgIdOrNull. This is slightly misleading as the names suggest they might throw.Consider renaming for clarity:
// Option A: Rename the mappings to be explicit getActiveProjectIdOrNull: getActiveProjectIdOrNull, getActiveOrgIdOrNull: getActiveOrgIdOrNull, // Option B: Keep short names but document clearlypackages/workers/src/routes/orgs/invitations.js (2)
173-207: Dynamic imports on every request are inefficient.These modules are imported dynamically on each POST request, adding latency. Move them to top-level static imports unless there's a specific code-splitting requirement.
Proposed fix
Move these imports to the top of the file:
+import { betterAuth } from 'better-auth'; +import { magicLink } from 'better-auth/plugins'; +import { drizzleAdapter } from 'better-auth/adapters/drizzle'; +import { drizzle } from 'drizzle-orm/d1'; +import * as schema from '../../db/schema.js'; +import { MAGIC_LINK_EXPIRY_MINUTES, getProjectInvitationEmailHtml, getProjectInvitationEmailText } from '../../auth/emailTemplates.js'; +import { escapeHtml } from '../../lib/escapeHtml.js';Then remove the dynamic imports inside the route handler.
308-314: Use a semantically correct error for "not found" case.
VALIDATION_ERRORS.FIELD_INVALID_FORMATindicates a format issue, not a missing resource. Consider using a more appropriate error or creating anINVITATION_NOT_FOUNDerror.Proposed fix
if (!invitation) { - const error = createDomainError(VALIDATION_ERRORS.FIELD_INVALID_FORMAT, { - field: 'invitationId', - value: invitationId, - }); + const error = createDomainError(PROJECT_ERRORS.NOT_FOUND, { + resource: 'invitation', + invitationId, + }); return c.json(error, error.statusCode); }packages/workers/src/routes/orgs/projects.js (1)
319-342: Consider batching R2 deletes to avoid rate limits.For projects with many PDFs,
Promise.allon all delete operations could hit R2 rate limits. Consider using a chunked approach or sequential deletion with concurrency limits.Proposed improvement using chunked deletion
if (listed.objects.length > 0) { const keysToDelete = listed.objects.map(obj => obj.key); - await Promise.all(keysToDelete.map(key => c.env.PDF_BUCKET.delete(key))); + // Delete in chunks to avoid rate limits + const chunkSize = 10; + for (let i = 0; i < keysToDelete.length; i += chunkSize) { + const chunk = keysToDelete.slice(i, i + chunkSize); + await Promise.all(chunk.map(key => c.env.PDF_BUCKET.delete(key))); + } deletedCount += keysToDelete.length; }packages/workers/src/db/schema.js (1)
36-47: Add indexes on foreign key columns for improved query performance.The foreign key columns
member.userId,member.organizationId,invitation.inviterId, andinvitation.organizationIdlack indexes. SQLite does not automatically create indexes for non-unique foreign keys, which can impact performance on queries filtering or joining by these columns. Consider adding indexes in a migration.
| it('should signout successfully', async () => { | ||
| authClient.signOut.mockResolvedValue({ | ||
| error: null, | ||
| }); | ||
|
|
||
| await authStore.signout(); | ||
|
|
||
| expect(authClient.signOut).toHaveBeenCalled(); | ||
| expect(projectStore.clearProjectList).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test coverage gap: signout should verify project data cleanup.
The test name was simplified from "signout successfully and clear data" to "signout successfully", and the test no longer verifies that project data is cleared. Even if the cleanup responsibility has moved to a different module, this test should verify the end-to-end behavior to ensure users' local data is properly cleared on logout.
Consider adding an assertion or integration test to verify that:
- Local project data is cleared from stores
- IndexedDB project data is cleared
- Any cached data related to projects is invalidated
Suggested test enhancement
If project cleanup is handled by the signout implementation, add verification:
it('should signout successfully', async () => {
authClient.signOut.mockResolvedValue({
error: null,
});
await authStore.signout();
expect(authClient.signOut).toHaveBeenCalled();
+ // Verify project data cleanup happens (adjust based on actual implementation)
+ // Example: expect(projectStore.someCleanupMethod).toHaveBeenCalled();
});If cleanup is handled elsewhere, add a note explaining where this behavior is tested.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/api/__tests__/better-auth-store.test.js around lines 300 to
308, the test "should signout successfully" was simplified and no longer asserts
that project-related data is cleaned up on signout; restore end-to-end
verification by adding assertions after await authStore.signout() that confirm
(1) the in-memory project store(s) are emptied or reset to initial state, (2)
IndexedDB entries for projects have been removed (query the test DB or mock the
IDB delete calls), and (3) any project-related caches or query clients have been
invalidated (e.g., ensure cache keys are cleared or invalidate calls were made);
if cleanup is now the responsibility of another module, update the test to call
that module or add a comment referencing the specific test that covers cleanup
instead of removing these assertions.
| // Set active project for action store | ||
| createEffect(() => { | ||
| const pid = params.projectId; | ||
| const oid = orgId(); | ||
| if (pid && oid) { | ||
| projectActionsStore._setActiveProject(pid, oid); | ||
| connect(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full ReconciliationWrapper.jsx file to see the complete context
wc -l packages/web/src/components/checklist/compare/ReconciliationWrapper.jsxRepository: InfinityBowman/corates
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Read the component file to check for onCleanup usage and full connect() context
cat -n packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx | head -100Repository: InfinityBowman/corates
Length of output: 4218
🏁 Script executed:
#!/bin/bash
# Find and examine the useProject hook implementation
fd "useProject" packages/web/src/primitives --type fRepository: InfinityBowman/corates
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Search for onCleanup patterns with connect/disconnect in the web package
rg -n "onCleanup.*disconnect|onCleanup.*connect" packages/web/src --type js --type jsx --type ts --type tsx -A 2Repository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check for connect() usage patterns with cleanup in the codebase
rg -n "connect\(\)" packages/web/src --type js --type jsx --type ts --type tsx -B 2 -A 3 | head -50Repository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Find useProject hook - check index.js in useProject directory
ls -la packages/web/src/primitives/useProject/Repository: InfinityBowman/corates
Length of output: 611
🏁 Script executed:
#!/bin/bash
# Read the useProject hook implementation
cat -n packages/web/src/primitives/useProject/index.js | head -150Repository: InfinityBowman/corates
Length of output: 6050
🏁 Script executed:
#!/bin/bash
# Search for onCleanup with connect in web package - use -C for context instead
rg -n "onCleanup" packages/web/src --type js -A 3 | grep -A 3 -B 3 "connect\|disconnect"Repository: InfinityBowman/corates
Length of output: 2145
🏁 Script executed:
#!/bin/bash
# Check if there's an onCleanup in ReconciliationWrapper by reading more of the file
cat -n packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx | sed -n '40,100p'Repository: InfinityBowman/corates
Length of output: 2420
🏁 Script executed:
#!/bin/bash
# Read more of the useProject hook to understand connect and disconnect functions
cat -n packages/web/src/primitives/useProject/index.js | sed -n '149,250p'Repository: InfinityBowman/corates
Length of output: 5620
🏁 Script executed:
#!/bin/bash
# Check the connection.js file to see what connect/disconnect do
cat -n packages/web/src/primitives/useProject/connection.jsRepository: InfinityBowman/corates
Length of output: 9805
🏁 Script executed:
#!/bin/bash
# Read the rest of the useProject hook to see the full onCleanup and disconnect handling
cat -n packages/web/src/primitives/useProject/index.js | sed -n '250,310p'Repository: InfinityBowman/corates
Length of output: 2262
🏁 Script executed:
#!/bin/bash
# Check if there are examples of tracking projectId changes in effects within the codebase
rg -n "params\.projectId" packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx -A 2 -B 2Repository: InfinityBowman/corates
Length of output: 1885
Remove the redundant connect() call—useProject already auto-connects.
The useProject hook automatically establishes the connection when projectId is provided (its internal createEffect handles this). Calling connect() again in the component's effect is redundant and unnecessary. The cleanup is already properly managed by the hook's onCleanup handler.
Keep only the projectActionsStore._setActiveProject() call:
Suggested change
createEffect(() => {
const pid = params.projectId;
const oid = orgId();
if (pid && oid) {
projectActionsStore._setActiveProject(pid, oid);
// Remove: connect() is already called by useProject hook
}
});🤖 Prompt for AI Agents
In packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx
around lines 46-54, remove the redundant connect() invocation inside the
createEffect so the effect only calls projectActionsStore._setActiveProject(pid,
oid) when pid and oid are present; leave the existing createEffect structure and
ensure no other logic is removed or altered (the useProject hook manages
connection and cleanup), then run lint/tests to verify no usages depended on the
removed call.
| // Build org-scoped project path | ||
| const getProjectPath = () => { | ||
| const slug = orgSlug(); | ||
| if (slug) { | ||
| return `/orgs/${slug}/projects/${params.projectId}`; | ||
| } | ||
| return `/projects/${params.projectId}`; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Routes.jsx file
fd -t f "Routes\.jsx" packages/web/srcRepository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
# Examine Routes.jsx to see project route definitions
cat -n packages/web/src/Routes.jsx | grep -A 2 -B 2 "projects\|orgs"Repository: InfinityBowman/corates
Length of output: 829
🏁 Script executed:
# Search entire Routes.jsx for /projects/ pattern (without org prefix)
grep -n "'/projects/" packages/web/src/Routes.jsxRepository: InfinityBowman/corates
Length of output: 48
🏁 Script executed:
# Get the full Routes.jsx to see all route definitions
wc -l packages/web/src/Routes.jsxRepository: InfinityBowman/corates
Length of output: 96
🏁 Script executed:
# Read entire Routes.jsx
cat -n packages/web/src/Routes.jsxRepository: InfinityBowman/corates
Length of output: 4109
🏁 Script executed:
# Examine ReconciliationWrapper to understand orgSlug signal
cat -n packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx | head -400 | tail -100Repository: InfinityBowman/corates
Length of output: 4035
🏁 Script executed:
# Check where orgSlug is defined in ReconciliationWrapper
head -50 packages/web/src/components/checklist/compare/ReconciliationWrapper.jsxRepository: InfinityBowman/corates
Length of output: 1869
🏁 Script executed:
# Check useOrgContext implementation
fd -t f "useOrgContext" packages/web/srcRepository: InfinityBowman/corates
Length of output: 110
🏁 Script executed:
# Check useOrgContext to see how orgSlug is derived
cat -n packages/web/src/primitives/useOrgContext.jsRepository: InfinityBowman/corates
Length of output: 4506
Remove the unreachable fallback path—orgSlug is guaranteed to exist.
ReconciliationWrapper is only accessible via the route /orgs/:orgSlug/projects/:projectId/studies/:studyId/reconcile/:checklist1Id/:checklist2Id, which always includes :orgSlug in the URL parameters. The fallback /projects/${params.projectId} can never be reached, and this route no longer exists in Routes.jsx anyway. Remove the dead code and return the org-scoped path directly:
const getProjectPath = () => {
return `/orgs/${orgSlug()}/projects/${params.projectId}`;
};🤖 Prompt for AI Agents
packages/web/src/components/checklist/compare/ReconciliationWrapper.jsx around
lines 375-382: the current getProjectPath includes an unreachable fallback that
returns `/projects/${params.projectId}` because orgSlug is guaranteed to exist
for this route; remove the conditional and dead code and have getProjectPath
unconditionally return the org-scoped path using orgSlug() and params.projectId
(e.g., return `/orgs/${orgSlug()}/projects/${params.projectId}`), eliminating
the unused branch.
| const response = await fetch(`${API_BASE}/api/orgs/${orgId}/projects`, { | ||
| credentials: 'include', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const data = await response.json().catch(() => ({})); | ||
| throw new Error(data.error || data.message || 'Failed to fetch projects'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use handleFetchError for fetch calls.
According to the coding guidelines, all frontend fetch calls should use handleFetchError from @/lib/error-utils.js. The current implementation manually handles errors.
Based on coding guidelines: Always use handleFetchError from @/lib/error-utils.js for frontend fetch calls.
Refactor to use handleFetchError
+import { handleFetchError } from '@/lib/error-utils.js';
+
async function fetchOrgProjects(orgId) {
if (!orgId) {
return [];
}
- const response = await fetch(`${API_BASE}/api/orgs/${orgId}/projects`, {
+ const response = await handleFetchError(
+ fetch(`${API_BASE}/api/orgs/${orgId}/projects`, {
- credentials: 'include',
- headers: { 'Content-Type': 'application/json' },
- });
-
- if (!response.ok) {
- const data = await response.json().catch(() => ({}));
- throw new Error(data.error || data.message || 'Failed to fetch projects');
- }
+ credentials: 'include',
+ headers: { 'Content-Type': 'application/json' },
+ }),
+ { showToast: false }
+ );
return response.json();
}🤖 Prompt for AI Agents
In packages/web/src/primitives/useOrgProjectList.js around lines 21 to 29,
replace the manual response.ok check and custom error parsing with the shared
helper: import handleFetchError from '@/lib/error-utils.js' (or update existing
imports), call handleFetchError(response) immediately after awaiting fetch to
let it throw if the response is an error, and then proceed to parse
response.json() and return the data; remove the manual .catch(() => ({})) and
the custom throw so all fetch errors use the centralized handler.
| {/* Local checklists (not org-scoped, work offline) */} | ||
| <Route path='/checklist/*' component={LocalChecklistView} /> | ||
| <Route path='/checklist/:checklistId' component={LocalChecklistView} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the exact route definitions in Routes.jsx
cat -n packages/web/src/Routes.jsx | head -80Repository: InfinityBowman/corates
Length of output: 4109
🏁 Script executed:
# Check LocalChecklistView implementation to understand how it uses route params
find packages/web/src -name "LocalChecklistView.jsx" -o -name "LocalChecklistView.js"Repository: InfinityBowman/corates
Length of output: 126
🏁 Script executed:
# Search for route parameter usage in the codebase
rg "useParams|useNavigate" packages/web/src/components/checklist-ui -A 2Repository: InfinityBowman/corates
Length of output: 146
🏁 Script executed:
# Examine LocalChecklistView to understand parameter usage
cat -n packages/web/src/components/checklist/LocalChecklistView.jsx | head -60Repository: InfinityBowman/corates
Length of output: 2603
🏁 Script executed:
# Search for route parameter usage in the checklist components
rg "useParams" packages/web/src/components/checklist -A 3Repository: InfinityBowman/corates
Length of output: 2834
🏁 Script executed:
# Check if there are any nested paths under /checklist that might need the wildcard
rg "navigate.*checklist" packages/web/src/components/checklist -A 2Repository: InfinityBowman/corates
Length of output: 1300
🏁 Script executed:
# Look for any route definitions or navigation patterns that might clarify intent
rg "\/checklist\/" packages/web/src -t jsx -t js | head -20Repository: InfinityBowman/corates
Length of output: 145
🏁 Script executed:
# Check if there are any sub-routes or nested paths under /checklist
rg '/checklist' packages/web/src/components/checklist -A 1 -B 1 | grep -E "(navigate|Route|path)" | head -20Repository: InfinityBowman/corates
Length of output: 514
🏁 Script executed:
# Check Solid Router documentation or examples in the codebase for route ordering
rg "Route path=" packages/web/src/Routes.jsx -A 1Repository: InfinityBowman/corates
Length of output: 1526
🏁 Script executed:
# See if the wildcard route is used elsewhere or if it's truly redundant
cat packages/web/src/Routes.jsx | grep -A 5 -B 5 "checklist"Repository: InfinityBowman/corates
Length of output: 1898
Remove the redundant /checklist/* wildcard route or reorder it after /checklist/:checklistId.
Lines 67–68 have overlapping patterns where /checklist/* (wildcard) will match /checklist/abc123 before the more specific /checklist/:checklistId route gets evaluated. This causes params.checklistId to be undefined, and LocalChecklistView shows the create form instead of loading the checklist. Navigation in the codebase uses only /checklist (base) and /checklist/${checklistId} (with ID)—no nested sub-routes requiring the wildcard. Remove line 67 or move it after line 68 if future nested routes are planned.
🤖 Prompt for AI Agents
In packages/web/src/Routes.jsx around lines 66 to 68, the wildcard route Route
path='/checklist/*' is shadowing the more specific Route
path='/checklist/:checklistId' so params.checklistId is undefined and
LocalChecklistView shows the create form; remove the redundant '/checklist/*'
route (line 67) or alternatively move it after the '/checklist/:checklistId'
route so the more specific param route matches first.
| } | ||
|
|
||
| const db = createDb(c.env.DB); | ||
| const { projectMembers } = await import('../db/schema.js'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use static import for projectMembers.
projectMembers is dynamically imported inside the middleware, but other schema entities (member, organization, projects) are already statically imported at line 7. This adds unnecessary overhead on each request.
Proposed fix
Update the static import at the top of the file:
-import { member, organization, projects } from '../db/schema.js';
+import { member, organization, projects, projectMembers } from '../db/schema.js';Then remove the dynamic import at line 109:
const db = createDb(c.env.DB);
- const { projectMembers } = await import('../db/schema.js');📝 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.
| const { projectMembers } = await import('../db/schema.js'); | |
| import { member, organization, projects, projectMembers } from '../db/schema.js'; |
| const { projectMembers } = await import('../db/schema.js'); | |
| const db = createDb(c.env.DB); |
🤖 Prompt for AI Agents
In packages/workers/src/middleware/requireOrg.js around line 7 and line 109,
projectMembers is being dynamically imported at line 109 while member,
organization, and projects are statically imported at the top; add
projectMembers to the existing static import statement at the top of the file
(the line around 7 that imports member, organization, projects) and remove the
dynamic import at line 109 so projectMembers is imported once at module load
instead of per request.
| /** | ||
| * GET /api/orgs/:orgId/projects/:projectId/invitations | ||
| * List pending invitations for a project | ||
| */ | ||
| orgInvitationRoutes.get('/', requireOrgMembership(), requireProjectAccess(), async c => { | ||
| const { projectId } = getProjectContext(c); | ||
| const db = createDb(c.env.DB); | ||
|
|
||
| try { | ||
| const invitations = await db | ||
| .select({ | ||
| id: projectInvitations.id, | ||
| email: projectInvitations.email, | ||
| role: projectInvitations.role, | ||
| orgRole: projectInvitations.orgRole, | ||
| expiresAt: projectInvitations.expiresAt, | ||
| acceptedAt: projectInvitations.acceptedAt, | ||
| createdAt: projectInvitations.createdAt, | ||
| invitedBy: projectInvitations.invitedBy, | ||
| }) | ||
| .from(projectInvitations) | ||
| .where(eq(projectInvitations.projectId, projectId)) | ||
| .orderBy(desc(projectInvitations.createdAt)); | ||
|
|
||
| return c.json(invitations); | ||
| } catch (error) { | ||
| console.error('Error listing invitations:', error); | ||
| const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { | ||
| operation: 'list_invitations', | ||
| originalError: error.message, | ||
| }); | ||
| return c.json(dbError, dbError.statusCode); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Comment says "pending" but query returns all invitations.
The route docstring states "List pending invitations" but the query fetches all invitations without filtering by acceptedAt IS NULL. Either update the comment or add a filter.
Proposed fix to filter only pending invitations
const invitations = await db
.select({
id: projectInvitations.id,
email: projectInvitations.email,
role: projectInvitations.role,
orgRole: projectInvitations.orgRole,
expiresAt: projectInvitations.expiresAt,
acceptedAt: projectInvitations.acceptedAt,
createdAt: projectInvitations.createdAt,
invitedBy: projectInvitations.invitedBy,
})
.from(projectInvitations)
- .where(eq(projectInvitations.projectId, projectId))
+ .where(
+ and(
+ eq(projectInvitations.projectId, projectId),
+ sql`${projectInvitations.acceptedAt} IS NULL`
+ )
+ )
.orderBy(desc(projectInvitations.createdAt));📝 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.
| /** | |
| * GET /api/orgs/:orgId/projects/:projectId/invitations | |
| * List pending invitations for a project | |
| */ | |
| orgInvitationRoutes.get('/', requireOrgMembership(), requireProjectAccess(), async c => { | |
| const { projectId } = getProjectContext(c); | |
| const db = createDb(c.env.DB); | |
| try { | |
| const invitations = await db | |
| .select({ | |
| id: projectInvitations.id, | |
| email: projectInvitations.email, | |
| role: projectInvitations.role, | |
| orgRole: projectInvitations.orgRole, | |
| expiresAt: projectInvitations.expiresAt, | |
| acceptedAt: projectInvitations.acceptedAt, | |
| createdAt: projectInvitations.createdAt, | |
| invitedBy: projectInvitations.invitedBy, | |
| }) | |
| .from(projectInvitations) | |
| .where(eq(projectInvitations.projectId, projectId)) | |
| .orderBy(desc(projectInvitations.createdAt)); | |
| return c.json(invitations); | |
| } catch (error) { | |
| console.error('Error listing invitations:', error); | |
| const dbError = createDomainError(SYSTEM_ERRORS.DB_ERROR, { | |
| operation: 'list_invitations', | |
| originalError: error.message, | |
| }); | |
| return c.json(dbError, dbError.statusCode); | |
| } | |
| }); | |
| const invitations = await db | |
| .select({ | |
| id: projectInvitations.id, | |
| email: projectInvitations.email, | |
| role: projectInvitations.role, | |
| orgRole: projectInvitations.orgRole, | |
| expiresAt: projectInvitations.expiresAt, | |
| acceptedAt: projectInvitations.acceptedAt, | |
| createdAt: projectInvitations.createdAt, | |
| invitedBy: projectInvitations.invitedBy, | |
| }) | |
| .from(projectInvitations) | |
| .where( | |
| and( | |
| eq(projectInvitations.projectId, projectId), | |
| isNull(projectInvitations.acceptedAt) | |
| ) | |
| ) | |
| .orderBy(desc(projectInvitations.createdAt)); |
🤖 Prompt for AI Agents
In packages/workers/src/routes/orgs/invitations.js around lines 41-74 the route
comment says "List pending invitations" but the query returns all invitations;
update the query to only return pending invitations by adding a filter for
acceptedAt IS NULL (and place it alongside the existing projectId where clause,
before orderBy), or alternatively change the comment to reflect that it
intentionally returns all invitations; ensure the chosen behavior is consistent
with any callers/tests.
| console.error( | ||
| `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`, | ||
| ); | ||
| const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { | ||
| reason: 'email_mismatch', | ||
| userEmail: currentUser.email, | ||
| invitationEmail: invitation.email, | ||
| }); | ||
| return c.json(error, error.statusCode); |
There was a problem hiding this comment.
**Avoid logging user email addresses - potential PII/compliance issue.**Email addresses are considered Personal Identifiable Information (PII) under GDPR. GDPR regulations encourage organizations not to collect or log information about users (including email addresses) unless necessary. Logging both user and invitation emails in error messages creates compliance risk. As per coding guidelines, avoid logging sensitive data like emails.
Proposed fix
if (normalizedUserEmail !== normalizedInvitationEmail) {
- console.error(
- `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`,
- );
+ console.error('[Invitation] Email mismatch for invitation:', invitation.id);
const error = createDomainError(AUTH_ERRORS.FORBIDDEN, {
reason: 'email_mismatch',
- userEmail: currentUser.email,
- invitationEmail: invitation.email,
});
return c.json(error, error.statusCode);
}📝 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.
| console.error( | |
| `[Invitation] Email mismatch: user email="${currentUser.email}", invitation email="${invitation.email}"`, | |
| ); | |
| const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { | |
| reason: 'email_mismatch', | |
| userEmail: currentUser.email, | |
| invitationEmail: invitation.email, | |
| }); | |
| return c.json(error, error.statusCode); | |
| console.error('[Invitation] Email mismatch for invitation:', invitation.id); | |
| const error = createDomainError(AUTH_ERRORS.FORBIDDEN, { | |
| reason: 'email_mismatch', | |
| }); | |
| return c.json(error, error.statusCode); |
🤖 Prompt for AI Agents
In packages/workers/src/routes/orgs/invitations.js around lines 416 to 424, the
code logs plain email addresses and returns them inside the domain error
payload; remove PII by deleting the console.error that prints user/invitation
emails and change the createDomainError payload to omit or redact emails (e.g.,
only include reason: 'email_mismatch' or a non-identifying flag). Ensure the
HTTP JSON response does not contain raw email addresses (replace with masked
values or omit entirely) and keep a generic log message like "Invitation email
mismatch" if telemetry is required.
| await db | ||
| .update(projectMembers) | ||
| .set({ role }) | ||
| .where(and(eq(projectMembers.projectId, projectId), eq(projectMembers.userId, memberId))); |
There was a problem hiding this comment.
Missing check for target member existence in PUT handler.
The PUT handler updates the member role without first verifying the member exists. If the member doesn't exist, the update will silently succeed with no rows affected.
Suggested fix
+ // Verify member exists before update
+ const memberExists = await db
+ .select({ id: projectMembers.id })
+ .from(projectMembers)
+ .where(and(eq(projectMembers.projectId, projectId), eq(projectMembers.userId, memberId)))
+ .get();
+
+ if (!memberExists) {
+ const error = createDomainError(
+ PROJECT_ERRORS.NOT_FOUND,
+ { projectId, userId: memberId },
+ 'Member not found',
+ );
+ return c.json(error, error.statusCode);
+ }
await db
.update(projectMembers)
.set({ role })
.where(and(eq(projectMembers.projectId, projectId), eq(projectMembers.userId, memberId)));🤖 Prompt for AI Agents
In packages/workers/src/routes/orgs/members.js around lines 262 to 265, the PUT
handler updates a member's role without verifying the target member exists, so
the update can silently affect 0 rows; before performing the update, query the
projectMembers table for a row matching projectId and memberId (or perform the
update with a RETURNING/affected-rows check) and if no row is found respond with
a 404 (or appropriate error) instead of proceeding, otherwise apply the role
update; ensure the handler returns the correct status/body based on existence
check or update result.
| function isCodeFile(file) { | ||
| const ext = file.substring(file.lastIndexOf('.')); | ||
| return CODE_EXTENSIONS.has(ext.toLowerCase()); | ||
| } |
There was a problem hiding this comment.
Handle files without extensions.
The isCodeFile() function doesn't handle files without extensions correctly. When lastIndexOf('.') returns -1, substring(-1) returns the entire string except the first character, which may incorrectly match or fail to match against CODE_EXTENSIONS.
Proposed fix
function isCodeFile(file) {
- const ext = file.substring(file.lastIndexOf('.'));
+ const lastDot = file.lastIndexOf('.');
+ if (lastDot === -1) {
+ return false;
+ }
+ const ext = file.substring(lastDot);
return CODE_EXTENSIONS.has(ext.toLowerCase());
}🤖 Prompt for AI Agents
In scripts/loc-report.mjs around lines 61 to 64, isCodeFile() fails for
filenames without a dot because substring(-1) returns almost the whole name;
update the function to first get the lastIndexOf('.') and if it is -1 return
false (or use path.extname and check for empty string), then extract the
extension, normalize to lowercase, and test against CODE_EXTENSIONS so files
with no extension are not treated as code files.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | d22b5eb | Commit Preview URL Branch Preview URL |
Jan 01 2026, 11:32 PM |
…lways refetch in dev
…nfinityBowman/corates into 204-improve-org-implementation
…nfinityBowman/corates into 204-improve-org-implementation
…ation 204 improve org implementation
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.