Refactor to use React Query mutations #875
Conversation
WalkthroughReplace local try/catch and Effect.fn flows with React Query mutations across notifications, caps, sharing, custom domain, and login; comment out lifecycle hook wiring in effect-react-query. UI loading/disabled states now derive from mutation.isPending; many onSuccess/onError callbacks converted to plain functions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Component
participant RQ as React Query
participant API as Server/API
participant UX as Router/Toast
User->>UI: trigger action (download/delete/mark/share/etc.)
UI->>RQ: mutate(variables)
RQ->>API: call mutationFn(variables)
API-->>RQ: result / error
RQ-->>UI: update isPending / isSuccess / isError
UI->>UX: invalidate/refresh queries, show toast, update UI state
sequenceDiagram
actor User
participant Dialog as CustomDomainDialog
participant Update as updateDomainMutation
participant Check as checkDomainMutation
participant API as Domain API
participant UX as Router/Toast
User->>Dialog: Submit domain
Dialog->>Update: mutate({ domain, orgId })
Update->>API: remove existing domain? + update
API-->>Update: response
Update-->>Dialog: success -> handleNext + toast + refresh
User->>Dialog: Check verification
Dialog->>Check: mutate({ orgId, showToasts })
Check->>API: verify domain
API-->>Check: { verified, config }
Check-->>Dialog: setIsVerified + setDomainConfig + optional toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
258-267: Align spaceId fallback strategy across drop handlersThe mobile drop target handler uses a fallback for spaceId (spaceId ?? activeOrganization?.organization.id), but the desktop drop handler does not. This inconsistency can cause moveVideoToFolder to receive undefined spaceId in one path and a valid fallback in the other.
Consider aligning the desktop drop to use the same fallback:
- await moveVideoToFolder({ videoId: capData.id, folderId: id, spaceId }); + await moveVideoToFolder({ + videoId: capData.id, + folderId: id, + spaceId: spaceId ?? activeOrganization?.organization.id, + });apps/web/lib/effect-react-query.ts (1)
268-268: Remove debug console.log statementDebug logging should be removed from production code.
- console.log({ result });
♻️ Duplicate comments (1)
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (1)
90-93: Inconsistent callback type with effect-react-query expectationsSimilar to the issue in Caps.tsx, the
onSuccesscallback is now a plain function but the type system expects an Effect-wrapped function.This needs to be wrapped in Effect or the type definitions need to be updated to accept plain functions.
🧹 Nitpick comments (3)
apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
252-256: Comment contradicts behavior ("Keep the folder open" vs playing "folder-close")The comment says to keep the folder open, but the code plays the "folder-close" animation. Update the comment or animation to match intent.
apps/web/app/(org)/login/form.tsx (1)
58-63: Avoid relying on outer email state inside onSuccess; use mutation variablesUsing the variables parameter ensures the tracked domain matches the email submitted for this mutation, even if state changes between submit and resolution.
- onSuccess: () => { - trackEvent("auth_email_sent", { - email_domain: email.split("@")[1], - }); + onSuccess: (_result, submittedEmail) => { + const domain = submittedEmail?.includes("@") + ? submittedEmail.split("@")[1] + : undefined; + trackEvent("auth_email_sent", { + ...(domain ? { email_domain: domain } : {}), + }); toast.success("Email sent - check your inbox!"); },apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
124-134: Close dialog on settle is good; consider adding a success toastThe onSettled handler reliably closes the dialog. Consider adding an onSuccess toast to confirm the deletion to users (optional).
const deleteMutation = useMutation({ mutationFn: async () => { await onDelete?.(); }, onError: (error) => { console.error("Error deleting cap:", error); }, + onSuccess: () => { + toast.success("Cap deleted"); + }, onSettled: () => { setConfirmOpen(false); }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/Caps.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(7 hunks)apps/web/app/(org)/dashboard/caps/components/Folder.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/components/SelectedCapsBar.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx(8 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx(4 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx(7 hunks)apps/web/app/(org)/login/form.tsx(5 hunks)apps/web/lib/effect-react-query.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (1)
apps/web/actions/organization/remove-domain.ts (1)
removeOrganizationDomain(9-59)
apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx (1)
apps/web/actions/notifications/mark-as-read.ts (1)
markAsRead(9-34)
apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
apps/web/lib/EffectRuntime.ts (1)
useEffectMutation(20-20)apps/web/lib/Rpcs.ts (1)
withRpc(16-17)
apps/web/app/(org)/login/form.tsx (1)
packages/database/auth/auth-options.tsx (1)
signIn(107-175)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
apps/web/actions/caps/share.ts (1)
shareCap(16-141)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (3)
apps/web/actions/videos/download.ts (1)
downloadVideo(9-50)apps/web/lib/EffectRuntime.ts (1)
useEffectMutation(20-20)apps/web/lib/Rpcs.ts (1)
withRpc(16-17)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (3)
apps/web/actions/organization/remove-domain.ts (1)
removeOrganizationDomain(9-59)apps/web/actions/organization/update-domain.ts (1)
updateDomain(10-78)apps/web/actions/organization/check-domain.ts (1)
checkOrganizationDomain(9-55)
⏰ 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). (2)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (15)
apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
77-87: Delete mutation: behavior and callbacks look correctThe mutation uses a clear success/error flow, refreshes the router, provides user feedback, and closes the dialog on success. Looks good.
apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx (1)
10-22: Good migration to a mutation with query invalidationEncapsulating the mark-as-read action in a mutation and invalidating ["notifications"] is aligned with the PR objective and keeps UI consistent. Nice.
apps/web/app/(org)/login/form.tsx (2)
39-67: Solid mutation encapsulation for email sign-inGood use of useMutation to centralize the flow, wire analytics, and handle toasts. onSuccess/onError are concise and effective.
277-287: Form submission wiring looks correctThe button derives disabled/loading from mutation state, and reset uses mutation.reset() to clear state. This keeps UI responsive and consistent.
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (4)
107-123: Download flow is clean and user-friendlyGood use of a mutation and ephemeral anchor to trigger downloads. Combined with toast.promise in the caller, this provides clear feedback.
219-232: Nice: toast-wrapped download with pending guardEarly-return while pending avoids duplicate requests; toast.promise surfaces errors cleanly.
336-345: Good UX for duplicate action stateDisabling while pending and inline label swap to “Duplicating...” improves clarity.
384-386: ConfirmationDialog wired correctly to mutation stateTying loading to deleteMutation.isPending and confirming via mutate keeps the flow consistent with other components.
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (2)
149-160: Dialog initialization logic is clear and comprehensiveCleanly resets selected spaces, public state, search term, and tab when opened or props change. Looks good.
351-364: Save button state wired to mutation pendingSpinner/disabled tied to updateSharing.isPending is correct and prevents duplicate submissions.
apps/web/app/(org)/dashboard/caps/components/SelectedCapsBar.tsx (1)
70-70: Simplified onClick handler - good refactor!Removing the intermediate
handleDeleteClickwrapper and directly opening the confirmation dialog is a cleaner approach.apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (2)
29-41: Well-structured mutation implementationThe mutation setup properly handles success and error states with appropriate UI feedback and state management. Good use of React Query's mutation pattern.
43-52: Good defensive programmingThe function properly checks for subscription status before proceeding with the mutation, and includes a guard clause for the organization ID.
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (2)
155-187: Good separation of concerns with conditional toastsThe
showToastsparameter allows the mutation to be used both for automatic polling (without spamming toasts) and manual checks (with user feedback). This is a thoughtful design.
285-296: Clean refactor to mutation-based approachThe verification check is now properly handled through a mutation with appropriate loading states.
apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
Outdated
Show resolved
Hide resolved
| const updateDomainMutation = useMutation({ | ||
| mutationFn: async ({ domain, orgId }: { domain: string; orgId: string }) => { | ||
| if (activeOrganization?.organization.customDomain) { | ||
| await removeOrganizationDomain(orgId); | ||
| } | ||
| return await updateDomain(domain, orgId); | ||
| }, | ||
| onSuccess: (data) => { | ||
| handleNext(); | ||
| toast.success("Domain settings updated"); | ||
| router.refresh(); | ||
| if (data) { | ||
| setIsVerified(data.verified); | ||
| } | ||
| }, | ||
| onError: (error) => { | ||
| toast.error( | ||
| error instanceof Error ? error.message : "Failed to update domain settings" | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sequential domain operations may cause issues
The mutation performs removeOrganizationDomain followed by updateDomain sequentially. If updateDomain fails after successful removal, the user loses their domain configuration without a replacement.
Consider making this operation atomic or adding rollback capability:
- mutationFn: async ({ domain, orgId }: { domain: string; orgId: string }) => {
- if (activeOrganization?.organization.customDomain) {
- await removeOrganizationDomain(orgId);
- }
- return await updateDomain(domain, orgId);
- },
+ mutationFn: async ({ domain, orgId }: { domain: string; orgId: string }) => {
+ // updateDomain should handle the removal internally in a transaction
+ // to ensure atomicity
+ return await updateDomain(domain, orgId);
+ },Alternatively, implement proper error recovery:
mutationFn: async ({ domain, orgId }: { domain: string; orgId: string }) => {
+ const previousDomain = activeOrganization?.organization.customDomain;
if (activeOrganization?.organization.customDomain) {
- await removeOrganizationDomain(orgId);
+ try {
+ await removeOrganizationDomain(orgId);
+ } catch (error) {
+ throw new Error(`Failed to remove existing domain: ${error.message}`);
+ }
+ }
+ try {
+ return await updateDomain(domain, orgId);
+ } catch (error) {
+ // Attempt to restore previous domain if update fails
+ if (previousDomain) {
+ await updateDomain(previousDomain, orgId);
+ }
+ throw error;
}
- return await updateDomain(domain, orgId);
},📝 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 updateDomainMutation = useMutation({ | |
| mutationFn: async ({ domain, orgId }: { domain: string; orgId: string }) => { | |
| if (activeOrganization?.organization.customDomain) { | |
| await removeOrganizationDomain(orgId); | |
| } | |
| return await updateDomain(domain, orgId); | |
| }, | |
| onSuccess: (data) => { | |
| handleNext(); | |
| toast.success("Domain settings updated"); | |
| router.refresh(); | |
| if (data) { | |
| setIsVerified(data.verified); | |
| } | |
| }, | |
| onError: (error) => { | |
| toast.error( | |
| error instanceof Error ? error.message : "Failed to update domain settings" | |
| ); | |
| } | |
| }); | |
| const updateDomainMutation = useMutation({ | |
| mutationFn: async ({ domain, orgId }: { domain: string; orgId: string }) => { | |
| const previousDomain = activeOrganization?.organization.customDomain; | |
| if (activeOrganization?.organization.customDomain) { | |
| try { | |
| await removeOrganizationDomain(orgId); | |
| } catch (error) { | |
| throw new Error( | |
| `Failed to remove existing domain: ${error instanceof Error ? error.message : error}` | |
| ); | |
| } | |
| } | |
| try { | |
| return await updateDomain(domain, orgId); | |
| } catch (error) { | |
| // Attempt to restore previous domain if update fails | |
| if (previousDomain) { | |
| await updateDomain(previousDomain, orgId); | |
| } | |
| throw error; | |
| } | |
| }, | |
| onSuccess: (data) => { | |
| handleNext(); | |
| toast.success("Domain settings updated"); | |
| router.refresh(); | |
| if (data) { | |
| setIsVerified(data.verified); | |
| } | |
| }, | |
| onError: (error) => { | |
| toast.error( | |
| error instanceof Error ? error.message : "Failed to update domain settings" | |
| ); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx
around lines 132–152, the mutation removes the existing domain then calls
updateDomain, which can leave the org without a domain if updateDomain fails;
either 1) change the flow to use/to call a single backend endpoint that
atomically swaps domains (preferred), or 2) implement client-side rollback:
capture the current domain before removing, perform updateDomain first (or
create the new domain) and only remove the old on success, or if you must remove
first then on updateDomain failure call updateDomain(oldDomain, orgId) to
restore the previous value and surface a clear error; ensure to handle and
surface errors from the rollback attempt and update UI state (isVerified) only
on confirmed success.
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
80-143: Avoid stale state in onSuccess: use mutation variables to compute diffs and toastsonSuccess closes over selectedSpaces/publicToggle at callback execution time, which can drift from the values sent to the server if the user toggles while the request is in flight. Use the variables argument from React Query to compute toasts and propagate updates, ensuring consistency with the actually-saved payload.
Apply:
- const updateSharing = useMutation({ - mutationFn: async ({ + const updateSharing = useMutation< + void, + Error, + { capId: string; spaceIds: string[]; public: boolean } + >({ + mutationFn: async ({ capId, spaceIds, public: isPublic, - }: { - capId: string; - spaceIds: string[]; - public: boolean; - }) => { + }) => { const result = await shareCap({ capId, spaceIds, public: isPublic }); if (!result.success) { throw new Error(result.error || "Failed to update sharing settings"); } }, - onSuccess: () => { - const newSelectedSpaces = Array.from(selectedSpaces); + onSuccess: (_data, variables) => { + const newSelectedSpaces = variables.spaceIds; const initialSpaces = Array.from(initialSelectedSpaces); const addedSpaceIds = newSelectedSpaces.filter( (id) => !initialSpaces.includes(id) ); const removedSpaceIds = initialSpaces.filter( (id) => !newSelectedSpaces.includes(id) ); - const publicChanged = publicToggle !== initialPublicState; + const publicChanged = variables.public !== initialPublicState; const getSpaceName = (id: string) => { const space = spacesData?.find((space) => space.id === id); return space?.name || `Space ${id}`; }; if ( publicChanged && addedSpaceIds.length === 0 && removedSpaceIds.length === 0 ) { toast.success( - publicToggle ? "Video is now public" : "Video is now private" + variables.public ? "Video is now public" : "Video is now private" ); } else if ( addedSpaceIds.length === 1 && removedSpaceIds.length === 0 && !publicChanged ) { toast.success(`Shared to ${getSpaceName(addedSpaceIds[0] as string)}`); } else if ( removedSpaceIds.length === 1 && addedSpaceIds.length === 0 && !publicChanged ) { toast.success( `Unshared from ${getSpaceName(removedSpaceIds[0] as string)}` ); } else if ( addedSpaceIds.length > 0 && removedSpaceIds.length === 0 && !publicChanged ) { toast.success(`Shared to ${addedSpaceIds.length} spaces`); } else if ( removedSpaceIds.length > 0 && addedSpaceIds.length === 0 && !publicChanged ) { toast.success(`Unshared from ${removedSpaceIds.length} spaces`); } else if ( addedSpaceIds.length > 0 || removedSpaceIds.length > 0 || publicChanged ) { toast.success(`Sharing settings updated`); } else { toast.info("No changes to sharing settings"); } - onSharingUpdated(newSelectedSpaces); + onSharingUpdated(newSelectedSpaces); onClose(); },
♻️ Duplicate comments (1)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
64-79: Previous feedback addressed: error handling is now in mutationFn; onSuccess no longer throwsThis resolves the earlier review direction. The mutation rejects on failure and keeps onSuccess side-effect-only. Good use of isPending for UI state.
Also applies to: 80-147
🧹 Nitpick comments (4)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (4)
80-143: Prefer cache invalidation over manual parent callbacks to align with React QueryGiven the PR objective to centralize state with React Query, consider replacing onSharingUpdated with query invalidations (e.g., invalidate “caps” and “cap by id” queries) so the UI derives from the cache instead of bespoke callbacks.
If helpful, I can propose a concrete invalidateQueries snippet once you confirm the query keys used for caps and cap detail.
280-283: Disable the public switch while savingAvoids public toggle changes mid-request which could desync the success toasts and updated state.
<Switch checked={publicToggle} - onCheckedChange={setPublicToggle} + onCheckedChange={setPublicToggle} + disabled={updateSharing.isPending} />
450-453: Conflicting background classes override the green check indicatorbg-green-500 and bg-gray-4 are both applied; the latter overrides the green. Remove the gray background to keep the check badge green.
- className="flex absolute -top-2 -right-2 z-10 justify-center items-center bg-green-500 rounded-full bg-gray-4 size-4" + className="flex absolute -top-2 -right-2 z-10 justify-center items-center bg-green-500 rounded-full size-4"
236-245: Don’t attach onClick handler to the active tabThe UI shows a not-allowed cursor for the active tab but still wires onClick. No-op clicks improve semantics and avoid unnecessary state updates.
- <div + <div key={tab} className={clsx( "flex relative flex-1 justify-center items-center w/full min-w-0 text-sm font-medium transition-colors", activeTab === tab ? "cursor-not-allowed bg-gray-3" : "cursor-pointer" )} - onClick={() => setActiveTab(tab)} + onClick={activeTab === tab ? undefined : () => setActiveTab(tab)} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
apps/web/actions/caps/share.ts (1)
shareCap(16-141)
| import { motion } from "framer-motion"; | ||
| import { Check, Search, Globe2 } from "lucide-react"; | ||
| import Image from "next/image"; | ||
| import { useCallback, useEffect, useState } from "react"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import useMemo to support organization-level memoization
Needed by the selectedOrgIds computation.
-import { useCallback, useEffect, useState } from "react";
+import { useCallback, useEffect, useMemo, useState } from "react";📝 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.
| import { useCallback, useEffect, useState } from "react"; | |
| import { useCallback, useEffect, useMemo, useState } from "react"; |
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx around line
19, the code uses a memoized computation for selectedOrgIds but did not import
useMemo; add useMemo to the React import list (import { useCallback, useEffect,
useState, useMemo } from "react") and wrap the selectedOrgIds computation with
useMemo so the list is only recalculated when its dependencies change.
| const sharedSpaceIds = new Set(sharedSpaces?.map((space) => space.id) || []); | ||
|
|
||
| useEffect(() => { | ||
| if (isOpen && sharedSpaces) { | ||
| const spaceIds = new Set(sharedSpaces.map((space) => space.id)); | ||
| setSelectedSpaces(spaceIds); | ||
| setInitialSelectedSpaces(spaceIds); | ||
| setPublicToggle(isPublic); | ||
| setInitialPublicState(isPublic); | ||
| setSearchTerm(""); | ||
| setActiveTab(tabs[0]); | ||
| } | ||
| }, [isOpen, sharedSpaces, isPublic]); | ||
|
|
||
| const isSpaceSharedViaOrganization = useCallback( | ||
| (spaceId: string) => { | ||
| const space = spacesData?.find((s) => s.id === spaceId); | ||
| if (!space) return false; | ||
| return sharedSpaceIds.has(space.id); | ||
| }, | ||
| [spacesData, sharedSpaceIds] | ||
| ); |
There was a problem hiding this comment.
Incorrect ‘shared via organization’ detection and stale basis: use selected org entries, not initial sharedSpaces
The current logic returns sharedSpaceIds.has(space.id), which simply marks directly shared spaces, not spaces implicitly shared via an organization. It also never updates when the user toggles org entries during this dialog (it’s based on props, not current selection).
Refactor to:
- Derive the set of selected organization IDs from selectedSpaces.
- Consider a space “shared via organization” if its organizationId is in that set and it is not itself the org entry.
Apply:
- const sharedSpaceIds = new Set(sharedSpaces?.map((space) => space.id) || []);
+ // Track which organizations are selected (org entry is a space where id === organizationId and primary === true)
+ const selectedOrgIds = useMemo(() => {
+ const orgs = new Set<string>();
+ if (!spacesData) return orgs;
+ for (const id of selectedSpaces) {
+ const s = spacesData.find((sp) => sp.id === id);
+ if (s && s.id === s.organizationId && s.primary === true) {
+ orgs.add(s.organizationId);
+ }
+ }
+ return orgs;
+ }, [selectedSpaces, spacesData]);
@@
- const isSpaceSharedViaOrganization = useCallback(
- (spaceId: string) => {
- const space = spacesData?.find((s) => s.id === spaceId);
- if (!space) return false;
- return sharedSpaceIds.has(space.id);
- },
- [spacesData, sharedSpaceIds]
- );
+ const isSpaceSharedViaOrganization = useCallback(
+ (spaceId: string) => {
+ const space = spacesData?.find((s) => s.id === spaceId);
+ if (!space) return false;
+ return selectedOrgIds.has(space.organizationId) && space.id !== space.organizationId;
+ },
+ [spacesData, selectedOrgIds]
+ );Note: This requires importing useMemo (see separate import change).
📝 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 sharedSpaceIds = new Set(sharedSpaces?.map((space) => space.id) || []); | |
| useEffect(() => { | |
| if (isOpen && sharedSpaces) { | |
| const spaceIds = new Set(sharedSpaces.map((space) => space.id)); | |
| setSelectedSpaces(spaceIds); | |
| setInitialSelectedSpaces(spaceIds); | |
| setPublicToggle(isPublic); | |
| setInitialPublicState(isPublic); | |
| setSearchTerm(""); | |
| setActiveTab(tabs[0]); | |
| } | |
| }, [isOpen, sharedSpaces, isPublic]); | |
| const isSpaceSharedViaOrganization = useCallback( | |
| (spaceId: string) => { | |
| const space = spacesData?.find((s) => s.id === spaceId); | |
| if (!space) return false; | |
| return sharedSpaceIds.has(space.id); | |
| }, | |
| [spacesData, sharedSpaceIds] | |
| ); | |
| // Track which organizations are selected (org entry is a space where id === organizationId and primary === true) | |
| const selectedOrgIds = useMemo(() => { | |
| const orgs = new Set<string>(); | |
| if (!spacesData) return orgs; | |
| for (const id of selectedSpaces) { | |
| const s = spacesData.find((sp) => sp.id === id); | |
| if (s && s.id === s.organizationId && s.primary === true) { | |
| orgs.add(s.organizationId); | |
| } | |
| } | |
| return orgs; | |
| }, [selectedSpaces, spacesData]); | |
| useEffect(() => { | |
| if (isOpen && sharedSpaces) { | |
| const spaceIds = new Set(sharedSpaces.map((space) => space.id)); | |
| setSelectedSpaces(spaceIds); | |
| setInitialSelectedSpaces(spaceIds); | |
| setPublicToggle(isPublic); | |
| setInitialPublicState(isPublic); | |
| setSearchTerm(""); | |
| setActiveTab(tabs[0]); | |
| } | |
| }, [isOpen, sharedSpaces, isPublic]); | |
| const isSpaceSharedViaOrganization = useCallback( | |
| (spaceId: string) => { | |
| const space = spacesData?.find((s) => s.id === spaceId); | |
| if (!space) return false; | |
| return selectedOrgIds.has(space.organizationId) && space.id !== space.organizationId; | |
| }, | |
| [spacesData, selectedOrgIds] | |
| ); |
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx around lines
149 to 170, the isSpaceSharedViaOrganization check currently uses the stale
sharedSpaceIds derived from props and only detects directly shared spaces;
change it to compute the set of selected organization IDs from the current
selectedSpaces (use useMemo and add it to imports), and then consider a space
"shared via organization" when its organizationId is in that selected-org-id set
and the space itself is not the org entry (i.e., exclude when space.id ===
orgId); update the hook dependencies to use selectedSpaces (or the memo) instead
of sharedSpaces/props so the result updates as the user toggles org entries.
| const handleToggleSpace = (spaceId: string) => { | ||
| setSelectedSpaces((prev) => { | ||
| const newSet = new Set(prev); | ||
| if (newSet.has(spaceId)) { | ||
| newSet.delete(spaceId); | ||
| } else { | ||
| newSet.add(spaceId); | ||
| } | ||
| return newSet; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent edits while saving to keep UI state consistent with the in-flight request
Guard toggling when the mutation is pending to avoid divergence between server-persisted data and the local selection used in toasts and callbacks.
- const handleToggleSpace = (spaceId: string) => {
- setSelectedSpaces((prev) => {
+ const handleToggleSpace = (spaceId: string) => {
+ if (updateSharing.isPending) return;
+ setSelectedSpaces((prev) => {
const newSet = new Set(prev);
if (newSet.has(spaceId)) {
newSet.delete(spaceId);
} else {
newSet.add(spaceId);
}
return newSet;
});
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx around lines
172 to 182, the handleToggleSpace function allows toggling while a save mutation
is in-flight which can cause UI/server state divergence; update the function to
early-return when the relevant saving flag (e.g., mutation.isLoading, isSaving
or similar prop/state used for the sharing mutation) is true so toggles are
ignored during pending saves, and optionally show a disabled UI affordance or a
toast to indicate the action is blocked while saving.
Brendonovich
force-pushed
the
more-useMutation
branch
from
be052c0 to
21bbfe9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (2)
172-182: Prevent toggles while save is in-flightAllowing edits while updateSharing is pending can desync UI from persisted state and break your toasts/diff logic.
- const handleToggleSpace = (spaceId: string) => { - setSelectedSpaces((prev) => { + const handleToggleSpace = (spaceId: string) => { + if (updateSharing.isPending) return; + setSelectedSpaces((prev) => { const newSet = new Set(prev); if (newSet.has(spaceId)) { newSet.delete(spaceId); } else { newSet.add(spaceId); } return newSet; }); };
149-170: Fix “shared via organization” detection; current logic is stale and incorrect
The current check uses sharedSpaceIds derived from props (initial shares) and only detects direct shares. It fails to reflect user changes during the dialog and doesn’t capture org-level implicit sharing.Refactor to compute selected organization entries from current selection and base the “via org” flag on that:
- const sharedSpaceIds = new Set(sharedSpaces?.map((space) => space.id) || []); + // Track which organization entries are currently selected. + // An org entry is represented by a space with id === organizationId and primary === true. + const selectedOrgIds = useMemo(() => { + const orgs = new Set<string>(); + if (!spacesData) return orgs; + for (const id of selectedSpaces) { + const space = spacesData.find((sp) => sp.id === id); + if (space && space.id === space.organizationId && space.primary === true) { + orgs.add(space.organizationId); + } + } + return orgs; + }, [selectedSpaces, spacesData]); @@ - const isSpaceSharedViaOrganization = useCallback( - (spaceId: string) => { - const space = spacesData?.find((s) => s.id === spaceId); - if (!space) return false; - return sharedSpaceIds.has(space.id); - }, - [spacesData, sharedSpaceIds], - ); + const isSpaceSharedViaOrganization = useCallback( + (spaceId: string) => { + const space = spacesData?.find((s) => s.id === spaceId); + if (!space) return false; + // Consider it shared via org when its org is selected and it isn't the org-entry itself + return selectedOrgIds.has(space.organizationId) && space.id !== space.organizationId; + }, + [spacesData, selectedOrgIds], + );Additionally (outside this hunk), ensure useMemo is imported:
// at import line with React hooks import { useCallback, useEffect, useMemo, useState } from "react";apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
136-144: Refresh list after duplicate so new item appears immediatelyAfter duplication, the caps list won’t reflect the new item until the page revalidates. Trigger a refresh on success.
const duplicateMutation = useEffectMutation({ mutationFn: () => withRpc((r) => r.VideoDuplicate(cap.id)), onSuccess: () => { toast.success("Cap duplicated successfully"); + router.refresh(); }, onError: () => { toast.error("Failed to duplicate cap"); }, });apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (1)
144-158: Avoid “remove then update” split; make it atomic or add rollbackRemoving the existing domain and then updating can leave the org with no domain if the update fails. Prefer a single backend operation that swaps atomically. If you must keep it client-driven, implement a best-effort rollback.
Apply a client-side rollback until a server-side atomic swap is available:
const updateDomainMutation = useMutation({ mutationFn: async ({ domain, orgId, }: { domain: string; orgId: string; }) => { - if (activeOrganization?.organization.customDomain) { - await removeOrganizationDomain(orgId); - } - return await updateDomain(domain, orgId); + const previousDomain = activeOrganization?.organization.customDomain; + if (previousDomain) { + try { + await removeOrganizationDomain(orgId); + } catch (err) { + throw new Error( + `Failed to remove existing domain: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } + } + try { + return await updateDomain(domain, orgId); + } catch (err) { + // Best-effort restore of previous domain + if (previousDomain) { + try { + await updateDomain(previousDomain, orgId); + } catch { + // ignore restore failure, original error will be thrown + } + } + throw err instanceof Error ? err : new Error("Failed to update domain"); + } },Longer-term: move this logic to a server action (transaction/compensating steps) to guarantee atomicity.
🧹 Nitpick comments (4)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (1)
32-45: Consolidate dialog closing into onSettled and surface server error messagesReduce duplication by moving setConfirmOpen(false) to onSettled, and show actionable error messages from the server instead of a generic one.
Apply:
- const removeDomainMutation = useMutation({ + const removeDomainMutation = useMutation({ mutationFn: (organizationId: string) => removeOrganizationDomain(organizationId), - onSuccess: () => { + onSuccess: () => { setIsVerified(false); toast.success("Custom domain removed"); router.refresh(); - setConfirmOpen(false); - }, - onError: () => { - toast.error("Failed to remove domain"); - setConfirmOpen(false); - }, + }, + onError: (error) => { + toast.error( + error instanceof Error ? error.message : "Failed to remove domain", + ); + }, + onSettled: () => { + setConfirmOpen(false); + }, });apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
144-147: Revert local state on error to avoid UI/server divergenceOn failure, selection may not match the server but the dialog keeps showing local changes. Resetting selection and publicToggle to initial values keeps UI consistent.
- onError: () => { - toast.error("Failed to update sharing settings"); - }, + onError: () => { + toast.error("Failed to update sharing settings"); + setSelectedSpaces(new Set(initialSelectedSpaces)); + setPublicToggle(initialPublicState); + },apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
124-134: Surface delete errors to usersCurrently logs to console only. Add a toast to inform users of failures.
const deleteMutation = useMutation({ mutationFn: async () => { await onDelete?.(); }, - onError: (error) => { - console.error("Error deleting cap:", error); - }, + onError: (error) => { + console.error("Error deleting cap:", error); + toast.error("Failed to delete cap"); + }, onSettled: () => { setConfirmOpen(false); }, });apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (1)
308-312: Guard against missing organization ID before mutatingactiveOrganization?.organization.id is asserted as string. If undefined, this will pass a wrong value into the mutation.
Consider a defensive guard:
- updateDomainMutation.mutate({ - domain: cleanedDomain, - orgId: activeOrganization?.organization.id as string, - }); + const orgId = activeOrganization?.organization.id; + if (!orgId) { + toast.error("No active organization found"); + return; + } + updateDomainMutation.mutate({ domain: cleanedDomain, orgId });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.eslintrc.ts(0 hunks)apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/Caps.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(8 hunks)apps/web/app/(org)/dashboard/caps/components/Folder.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/components/SelectedCapsBar.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx(4 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx(4 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx(8 hunks)apps/web/app/(org)/login/form.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- .eslintrc.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/app/(org)/dashboard/caps/components/SelectedCapsBar.tsx
- apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx
- apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx
- apps/web/app/(org)/dashboard/caps/Caps.tsx
- apps/web/app/(org)/dashboard/caps/components/Folder.tsx
- apps/web/app/(org)/login/form.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
apps/web/actions/caps/share.ts (1)
shareCap(23-157)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (1)
apps/web/actions/organization/remove-domain.ts (1)
removeOrganizationDomain(9-59)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (3)
apps/web/actions/videos/download.ts (1)
downloadVideo(9-50)apps/web/lib/EffectRuntime.ts (1)
useEffectMutation(19-19)apps/web/lib/Rpcs.ts (1)
withRpc(16-17)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (3)
apps/web/actions/organization/remove-domain.ts (1)
removeOrganizationDomain(9-59)apps/web/actions/organization/update-domain.ts (1)
updateDomain(10-78)apps/web/actions/organization/check-domain.ts (1)
checkOrganizationDomain(9-55)
⏰ 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). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (7)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (2)
47-55: Subscription gate for removal may be unintended; backend doesn’t enforce itFrontend blocks removal when not subscribed, but removeOrganizationDomain (apps/web/actions/organization/remove-domain.ts) does not check subscription. This can prevent users from removing a domain they no longer want/need when unsubscribed.
- Confirm product intent. If removal should be allowed regardless of subscription, drop the isSubscribed check here.
- If removal must require subscription, mirror that policy server-side to keep behavior consistent and secure.
77-77: Good: UI bound to mutation stateConfirmationDialog and the primary Button reliably reflect mutation.isPending, preventing duplicate submissions and keeping UX consistent.
Also applies to: 148-150
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (2)
64-80: Good: mutation throws on failure; onSuccess used for side-effects onlyCorrectly moves failure handling into mutationFn and keeps onSuccess strictly for side-effects. This aligns with React Query best practices.
353-366: Good: Disabled and spinner wired to mutation stateSave button correctly reflects updateSharing.isPending and prevents duplicate submissions.
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
219-232: Good: Download flow uses mutation + toast.promise and guards re-entry
- Prevents double-trigger via isPending guard.
- Surfaces progress/success/failure via toast.
- Safely generates and clicks a temporary anchor for download.
Also applies to: 107-123
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (2)
194-202: Confirm pollInterval lifecycleYou clear pollInterval.current on verification, but this ref isn’t set anywhere in this component. If polling is intended, ensure it is initialized (and cleared on unmount) or remove the cleanup.
381-383: Good: Buttons are correctly bound to mutation state
- Domain step Next: spinner/disabled driven by updateDomainMutation.isPending.
- Verify step Check Status: disabled and shows spinner while pending.
Also applies to: 440-443, 416-431
The commit replaces local loading states and manual error handling with React Query mutations across multiple components for better state management and consistency.
Summary by CodeRabbit
Improvements
Behavior Changes
Refactor
Chores