204 improve org implementation#205
Conversation
…lways refetch in dev
…nfinityBowman/corates into 204-improve-org-implementation
|
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 pull request transitions the application from organization-scoped to project-scoped routing and architecture. It removes org-context dependencies, updates API endpoints from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Notif as Notification Stream
participant RMS as RealtimeMembershipSync
participant QC as TanStack Query Client
participant Cache as Query Cache
User->>Notif: Project membership changes
Notif->>RMS: Emit membership-added/removed/updated
rect rgb(200, 220, 255)
Note over RMS,Cache: Cache Invalidation Flow
RMS->>QC: invalidateQueries(queryKeys.projects.all)
QC->>Cache: Clear all projects
RMS->>QC: invalidateQueries(queryKeys.projects.list(userId))
QC->>Cache: Clear user's project list
alt orgId present in event
RMS->>QC: invalidateQueries(queryKeys.projects.byOrg(orgId))
QC->>Cache: Clear org-scoped projects
end
end
Cache-->>User: Triggers refetch on next access
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 0ae5368 | Commit Preview URL | Jan 01 2026, 11:19 PM |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/durable-objects/UserSession.js (1)
28-31: Consider adding X-Internal-Request header verification for /notify endpoint.Per coding guidelines, internal sync endpoints should not be exposed publicly without
X-Internal-Requestverification. The/notifyendpoint accepts notification payloads and could be abused if called externally. Consider adding header verification:Proposed fix
// Handle internal notification requests (from other workers/DOs) - if (path.endsWith('/notify') && request.method === 'POST') { + if (path.endsWith('/notify') && request.method === 'POST') { + // Verify internal request + if (request.headers.get('X-Internal-Request') !== 'true') { + return new Response(JSON.stringify({ error: 'Forbidden' }), { + status: 403, + headers: { ...corsHeaders, 'Content-Type': 'application/json' }, + }); + } return await this.handleNotification(request, corsHeaders); }packages/workers/src/index.js (1)
351-362: Stale error message in legacy handler.The legacy
/api/project/:projectIdhandler still references the old org-scoped path that itself now returns 410. Update the message to point to the new project-scoped endpoint.Proposed fix
const legacyProjectDocHandler = c => c.json( { error: 'ENDPOINT_MOVED', - message: 'This endpoint has been moved. Use /api/orgs/:orgId/project-doc/:projectId instead.', + message: 'This endpoint has been moved. Use /api/project-doc/:projectId instead.', statusCode: 410, }, 410, );packages/workers/src/routes/orgs/invitations.js (2)
432-442: Avoid exposing emails in client-facing error responses.The error metadata includes both
userEmailandinvitationEmail, which are returned to the client. While useful for debugging, this could expose PII (the invitation email) to unauthorized parties. Consider limiting client-facing error details.Suggested fix
if (normalizedUserEmail !== normalizedInvitationEmail) { 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); }
494-524: Race condition: duplicate org membership possible without check inside batch.The member table has no unique constraint on (organizationId, userId). The check for existing membership (lines 495-499) runs outside the batch while the insert (lines 503-511) runs inside. Between the check and batch execution, concurrent requests could create duplicate memberships.
Move the existence check inside the batch, or add a unique constraint on (organizationId, userId) in the schema, or use an upsert pattern with
onConflictDoNothing()on the insert.
🧹 Nitpick comments (12)
packages/web/src/primitives/useProject/connection.js (1)
63-70: Consider adding projectId validation for defensive coding.The guard at line 64 validates
ydocand checksisLocalProject(), but doesn't validate thatprojectIdis a non-empty string. IfprojectIdis undefined/null, the WebsocketProvider would receive an invalid room parameter.This is likely validated upstream, but adding a guard here would make the function more robust.
Proposed defensive check
function connect() { - if (!ydoc || isLocalProject()) return; + if (!projectId || !ydoc || isLocalProject()) return;packages/web/src/primitives/useRecentsNav.js (1)
59-92: Consider optimizing regex matching strategy.The current implementation matches the path multiple times with increasingly specific patterns (project → study → checklist). Each regex match performs a full scan of the path string. A more efficient approach would be to match the most specific patterns first and fall back to less specific ones, avoiding redundant work.
Additionally, the current regex patterns don't handle optional trailing slashes (e.g.,
/projects/123/vs/projects/123), which could result in duplicate entries in recents for the same logical page.Proposed optimization
function parsePathToRecentItem(path) { + // Normalize trailing slashes + const normalizedPath = path.replace(/\/$/, ''); + - // Match project pages: /projects/:projectId - const projectMatch = path.match(/^\/projects\/([^/]+)/); - if (projectMatch) { - const [, projectId] = projectMatch; - // Check if it's a study page: /projects/:projectId/studies/:studyId - const studyMatch = path.match(/^\/projects\/([^/]+)\/studies\/([^/]+)/); - if (studyMatch) { - const [, , studyId] = studyMatch; - // Check if it's a checklist page - const checklistMatch = path.match( - /^\/projects\/([^/]+)\/studies\/([^/]+)\/checklists\/([^/]+)/, - ); - if (checklistMatch) { - return { - type: 'checklist', - path, - projectId, - studyId, - checklistId: checklistMatch[3], - }; - } - return { - type: 'study', - path, - projectId, - studyId, - }; - } - return { - type: 'project', - path, - projectId, - }; - } + // Match most specific patterns first (checklist → study → project) + let match = normalizedPath.match(/^\/projects\/([^/]+)\/studies\/([^/]+)\/checklists\/([^/]+)$/); + if (match) { + return { + type: 'checklist', + path: normalizedPath, + projectId: match[1], + studyId: match[2], + checklistId: match[3], + }; + } + + match = normalizedPath.match(/^\/projects\/([^/]+)\/studies\/([^/]+)$/); + if (match) { + return { + type: 'study', + path: normalizedPath, + projectId: match[1], + studyId: match[2], + }; + } + + match = normalizedPath.match(/^\/projects\/([^/]+)$/); + if (match) { + return { + type: 'project', + path: normalizedPath, + projectId: match[1], + }; + }Apply similar normalization to the legacy path handling section.
packages/web/src/primitives/useOrgProjectList.js (2)
47-53: Consider addingrefetchOnMount: 'always'for consistency.Based on the pattern in
packages/web/src/primitives/useMyProjectsList.js(lines 38-50), consider addingrefetchOnMount: 'always'to ensure the project list stays fresh when membership changes occur while the app is closed or when navigating back to this view.Proposed enhancement
return { queryKey: queryKeys.projects.byOrg(currentOrgId), queryFn: () => fetchOrgProjects(currentOrgId), enabled: enabledOption !== false && !!currentOrgId, staleTime: 1000 * 60 * 5, // 5 minutes gcTime: 1000 * 60 * 10, // 10 minutes + refetchOnMount: 'always', // Always refetch on mount to catch membership changes that occurred while app was closed };
59-59: LGTM! Consider adding touseMyProjectsListfor consistency.The
isInitialLoadingimplementation correctly distinguishes between initial loading (no data yet) and subsequent refetches. This is a useful addition for managing different loading UI states.For consistency, consider adding the same property to
packages/web/src/primitives/useMyProjectsList.js.packages/web/src/lib/queryClient.js (1)
196-209: Consider the implications of disabling cache in development.While disabling the cache in development (
staleTime: 0,gcTime: 0) provides predictable fresh data, it creates a behavioral difference between development and production that could hide caching-related issues:
- Cache invalidation logic won't be exercised during development
- Race conditions or stale data bugs may only surface in production
- Developers won't experience the actual user cache behavior
This is a common pattern with clear benefits (faster dev feedback, no confusion from stale data), but be aware that production-specific caching bugs may slip through.
packages/workers/src/routes/admin/users.js (1)
482-490: Minor cleanup opportunity: orgId is fetched but unused.The query fetches
orgIdfrom the joinedprojectstable, but it's no longer used after thesyncMemberToDOsignature change. Consider removing it to reduce query overhead, unless it's intentionally kept for future use or logging.Proposed cleanup
// Fetch all projects the user is a member of before any deletions (with orgId) const userProjects = await db .select({ projectId: projectMembers.projectId, - orgId: projects.orgId, }) .from(projectMembers) - .innerJoin(projects, eq(projectMembers.projectId, projects.id)) .where(eq(projectMembers.userId, userId));packages/web/src/api/better-auth-store.js (1)
217-225: Consider parallelizing the query invalidations.Both
invalidateQueriescalls are independent and could be executed in parallel usingPromise.allfor slightly better performance.Proposed refactor
- await queryClient.invalidateQueries({ - queryKey: queryKeys.projects.all, - }); - // Also invalidate legacy query key for backward compatibility - await queryClient.invalidateQueries({ - queryKey: queryKeys.projects.list(currentUser.id), - }); + await Promise.all([ + queryClient.invalidateQueries({ + queryKey: queryKeys.projects.all, + }), + // Also invalidate legacy query key for backward compatibility + queryClient.invalidateQueries({ + queryKey: queryKeys.projects.list(currentUser.id), + }), + ]);packages/workers/src/routes/avatars.js (1)
34-47: Remove unused orgId from the database query.The query still selects
orgIdfrom the database (line 38), but it's no longer used after removing it from the loop destructuring (line 45). Consider removingorgIdfrom the SELECT clause for clarity and minor performance improvement.Proposed refactor
- // Get all projects the user is a member of (with orgId for DO addressing) + // Get all projects the user is a member of const memberships = await db .select({ projectId: projectMembers.projectId, - orgId: projects.orgId, }) .from(projectMembers) - .innerJoin(projects, eq(projectMembers.projectId, projects.id)) .where(eq(projectMembers.userId, userId));Note: If
projects.orgIdis no longer needed, the join withprojectstable may also be unnecessary unless it serves as a filter (to ensure the project exists).packages/workers/src/routes/users.js (1)
134-167: New/me/projectsendpoint duplicates/:userId/projectslogic.This endpoint provides the same functionality as
/:userId/projectswhen called with the current user's ID. Consider extracting the shared query logic into a helper function to reduce duplication.Suggested refactor to reduce duplication
// Helper function for fetching user projects async function fetchUserProjects(db, userId) { return db .select({ id: projects.id, name: projects.name, description: projects.description, orgId: projects.orgId, role: projectMembers.role, createdAt: projects.createdAt, updatedAt: projects.updatedAt, }) .from(projects) .innerJoin(projectMembers, eq(projects.id, projectMembers.projectId)) .where(eq(projectMembers.userId, userId)) .orderBy(desc(projects.updatedAt)); } // Then use in both endpoints: // /me/projects const results = await fetchUserProjects(db, authUser.id); // /:userId/projects const results = await fetchUserProjects(db, userId);packages/web/src/primitives/useProjectOrgId.js (1)
19-41: Consider accepting a getter function for defensive reactivity.The hook currently accepts
projectIdas a direct parameter, but if it's ever passed a reactive accessor (vs. a static string), changes won't be tracked—SolidJS memos only track reactive primitives accessed during execution. While all current callers pass values at component body level (which re-execute appropriately when route params/props change), accepting a getter function would make the hook more robust:-export function useProjectOrgId(projectId) { +export function useProjectOrgId(projectIdAccessor) { const queryClient = useQueryClient(); return createMemo(() => { + const projectId = typeof projectIdAccessor === 'function' ? projectIdAccessor() : projectIdAccessor; if (!projectId) return null;Alternatively, document that
projectIdmust be a static value, not a reactive accessor.packages/web/src/primitives/useMyProjectsList.js (1)
53-61: Consider separating background refetch state from initial loading.The current
isLoadingincludesisFetching, which will be true during background refetches even when stale data is available. This may cause the UI to show loading spinners during silent background refreshes.If consumers need to distinguish between initial load and background refresh, consider:
Possible refinement
return { projects: () => query.data || [], - isLoading: () => query.isLoading || query.isFetching, + isLoading: () => query.isLoading, + isFetching: () => query.isFetching, isInitialLoading: () => query.isLoading && !query.data, isError: () => query.isError, error: () => query.error, refetch: () => query.refetch(), query, };This would let consumers show a subtle refresh indicator (via
isFetching) without replacing content with a loading skeleton.packages/web/src/components/project/ProjectsPanel.jsx (1)
107-125: UsehandleFetchErrorfor fetch error handling.Per coding guidelines, frontend fetch calls should use
handleFetchErrorfrom@/lib/error-utils.jsinstead of a dynamic import ofhandleError. This provides consistent error handling with optional toast display.Suggested refactor
Add the import at the top of the file:
import { handleFetchError } from '@/lib/error-utils.js';Then update the error handling:
try { const response = await fetch( `${API_BASE}/api/orgs/${project.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.all }); 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' }); + } catch (err) { + showToast.error('Delete Failed', err.message || 'An unexpected error occurred'); }
| createEffect(() => { | ||
| const pid = params.projectId; | ||
| const oid = orgId(); | ||
| if (pid && oid) { | ||
| projectActionsStore._setActiveProject(pid, oid); | ||
| if (pid) { | ||
| if (oid) { | ||
| projectActionsStore._setActiveProject(pid, oid); | ||
| } | ||
| connect(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine ProjectView.jsx to understand the context
find . -name "ProjectView.jsx" -type f | head -5Repository: InfinityBowman/corates
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Search for useProjectOrgId hook definition
rg -l "useProjectOrgId" --type=js --type=jsx --type=ts --type=tsxRepository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Find projectActionsStore and understand setActiveProject
rg -l "projectActionsStore" --type=js --type=jsx --type=ts --type=tsx | head -10Repository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Search for the connect function and how it's used
rg -n "const connect|function connect|export.*connect" packages/web/src --type=js --type=jsx --type=ts --type=tsx -A 5Repository: InfinityBowman/corates
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Read ProjectView.jsx directly to see the context
cat -n ./packages/web/src/components/project/ProjectView.jsx | head -100Repository: InfinityBowman/corates
Length of output: 4737
🏁 Script executed:
#!/bin/bash
# Search for useProjectOrgId definition without strict type filtering
rg "export.*useProjectOrgId|const useProjectOrgId|function useProjectOrgId" -A 20Repository: InfinityBowman/corates
Length of output: 1760
🏁 Script executed:
#!/bin/bash
# Search for projectActionsStore definition and _setActiveProject
rg "projectActionsStore" -A 5 -B 5 | head -150Repository: InfinityBowman/corates
Length of output: 12521
🏁 Script executed:
#!/bin/bash
# Find connect function that's used in ProjectView
rg "connect()" -B 5 -A 5 packages/web/src | head -100Repository: InfinityBowman/corates
Length of output: 6920
🏁 Script executed:
#!/bin/bash
# Find projectActionsStore implementation to understand how _setActiveProject and _setConnection work
cat -n packages/web/src/stores/projectActionsStore/index.js | head -200Repository: InfinityBowman/corates
Length of output: 6825
🏁 Script executed:
#!/bin/bash
# Look for how getActiveConnection is implemented
rg -n "getActiveConnection|_setActiveProject|_setConnection" packages/web/src/stores/projectActionsStore/index.js -A 3Repository: InfinityBowman/corates
Length of output: 1314
🏁 Script executed:
#!/bin/bash
# Check if there are operations that don't require activeProject to be set
rg -n "getActiveConnection\(\)" packages/web/src/stores/projectActionsStore -A 10 | head -80Repository: InfinityBowman/corates
Length of output: 6611
🏁 Script executed:
#!/bin/bash
# Check useProjectOrgId more thoroughly to understand if it's async
cat -n packages/web/src/primitives/useProjectOrgId.jsRepository: InfinityBowman/corates
Length of output: 1559
🏁 Script executed:
#!/bin/bash
# Check if connect() is async and what it does
rg -n "export.*connect|const connect" packages/web/src/primitives/useProject/index.js -A 20 | head -100Repository: InfinityBowman/corates
Length of output: 2013
🏁 Script executed:
#!/bin/bash
# Understand when _setConnection is called during connect()
rg -n "_setConnection" packages/web/src/primitives/useProject/index.js -B 5 -A 5Repository: InfinityBowman/corates
Length of output: 744
🏁 Script executed:
#!/bin/bash
# Check if there are any immediate operations attempted on ProjectView mount
rg -n "projectActionsStore\.(study|pdf|project|checklist|member|reconciliation)\." packages/web/src/components/project/ProjectView.jsx -B 2 -A 2Repository: InfinityBowman/corates
Length of output: 1654
🏁 Script executed:
#!/bin/bash
# Look at the pending effects more carefully to see what they depend on
sed -n '88,160p' packages/web/src/components/project/ProjectView.jsxRepository: InfinityBowman/corates
Length of output: 3051
🏁 Script executed:
#!/bin/bash
# Check what getActiveProjectId error message says and if it's thrown
rg -n "getActiveProjectId()" packages/web/src/stores/projectActionsStore/index.js -B 2 -A 8Repository: InfinityBowman/corates
Length of output: 2172
🏁 Script executed:
#!/bin/bash
# Verify if operations in ProjectView actually call getActiveProjectId before orgId is set
sed -n '105,200p' packages/web/src/components/project/ProjectView.jsxRepository: InfinityBowman/corates
Length of output: 3805
🏁 Script executed:
#!/bin/bash
# Check if createStudyActions uses getActiveOrgId and what happens if it's null
rg -n "getActiveOrgId" packages/web/src/stores/projectActionsStore/studies.js -B 2 -A 5Repository: InfinityBowman/corates
Length of output: 1394
🏁 Script executed:
#!/bin/bash
# Check the exact flow - what does study.create do when getActiveOrgId is called
sed -n '230,270p' packages/web/src/stores/projectActionsStore/studies.jsRepository: InfinityBowman/corates
Length of output: 1263
Set active project regardless of orgId availability.
The project connection is established via connect() (line 63) before _setActiveProject is called (line 61), which is gated on orgId availability (line 60). Since useProjectOrgId is a synchronous memo that may return null if orgId is not yet synced from Y.js or available in the query cache, the activeProjectId remains null if orgId fails to load.
Operations that depend on activeProjectId will throw "No active project - are you inside a ProjectView?" when invoked:
- User-triggered operations like
projectActionsStore.project.rename()andprojectActionsStore.project.updateDescription()(lines 281-282) have no guards - These call
getActiveConnection()→getActiveProjectId()which throws ifactiveProjectIdisnull
Since the WebSocket connection is project-scoped and doesn't require orgId (as noted in the comment on line 46), call _setActiveProject(pid, oid) unconditionally when pid exists, even if oid is null. This ensures activeProjectId is set and operations don't fail with a misleading error message.
🤖 Prompt for AI Agents
In packages/web/src/components/project/ProjectView.jsx around lines 56 to 65,
currently _setActiveProject(pid, oid) is only called when oid is truthy which
can leave activeProjectId null; change the logic to call
projectActionsStore._setActiveProject(pid, oid) unconditionally whenever pid
exists (even if oid is null) and then call connect(); remove or bypass the oid
guard so the active project is always set before or alongside establishing the
WebSocket connection.
| /** | ||
| * Internal component that handles notifications for a specific userId | ||
| * This is recreated when userId changes, ensuring the hook uses the correct userId | ||
| */ | ||
| function MembershipSyncForUser(props) { | ||
| // Create notification handler that invalidates queries | ||
| const handleNotification = notification => { | ||
| const notificationType = notification.type; | ||
|
|
||
| // Handle project membership change events | ||
| if ( | ||
| notificationType === 'project-membership-added' || | ||
| notificationType === 'project-membership-removed' || | ||
| notificationType === 'project-membership-updated' | ||
| ) { | ||
| // Invalidate all project list queries | ||
| queryClient.invalidateQueries({ queryKey: queryKeys.projects.all }); | ||
|
|
||
| // Also invalidate legacy query keys for backward compatibility | ||
| if (props.userId) { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: queryKeys.projects.list(props.userId), | ||
| }); | ||
| } | ||
|
|
||
| // If orgId is provided, also invalidate org-scoped project list | ||
| if (notification.orgId) { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: queryKeys.projects.byOrg(notification.orgId), | ||
| }); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const notifications = useNotifications(props.userId, { | ||
| onNotification: handleNotification, | ||
| }); | ||
|
|
||
| // Connect when component mounts | ||
| createEffect(() => { | ||
| if (props.userId) { | ||
| notifications.connect(); | ||
| } | ||
| }); | ||
|
|
||
| // Cleanup on unmount | ||
| onCleanup(() => { | ||
| notifications.disconnect(); | ||
| }); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the useNotifications hook
fd -t f "useNotifications" --extension js --extension jsx --extension ts --extension tsxRepository: InfinityBowman/corates
Length of output: 113
🏁 Script executed:
# Examine the RealtimeMembershipSync component to see the full context
fd -t f "RealtimeMembershipSync" --extension jsx --extension jsRepository: InfinityBowman/corates
Length of output: 120
🏁 Script executed:
# Search for useNotifications hook definition
rg "function useNotifications|const useNotifications|export.*useNotifications" -A 5Repository: InfinityBowman/corates
Length of output: 662
🏁 Script executed:
cat -n packages/web/src/primitives/useNotifications.jsRepository: InfinityBowman/corates
Length of output: 5928
🏁 Script executed:
cat -n packages/web/src/components/RealtimeMembershipSync.jsxRepository: InfinityBowman/corates
Length of output: 2932
Component will reuse stale WebSocket connection when a different user logs in.
The issue is critical and confirmed: When a user logs out and a different user logs in, the MembershipSyncForUser component updates its props but does not remount. Since useNotifications captures userId at initialization (line 46) and the hook doesn't support reactive userId updates, it never reconnects to the new user's WebSocket endpoint. The existing connection persists with the old userId, causing membership event cache invalidations to go to the wrong user's cache.
The Show component with callback pattern (lines 71-72) re-evaluates when the condition changes but does not remount the child component when the value changes from one truthy userId to another. The key={userId()} attribute on line 72 has no effect in SolidJS—it's just a regular prop and doesn't trigger remounting.
To fix this, use Show's keyed prop to force remounting:
Recommended fix using Show keyed prop
export default function RealtimeMembershipSync() {
const { user, isLoggedIn } = useBetterAuth();
return (
- <Show when={isLoggedIn() && user()?.id}>
+ <Show when={isLoggedIn() && user()?.id} keyed>
- {userId => <MembershipSyncForUser userId={userId()} key={userId()} />}
+ {userId => <MembershipSyncForUser userId={userId} />}
</Show>
);
}The keyed prop forces Show to remount its children whenever the value changes, ensuring MembershipSyncForUser reinitializes with the correct userId and creates a fresh WebSocket connection.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/components/RealtimeMembershipSync.jsx around lines 12 to 63,
MembershipSyncForUser can reuse a stale WebSocket when a different user logs in
because the parent Show wrapper does not remount the child on userId change;
update the parent Show to use the keyed prop (keyed={userId()}) instead of
relying on a regular prop so the child is remounted whenever userId changes,
ensuring useNotifications is reinitialized and a fresh connection is created for
the new user.
| // This ensures useNotifications is called with the correct userId | ||
| return ( | ||
| <Show when={isLoggedIn() && user()?.id}> | ||
| {userId => <MembershipSyncForUser userId={userId()} key={userId()} />} |
There was a problem hiding this comment.
Remove ineffective key attribute.
SolidJS doesn't use the key attribute for forcing component remounting like React does. The key attribute has no special meaning here and should be removed.
Proposed fix
- {userId => <MembershipSyncForUser userId={userId()} key={userId()} />}
+ {userId => <MembershipSyncForUser userId={userId()} />}If you need to force remounting when userId changes, use the keyed prop on the Show component instead (see previous comment).
🤖 Prompt for AI Agents
In packages/web/src/components/RealtimeMembershipSync.jsx around line 72, the
JSX is passing a React-style key prop to a SolidJS component: "{userId =>
<MembershipSyncForUser userId={userId()} key={userId()} />}", which is
ineffective in Solid and should be removed; delete the key attribute from the
MembershipSyncForUser element, and if you need to force remount when userId
changes use Solid's keyed behavior (e.g., use keyed prop on Show or a keyed
list) instead of a key attribute.
…nfinityBowman/corates into 204-improve-org-implementation
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.