refactor(desktop): Manage auth token in renderer instead of main thread for cleaner code / better interop with better auth #753
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (110)
📝 WalkthroughWalkthroughThis PR replaces the desktop auth subscription model with a token-centric Better Auth flow (encrypted file storage, deep-link OAuth callback handling), migrates renderer code from generic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Renderer (React)
participant Main as Main Process
participant OAuth as OAuth Provider
participant AuthServer as API/Auth Server
User->>Renderer: click "Sign in"
Renderer->>Main: trpc mutation -> signIn(provider)
Main->>Main: generate state, store stateStore
Main->>OAuth: openExternal(authURL with state)
OAuth->>User: user authenticates
OAuth->>Main: redirect to superset://app/callback?token...&state...
Main->>Main: parseAuthDeepLink -> validate state
Main->>Main: encrypt + save token file (TOKEN_FILE)
Main->>Renderer: emit token-saved via authEvents / onTokenChanged
Renderer->>Renderer: setAuthToken(token) -> authClient.refetch session
Renderer->>AuthServer: API calls with Authorization: Bearer <token>
AuthServer->>Renderer: return session/user
Renderer->>User: update UI (signed-in)
sequenceDiagram
participant Component as React Component
participant electronTrpc as electronTrpc client
participant IPC as Electron IPC
participant Main as Main Process
participant Backend as Backend Server
Component->>electronTrpc: useQuery / mutation
electronTrpc->>IPC: send request over IPC
IPC->>Main: deliver request
Main->>Backend: forward request to backend TRPC endpoints
Backend->>Main: respond
Main->>IPC: return response
IPC->>electronTrpc: deliver result
electronTrpc->>Component: update cache -> re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/DeleteWorktreeDialog.tsx (1)
30-37: Consider if infinite staleTime is appropriate for dynamic status data.Using
staleTime: Number.POSITIVE_INFINITYmeans the worktree status (uncommitted changes, unpushed commits) will never be refetched automatically. If a user closes this dialog, makes changes to the worktree, and reopens the dialog without unmounting the parent component, they could see stale status information.Consider removing
staleTimeto use the default behavior, or usegcTime: 0to clear the cache when the dialog closes so a fresh check is made on each open.Suggested fix to ensure fresh data on each dialog open
const { data: canDeleteData, isLoading } = electronTrpc.workspaces.canDeleteWorktree.useQuery( { worktreeId }, { enabled: open, - staleTime: Number.POSITIVE_INFINITY, + gcTime: 0, // Clear cache when dialog closes to ensure fresh check on reopen }, );apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx (1)
232-234: Hardcoded shortcut inconsistent with dynamic shortcut pattern.Line 233 uses hardcoded
⌘Owhile the rest of the component uses the dynamicopenInShortcutvariable. This creates inconsistency if the shortcut is reassigned.Proposed fix
{showShortcuts && app.id === lastUsedApp && ( - <span className="text-xs text-muted-foreground">⌘O</span> + <span className="text-xs text-muted-foreground"> + {openInShortcut} + </span> )}apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx (1)
23-30: Loading state may persist indefinitely if token or organization is unavailable.The comment states "Show loading only on initial mount," but the spinner renders whenever
collectionsisnull—including cases wheretokenoractiveOrganizationIdremain unavailable (e.g., user has no active organization). Consider adding a timeout, error state, or distinguishing between "loading" and "missing required data."apps/desktop/src/renderer/routes/_authenticated/layout.tsx (1)
13-18: Handle session loading state to avoid flash redirect.
authClient.useSession()exposes anisPendingstate (as used in AccountSettingsPage) that should be checked before redirecting. Currently, if the session is still loading,sessionwill beundefined, triggering an immediate redirect to/sign-inbefore the actual session data arrives, creating a brief redirect flash.Suggested fix
function AuthenticatedLayout() { - const { data: session } = authClient.useSession(); + const { data: session, isPending } = authClient.useSession(); const isSignedIn = !!session?.user; + if (isPending) { + return null; // or a loading spinner + } + if (!isSignedIn) { return <Navigate to="/sign-in" replace />; }
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/lib/auth-client.ts`:
- Around line 26-31: The fetchOptions.auth.token callback currently returns an
empty string when authToken is null which sends an empty Authorization header;
update the token callback in fetchOptions (auth.type "Bearer") to return
undefined instead of "" when unauthenticated (e.g., change token: () =>
authToken || "" to token: () => authToken || undefined) so the Authorization
header is omitted when no token is present.
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 33-44: The onTokenChanged subscription handler is causing an
infinite loop by calling persistMutation.mutate() which ultimately emits
"token-saved" via saveToken/persistToken and retriggers
electronTrpc.auth.onTokenChanged; remove the persistMutation.mutate(...) call
from the subscription onData (leave setToken(...) and setAuthToken(...)) so the
handler only updates local state, or alternatively add a guard in the handler to
ignore events that originated from the main process; reference:
electronTrpc.auth.onTokenChanged.useSubscription, persistMutation.mutate,
persistToken, saveToken, and authEvents.
In `@apps/desktop/src/renderer/providers/TRPCProvider/TRPCProvider.tsx`:
- Around line 27-40: The renderer TRPC client in TRPCProvider (apiClient created
via trpc.createClient and httpBatchLink) doesn't handle 401/403 responses like
the main process client; update the httpBatchLink to use a custom fetch/response
handler that checks response.status for 401 or 403 and calls
authService.signOut() (and clears any local token/session) when encountered,
while preserving existing header logic using getAuthToken(); ensure this
handling is implemented inside the link configuration for TRPCProvider so stale
auth state is cleared on auth errors.
In `@apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts`:
- Around line 86-105: The navigation target is being fetched in the onSuccess
handler after deletion, which fails because queries like
utils.workspaces.getPreviousWorkspace.fetch and
utils.workspaces.getNextWorkspace.fetch filter out deleted workspaces; move the
logic to onMutate so you compute prev/next BEFORE deletion. In
useDeleteWorkspace.ts, inside onMutate (using the same variables.id and
params.workspaceId context), call utils.workspaces.getPreviousWorkspace.fetch
and getNextWorkspace.fetch, pick targetWorkspaceId, and stash it on the mutation
context; then in onSuccess use that context value to call navigateToWorkspace or
navigate instead of re-querying with the deleted id.
In `@apps/desktop/src/renderer/routes/sign-in/page.tsx`:
- Around line 24-27: The page currently redirects based solely on session?.user
which causes UI flicker during initial hydration; update the
authClient.useSession() call to destructure isPending (e.g., const { data:
session, isPending } = authClient.useSession()), then short-circuit render while
isPending is true (return null or a loading component) before computing
isSignedIn and performing the Navigate to "/workspace"; this ensures you only
evaluate isSignedIn and call Navigate when isPending is false.
In `@apps/desktop/src/renderer/stores/app-state.ts`:
- Around line 24-26: The setView setter only updates currentView and leaves
related boolean flags stale; update setView to also update isTasksTabOpen (and
any other view-specific flags) to match the new view (e.g., set isTasksTabOpen =
(view === "tasks")), so state stays consistent with openTasks(); alternatively
remove setView if direct use is forbidden—change the implementation in the same
store where setView, openTasks, currentView, and isTasksTabOpen are defined to
implement the synchronization.
🧹 Nitpick comments (22)
apps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsx (1)
42-89: Consider extractingHotkeyRowto a separate file.Per coding guidelines, each file should contain only one component.
HotkeyRowcould be extracted to its own file (e.g.,hotkey-row.tsx) for consistency with the "one component per file" rule.That said, since
HotkeyRowis small and tightly coupled to this page with no reuse elsewhere, this is a minor concern that can be deferred.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts (1)
26-51: Consider adding error handlers to subscriptions.Both subscriptions lack
onErrorcallbacks. If the subscription fails (e.g., connection drops), errors will be silently lost. As per coding guidelines, errors should not be swallowed silently.♻️ Suggested improvement
electronTrpc.ports.subscribeStatic.useSubscription( { workspaceId: "" }, { onData: () => { utils.ports.getAllStatic.invalidate(); }, + onError: (error) => { + console.error("[PortsData/subscribeStatic] subscription error:", error); + }, }, );electronTrpc.ports.subscribe.useSubscription(undefined, { onData: (event) => { if (event.type === "add") { addPort(event.port); } else if (event.type === "remove") { removePort(event.port.paneId, event.port.port); } }, + onError: (error) => { + console.error("[PortsData/subscribe] subscription error:", error); + }, });apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx (1)
61-106: Consider logging the caught error for debugging.The migration to
electronTrpc.projects.close.useMutationis correct. However, the empty catch block at lines 72-74 silently swallows errors. As per coding guidelines, errors should at least be logged with context for debugging purposes.♻️ Suggested improvement
if (params.workspaceId) { try { const currentWorkspace = await utils.workspaces.get.fetch({ id: params.workspaceId, }); shouldNavigate = currentWorkspace?.projectId === id; - } catch { + } catch (error) { // Workspace might not exist, skip navigation + console.debug("[ProjectHeader/closeProject] Workspace fetch failed, skipping navigation:", error); } }apps/desktop/src/renderer/stores/app-state.ts (2)
6-15: Consider renamingAppStateto avoid confusion with main process type.There's an existing
AppStateinterface inapps/desktop/src/main/lib/app-state/schemas.tswith a different shape (containingtabsState,themeState,hotkeysState). Consider renaming this interface toRendererAppStateorAppViewStateto distinguish it clearly.
48-54: Misleading hook names and missinguseCloseTasks.Two issues with convenience hooks:
useOpenTasks,useOpenWorkspacesList,useCloseWorkspacesListsound like they return booleans but return action functions. Consider renaming (e.g.,useOpenTasksActionor keep justuseAppStore).useCloseTasksis missing whileuseCloseWorkspacesListexists—inconsistent API surface.Proposed fix
// Convenience hooks export const useCurrentView = () => useAppStore((state) => state.currentView); -export const useOpenTasks = () => useAppStore((state) => state.openTasks); -export const useOpenWorkspacesList = () => - useAppStore((state) => state.openWorkspacesList); -export const useCloseWorkspacesList = () => - useAppStore((state) => state.closeWorkspacesList); +export const useIsTasksTabOpen = () => useAppStore((state) => state.isTasksTabOpen); +export const useIsWorkspacesListOpen = () => useAppStore((state) => state.isWorkspacesListOpen);Alternatively, add the missing hook and rename for clarity:
+export const useCloseTasks = () => useAppStore((state) => state.closeTasks);apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/ProjectsSettings/ProjectsSettings.tsx (1)
16-20: Consider guarding the auto-expand to run only on initial load.The current
useEffectre-expands all projects whenevergroupschanges (e.g., on refetch), which will override any manual collapse actions by the user.If the intent is to expand only on initial load, consider tracking whether initial expansion has occurred:
♻️ Suggested improvement
+import { useRef } from "react"; ... + const hasInitializedRef = useRef(false); + // Expand all projects by default when groups are loaded useEffect(() => { - if (groups.length > 0) { + if (groups.length > 0 && !hasInitializedRef.current) { + hasInitializedRef.current = true; setExpandedProjects(new Set(groups.map((g) => g.project.id))); } }, [groups]);apps/desktop/src/renderer/providers/TRPCProvider/TRPCProvider.tsx (1)
11-25: Consider using shared QueryClient.A shared
queryClientis already defined inapps/desktop/src/renderer/lib/query-client.tswith identical configuration. Creating a new instance here means query cache isn't shared ifqueryClientis used elsewhere.Proposed refactor
+import { queryClient } from "renderer/lib/query-client"; export function TRPCProvider({ children }: { children: ReactNode }) { - const [queryClient] = useState( - () => - new QueryClient({ - defaultOptions: { - queries: { - networkMode: "always", - retry: false, - }, - mutations: { - networkMode: "always", - retry: false, - }, - }, - }), - );apps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsx (1)
3-4: Inconsistent import style.Line 3 uses the alias path
renderer/lib/electron-trpcwhile line 4 uses a relative path../../lib/trpc-client. Per coding guidelines, prefer using aliases as defined intsconfig.jsonwhen possible for consistency.Suggested fix
import { electronTrpc } from "renderer/lib/electron-trpc"; -import { electronReactClient } from "../../lib/trpc-client"; +import { electronReactClient } from "renderer/lib/trpc-client";apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx (1)
3-4: Consider removing the alias for clarity.The import aliases
electronTrpcClient as trpcClientwhich may cause confusion during code review or future maintenance. Since this is a migration PR, consider using the actual name directly:import { electronTrpc } from "renderer/lib/electron-trpc"; -import { electronTrpcClient as trpcClient } from "renderer/lib/trpc-client"; +import { electronTrpcClient } from "renderer/lib/trpc-client";Then update line 41 to use
electronTrpcClient.workspaces.get.query(...).apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/page.tsx (1)
5-5: Migration looks correct; consider prefixed logging for consistency.The migration to
electronTrpcClientis correct. Using the proxy client (not React hooks) for imperative mutations in callbacks is the right pattern here.Minor: The error logs could follow the prefixed pattern per coding guidelines:
} catch (error) { - console.error("Failed to stop ringtone:", error); + console.error("[RingtonesSettings/handleTogglePlay] Failed to stop ringtone:", error); } ... } catch (error) { - console.error("Failed to play ringtone:", error); + console.error("[RingtonesSettings/handleTogglePlay] Failed to play ringtone:", error); setPlayingId(null); }Also applies to: 170-173, 189-216
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/OrganizationDropdown.tsx (2)
57-60: Consider adding error handling for sign-out flow.The sign-out flow awaits
authClient.signOut()but then firessignOutMutation.mutate()without awaiting or handling errors. If the server-side sign-out fails, the user is already signed out client-side, potentially leaving server state inconsistent. Consider handling mutation errors or at minimum logging them.♻️ Suggested improvement
const handleSignOut = async () => { await authClient.signOut(); - signOutMutation.mutate(); + signOutMutation.mutate(undefined, { + onError: (error) => { + console.error("[auth/signOut] Server sign-out failed:", error); + }, + }); };
51-55: Consider adding error handling for organization switch.If
authClient.organization.setActivefails, users receive no feedback and the UI may show a stale organization state. Consider wrapping in try-catch with user feedback or at minimum logging.♻️ Suggested improvement
const switchOrganization = async (newOrgId: string) => { + try { await authClient.organization.setActive({ organizationId: newOrgId, }); + } catch (error) { + console.error("[auth/switchOrg] Failed to switch organization:", error); + // Optionally show toast/notification to user + } };apps/desktop/src/renderer/routes/_authenticated/settings/workspace/$workspaceId/page.tsx (1)
1-13: Unconventional import ordering - consider reorganizing.The route declaration (lines 1-7) appears before the imports (lines 9-13). While this works due to function hoisting, it's unconventional and may confuse readers. Consider moving imports to the top of the file.
♻️ Suggested reordering
+import { Input } from "@superset/ui/input"; +import { HiOutlineFolder, HiOutlinePencilSquare } from "react-icons/hi2"; +import { LuGitBranch } from "react-icons/lu"; +import { electronTrpc } from "renderer/lib/electron-trpc"; +import { useWorkspaceRename } from "renderer/screens/main/hooks/useWorkspaceRename"; import { createFileRoute } from "@tanstack/react-router"; export const Route = createFileRoute( "/_authenticated/settings/workspace/$workspaceId/", )({ component: WorkspaceSettingsPage, }); -import { Input } from "@superset/ui/input"; -import { HiOutlineFolder, HiOutlinePencilSquare } from "react-icons/hi2"; -import { LuGitBranch } from "react-icons/lu"; -import { electronTrpc } from "renderer/lib/electron-trpc"; -import { useWorkspaceRename } from "renderer/screens/main/hooks/useWorkspaceRename"; - function WorkspaceSettingsPage() {apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/page.tsx (1)
19-23: Redundantenabledoption.The
enabled: !!projectIdcheck is unnecessary sinceprojectIdis a required route parameter fromRoute.useParams()and will always be a truthy string when this component renders.Suggested simplification
const { data: configFilePath } = electronTrpc.config.getConfigFilePath.useQuery( - { projectId }, - { enabled: !!projectId }, + { projectId }, );apps/desktop/src/renderer/react-query/workspaces/useOpenWorktree.ts (1)
45-50: Consider adding error handling for terminal creation.The
createOrAttach.mutate()call doesn't handle potential errors. If terminal session creation fails, the user receives no feedback, and the tab will exist but with a non-functional terminal.Suggested error handling
// Pre-create terminal session (with initial commands if present) // Terminal component will attach to this session when it mounts - createOrAttach.mutate({ + createOrAttach.mutate( + { paneId, tabId, workspaceId: data.workspace.id, initialCommands, - }); + }, + { + onError: (error) => { + console.error("[useOpenWorktree] Failed to create terminal session:", error); + }, + }, + );apps/desktop/src/lib/trpc/routers/auth/utils/crypto-storage.ts (2)
55-61: Fallback machine ID is predictable.The fallback
${hostname()}-${homedir()}-superset-fallbackuses publicly known values. If an attacker has access to the encrypted file and knows it uses the fallback, they can compute the decryption key. This is acceptable for "defense in depth" against casual snooping, but the comment should clarify the threat model.Consider logging when the fallback is used (without exposing the actual ID) so developers can investigate platform-specific failures:
} catch { - // Fallback if platform-specific method fails + // Fallback if platform-specific method fails + console.warn("[crypto-storage] Platform-specific machine ID unavailable, using fallback"); }
67-70: Consider explicit scrypt parameters for reproducibility.
scryptSyncuses Node.js defaults for cost parameters (N=16384, r=8, p=1). While these are reasonable, specifying them explicitly ensures consistent behavior across Node.js versions and documents the security posture.Explicit scrypt options
+const SCRYPT_OPTIONS = { N: 16384, r: 8, p: 1 }; + function deriveKey(salt: Buffer): Buffer { const machineId = getMachineId(); - return scryptSync(machineId, salt, KEY_LENGTH); + return scryptSync(machineId, salt, KEY_LENGTH, SCRYPT_OPTIONS); }apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts (1)
29-36: Silent error handling loses diagnostic context.The catch block swallows all errors, which makes debugging difficult when token loading fails for unexpected reasons (e.g., corrupted file, permission issues). Per coding guidelines, errors should at minimum be logged with context.
Proposed fix
} catch { + console.log("[auth] No stored token found or failed to load"); return { token: null, expiresAt: null }; }apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (1)
56-62: Redundant effect for token synchronization.This effect synchronizes
tokentosetAuthToken, butsetAuthTokenis already called directly in both the subscription handler (line 37) and the hydration effect (line 50). This effect is redundant and causes unnecessary work on every token change.Consider removing this effect
- useEffect(() => { - if (token) { - setAuthToken(token); - } else { - setAuthToken(null); - } - }, [token]);apps/desktop/src/lib/trpc/routers/auth/index.ts (3)
35-53: Unhandled promise rejection in subscription initialization.The
loadToken().then()call on line 37 doesn't have a.catch()handler. IfloadToken()throws (despite its internal try-catch), the promise rejection would be unhandled. WhileloadTokencurrently catches errors internally, this is defensive programming against future changes.Proposed fix
onTokenChanged: publicProcedure.subscription(() => { return observable<{ token: string; expiresAt: string } | null>((emit) => { - loadToken().then((initial) => { - if (initial.token && initial.expiresAt) { - emit.next({ token: initial.token, expiresAt: initial.expiresAt }); - } - }); + loadToken() + .then((initial) => { + if (initial.token && initial.expiresAt) { + emit.next({ token: initial.token, expiresAt: initial.expiresAt }); + } + }) + .catch((err) => { + console.error("[auth/onTokenChanged] Failed to load initial token:", err); + });
66-70: Extract magic number to named constant.The 10-minute TTL for OAuth state cleanup is a magic number. Per coding guidelines, extract this to a named constant for clarity.
Proposed fix
+const STATE_TTL_MS = 10 * 60 * 1000; // 10 minutes + export const createAuthRouter = () => { return router({ // ... signIn: publicProcedure .input(z.object({ provider: z.enum(AUTH_PROVIDERS) })) .mutation(async ({ input }) => { try { const state = crypto.randomBytes(32).toString("base64url"); stateStore.set(state, Date.now()); // Clean up old states (older than 10 minutes) - const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + const tenMinutesAgo = Date.now() - STATE_TTL_MS; for (const [s, ts] of stateStore) { if (ts < tenMinutesAgo) stateStore.delete(s); }
88-93: Silent error swallowing in signOut.The empty catch block swallows all errors, including unexpected ones like permission issues. Per coding guidelines, at minimum log errors with context. Consider checking for
ENOENTspecifically.Proposed fix
signOut: publicProcedure.mutation(async () => { try { await fs.unlink(TOKEN_FILE); - } catch {} + } catch (err) { + // ENOENT is expected if file doesn't exist + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + console.error("[auth/signOut] Failed to remove token file:", err); + } + } return { success: true }; }),
| const apiClient = useMemo(() => { | ||
| return trpc.createClient({ | ||
| links: [ | ||
| httpBatchLink({ | ||
| url: `${env.NEXT_PUBLIC_API_URL}/api/trpc`, | ||
| headers() { | ||
| const token = getAuthToken(); | ||
| return token ? { Authorization: `Bearer ${token}` } : {}; | ||
| }, | ||
| transformer: superjson, | ||
| }), | ||
| ], | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Missing auth error handling compared to main process client.
The main process apiClient (in apps/desktop/src/main/lib/api-client.ts) handles 401/403 responses by clearing the session via authService.signOut(). This renderer apiClient lacks similar handling, which could lead to stale auth state when the API returns auth errors.
Proposed fix - add custom fetch with auth error handling
const apiClient = useMemo(() => {
return trpc.createClient({
links: [
httpBatchLink({
url: `${env.NEXT_PUBLIC_API_URL}/api/trpc`,
headers() {
const token = getAuthToken();
return token ? { Authorization: `Bearer ${token}` } : {};
},
transformer: superjson,
+ async fetch(url, options) {
+ const response = await globalThis.fetch(url, options);
+ if (response.status === 401 || response.status === 403) {
+ console.log("[TRPCProvider] Auth error, clearing session");
+ // TODO: Call appropriate auth clearing function
+ }
+ return response;
+ },
}),
],
});
}, []);📝 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 apiClient = useMemo(() => { | |
| return trpc.createClient({ | |
| links: [ | |
| httpBatchLink({ | |
| url: `${env.NEXT_PUBLIC_API_URL}/api/trpc`, | |
| headers() { | |
| const token = getAuthToken(); | |
| return token ? { Authorization: `Bearer ${token}` } : {}; | |
| }, | |
| transformer: superjson, | |
| }), | |
| ], | |
| }); | |
| }, []); | |
| const apiClient = useMemo(() => { | |
| return trpc.createClient({ | |
| links: [ | |
| httpBatchLink({ | |
| url: `${env.NEXT_PUBLIC_API_URL}/api/trpc`, | |
| headers() { | |
| const token = getAuthToken(); | |
| return token ? { Authorization: `Bearer ${token}` } : {}; | |
| }, | |
| transformer: superjson, | |
| async fetch(url, options) { | |
| const response = await globalThis.fetch(url, options); | |
| if (response.status === 401 || response.status === 403) { | |
| console.log("[TRPCProvider] Auth error, clearing session"); | |
| // TODO: Call appropriate auth clearing function | |
| } | |
| return response; | |
| }, | |
| }), | |
| ], | |
| }); | |
| }, []); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/providers/TRPCProvider/TRPCProvider.tsx` around
lines 27 - 40, The renderer TRPC client in TRPCProvider (apiClient created via
trpc.createClient and httpBatchLink) doesn't handle 401/403 responses like the
main process client; update the httpBatchLink to use a custom fetch/response
handler that checks response.status for 401 or 403 and calls
authService.signOut() (and clears any local token/session) when encountered,
while preserving existing header logic using getAuthToken(); ensure this
handling is implemented inside the link configuration for TRPCProvider so stale
auth state is cleared on auth errors.
| // If the deleted workspace is currently being viewed, navigate away | ||
| if (params.workspaceId === variables.id) { | ||
| // Try to navigate to previous workspace first, then next | ||
| const prevWorkspaceId = | ||
| await utils.workspaces.getPreviousWorkspace.fetch({ | ||
| id: variables.id, | ||
| }); | ||
| const nextWorkspaceId = await utils.workspaces.getNextWorkspace.fetch({ | ||
| id: variables.id, | ||
| }); | ||
|
|
||
| // If we optimistically navigated away, navigate back to the deleted workspace | ||
| if (context?.wasViewingDeleted) { | ||
| navigateToWorkspace(variables.id, navigate); | ||
| const targetWorkspaceId = prevWorkspaceId ?? nextWorkspaceId; | ||
|
|
||
| if (targetWorkspaceId) { | ||
| navigateToWorkspace(targetWorkspaceId, navigate); | ||
| } else { | ||
| // No other workspaces, navigate to workspace index (shows StartView) | ||
| navigate({ to: "/workspace" }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementations of getPreviousWorkspace and getNextWorkspace
echo "=== Finding all TS/TSX files that might contain these procedures ==="
find . -type f \( -name "*.ts" -o -name "*.tsx" \) | grep -E "(router|procedure|trpc)" | head -20
echo ""
echo "=== Search for getPreviousWorkspace and getNextWorkspace definitions ==="
rg -n "getPreviousWorkspace|getNextWorkspace" --type ts -B 2 -A 15 | head -100
echo ""
echo "=== Look for workspaces router or procedures ==="
fd -e ts -e tsx "workspaces" | grep -i router
echo ""
echo "=== Search in routers directory ==="
find . -path "*/routers/*" -name "*.ts" | head -20Repository: superset-sh/superset
Length of output: 13531
🏁 Script executed:
#!/bin/bash
# Get the full implementation of getPreviousWorkspace and getNextWorkspace
echo "=== Reading workspaces query procedures file ==="
wc -l apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
echo ""
echo "=== Examining getPreviousWorkspace and getNextWorkspace implementations ==="
sed -n '199,250p' apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsRepository: superset-sh/superset
Length of output: 1241
Move workspace navigation logic to onMutate to avoid querying with a deleted workspace ID.
After the workspace is deleted, calling getPreviousWorkspace and getNextWorkspace with the deleted workspace's ID will always return null because these queries filter workspaces where deletingAt is null. The deleted workspace won't be in the list, making the queries unnecessary. Fetch the navigation target in onMutate (before deletion) instead of onSuccess (after deletion) for efficiency and clarity.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts`
around lines 86 - 105, The navigation target is being fetched in the onSuccess
handler after deletion, which fails because queries like
utils.workspaces.getPreviousWorkspace.fetch and
utils.workspaces.getNextWorkspace.fetch filter out deleted workspaces; move the
logic to onMutate so you compute prev/next BEFORE deletion. In
useDeleteWorkspace.ts, inside onMutate (using the same variables.id and
params.workspaceId context), call utils.workspaces.getPreviousWorkspace.fetch
and getNextWorkspace.fetch, pick targetWorkspaceId, and stash it on the mutation
context; then in onSuccess use that context value to call navigateToWorkspace or
navigate instead of re-querying with the deleted id.
| setView: (view) => { | ||
| set({ currentView: view }); | ||
| }, |
There was a problem hiding this comment.
setView doesn't synchronize boolean flags, causing potential state inconsistency.
If setView("tasks") is called directly, currentView becomes "tasks" but isTasksTabOpen remains false. This contradicts the state set by openTasks(). Either:
- Remove
setViewif it shouldn't be used directly, or - Synchronize flags based on the view being set.
Proposed fix to synchronize flags
setView: (view) => {
- set({ currentView: view });
+ set({
+ currentView: view,
+ isTasksTabOpen: view === "tasks",
+ isWorkspacesListOpen: view === "workspaces-list",
+ });
},📝 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.
| setView: (view) => { | |
| set({ currentView: view }); | |
| }, | |
| setView: (view) => { | |
| set({ | |
| currentView: view, | |
| isTasksTabOpen: view === "tasks", | |
| isWorkspacesListOpen: view === "workspaces-list", | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/stores/app-state.ts` around lines 24 - 26, The
setView setter only updates currentView and leaves related boolean flags stale;
update setView to also update isTasksTabOpen (and any other view-specific flags)
to match the new view (e.g., set isTasksTabOpen = (view === "tasks")), so state
stays consistent with openTasks(); alternatively remove setView if direct use is
forbidden—change the implementation in the same store where setView, openTasks,
currentView, and isTasksTabOpen are defined to implement the synchronization.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/window-loader.ts (1)
16-21: Unusedidandqueryparameters.The
idandqueryparameters are declared in the props type but never used in the function body. This is especially concerning forquerysince callers may expect query parameters to be appended to the URL but they're silently dropped.Suggested fix
Either remove the unused parameters if they're not needed:
export function registerRoute(props: { - id: WindowId; browserWindow: BrowserWindow; htmlFile: string; - query?: Record<string, string>; }): void {Or if
queryshould be used, append it to the URL:} else { // Production: load from custom protocol with hash routing // Origin becomes: superset://app (trusted by Better Auth) const fileName = props.htmlFile.split("/").pop() || "index.html"; - const url = `${PROTOCOL_SCHEME}://app/${fileName}#/`; + const queryString = props.query + ? `?${new URLSearchParams(props.query).toString()}` + : ""; + const url = `${PROTOCOL_SCHEME}://app/${fileName}${queryString}#/`; props.browserWindow.loadURL(url); }
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 29-37: The effect's current guard (storedToken && !isHydrated)
skips calling setIsHydrated(true) when storedToken is null/undefined, causing an
infinite loader; change the logic in the useEffect (the block referencing
storedToken, isHydrated, setIsHydrated, setAuthToken, refetchSession) so that
when !isHydrated you always call setIsHydrated(true) — but only call
setAuthToken(storedToken.token) and refetchSession() if storedToken?.token and
storedToken?.expiresAt are present; alternatively, keep the existing if
(storedToken?.token && storedToken?.expiresAt) branch and add an else that still
calls setIsHydrated(true) to ensure hydration completes when no token exists.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`:
- Around line 10-11: The code is reading activeOrganizationId from the wrong
path; where you destructure `const { data: session } = authClient.useSession()`
the `session` variable is the session object itself, so replace any
`session?.session?.activeOrganizationId` access with
`session?.activeOrganizationId` (update usages in CollectionsProvider, e.g., the
`activeOrganizationId` assignment) so the correct property is read from the
`useSession` return.
🧹 Nitpick comments (4)
packages/auth/src/server.ts (1)
24-28: Consider extracting protocol origins to named constants.The conditional desktop URL inclusion is implemented correctly. However, per coding guidelines, hardcoded strings like the custom protocol origins could be extracted to module-level constants for better maintainability and reuse.
♻️ Suggested refactor
+const ELECTRON_PROD_ORIGIN = "superset://app"; +const ELECTRON_DEV_ORIGIN = "superset-dev://app"; + export const auth = betterAuth({ // ... trustedOrigins: [ env.NEXT_PUBLIC_WEB_URL, env.NEXT_PUBLIC_API_URL, env.NEXT_PUBLIC_MARKETING_URL, env.NEXT_PUBLIC_ADMIN_URL, // Electron desktop app origins ...(env.NEXT_PUBLIC_DESKTOP_URL ? [env.NEXT_PUBLIC_DESKTOP_URL] : []), - "superset://app", // Production Electron app - "superset-dev://app", // Development Electron app (if using custom protocol) + ELECTRON_PROD_ORIGIN, + ELECTRON_DEV_ORIGIN, ],apps/desktop/src/renderer/routes/sign-in/page.tsx (1)
38-41: Consider handling sign-in mutation errors.The
signInMutationdoesn't handle failure cases. If the OAuth flow fails (e.g., network error, cancelled by user), the user won't see any feedback.♻️ Suggested improvement
- const signInMutation = electronTrpc.auth.signIn.useMutation(); + const signInMutation = electronTrpc.auth.signIn.useMutation({ + onError: (error) => { + console.error("[auth/signIn] Sign-in failed:", error.message); + // Consider showing a toast or error message to the user + }, + });apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx (1)
13-27: Consider distinguishing loading vs. no-organization states.Currently,
collectionsisnullin two cases:
- Session is still loading (
isPending)- User has no
activeOrganizationIdBoth show the same spinner indefinitely. Consider checking
isPendingfromuseSession()to differentiate and potentially prompt the user to select an organization.♻️ Suggested improvement
export function CollectionsProvider({ children }: { children: ReactNode }) { - const { data: session } = authClient.useSession(); + const { data: session, isPending } = authClient.useSession(); const activeOrganizationId = session?.session?.activeOrganizationId; const collections = useMemo(() => { if (!activeOrganizationId) { return null; } return getCollections(activeOrganizationId); }, [activeOrganizationId]); + if (isPending) { + return ( + <div className="flex items-center justify-center h-screen"> + <div className="h-8 w-8 animate-spin rounded-full border-2 border-muted-foreground border-t-transparent" /> + </div> + ); + } + if (!collections) { - return ( - <div className="flex items-center justify-center h-screen"> - <div className="h-8 w-8 animate-spin rounded-full border-2 border-muted-foreground border-t-transparent" /> - </div> - ); + // No active organization - consider redirecting or showing org selector + return null; }apps/desktop/src/renderer/routes/_authenticated/layout.tsx (1)
16-20: Consider extracting the loading spinner to a shared component.This loading spinner pattern (full-screen centered spinner) is duplicated across multiple files:
AuthProvider.tsx(lines 53-55)_authenticated/layout.tsx(lines 17-19)CollectionsProvider.tsx(per relevant snippets)The border widths also differ slightly (
border-4vsborder-2). Consider extracting to a shared<FullScreenSpinner />component for consistency.Example shared component
// renderer/components/FullScreenSpinner.tsx export function FullScreenSpinner() { return ( <div className="flex h-screen w-screen items-center justify-center bg-background"> <div className="h-8 w-8 animate-spin rounded-full border-4 border-muted-foreground border-t-transparent" /> </div> ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
apps/desktop/docs/ROUTING_REFACTOR_PLAN.mdapps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/auth.tsapps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/lib/auth-client.tsapps/desktop/src/renderer/lib/electron-trpc.tsapps/desktop/src/renderer/lib/query-client.tsapps/desktop/src/renderer/lib/trpc.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/AuthProvider/index.tsapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/index.tsapps/desktop/src/renderer/providers/TRPCProvider/TRPCProvider.tsxapps/desktop/src/renderer/providers/TRPCProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/providers/OrganizationsProvider/OrganizationsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/providers/OrganizationsProvider/index.tsapps/desktop/src/renderer/routes/layout.tsxapps/desktop/src/renderer/routes/sign-in/page.tsxpackages/auth/src/env.tspackages/auth/src/server.ts
💤 Files with no reviewable changes (6)
- apps/desktop/src/renderer/providers/TRPCProvider/index.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/OrganizationsProvider/index.ts
- apps/desktop/src/renderer/lib/query-client.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/OrganizationsProvider/OrganizationsProvider.tsx
- apps/desktop/src/renderer/providers/TRPCProvider/TRPCProvider.tsx
- apps/desktop/src/renderer/lib/trpc.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/docs/ROUTING_REFACTOR_PLAN.md
- apps/desktop/src/main/lib/auth/auth.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/lib/auth-client.ts
- apps/desktop/src/main/index.ts
- apps/desktop/src/renderer/providers/ElectronTRPCProvider/index.ts
- apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/providers/AuthProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/layout.tsxapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/sign-in/page.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/providers/AuthProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/layout.tsxpackages/auth/src/server.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxpackages/auth/src/env.tsapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/sign-in/page.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/providers/AuthProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/layout.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/sign-in/page.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/layout.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/sign-in/page.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/providers/AuthProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/layout.tsxapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/sign-in/page.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/providers/AuthProvider/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/layout.tsxpackages/auth/src/server.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxpackages/auth/src/env.tsapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/sign-in/page.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
apps/desktop/src/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Files:
apps/desktop/src/lib/window-loader.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/layout.tsxpackages/auth/src/server.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/layout.tsxpackages/auth/src/server.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/layout.tsxapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/providers/AuthProvider/index.tspackages/auth/src/server.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/desktop/src/renderer/routes/_authenticated/layout.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
packages/auth/src/server.tsapps/desktop/src/lib/window-loader.tspackages/auth/src/env.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
Applied to files:
apps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Applied to files:
apps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/renderer/lib/electron-trpc.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
Applied to files:
apps/desktop/src/renderer/lib/electron-trpc.ts
🧬 Code graph analysis (8)
apps/desktop/src/renderer/index.tsx (1)
apps/desktop/src/renderer/providers/ElectronTRPCProvider/index.ts (1)
electronQueryClient(3-3)
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts (2)
packages/auth/src/env.ts (1)
env(8-28)apps/desktop/src/renderer/lib/auth-client.ts (1)
getAuthToken(11-13)
apps/desktop/src/renderer/routes/_authenticated/layout.tsx (3)
apps/desktop/src/renderer/lib/auth-client.ts (1)
authClient(21-33)apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx (1)
CollectionsProvider(9-34)
apps/desktop/src/renderer/routes/layout.tsx (2)
apps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsx (1)
ElectronTRPCProvider(23-35)apps/desktop/src/renderer/providers/ElectronTRPCProvider/index.ts (1)
ElectronTRPCProvider(2-2)
packages/auth/src/server.ts (1)
packages/auth/src/env.ts (1)
env(8-28)
apps/desktop/src/lib/window-loader.ts (1)
apps/desktop/src/shared/constants.ts (1)
PROTOCOL_SCHEME(30-31)
apps/desktop/src/renderer/routes/sign-in/page.tsx (2)
apps/desktop/src/renderer/lib/auth-client.ts (1)
authClient(21-33)apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
apps/desktop/src/lib/trpc/routers/index.ts (1)
AppRouter(43-43)
⏰ 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). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (15)
packages/auth/src/env.ts (1)
23-23: LGTM!The optional desktop URL environment variable is correctly defined with proper URL validation and follows the established naming convention.
apps/desktop/src/lib/window-loader.ts (1)
28-34: Production URL construction looks correct.The switch from
file://to the custom protocol (superset://app/...) with hash routing is a valid approach for establishing a consistent origin trusted by Better Auth. The filename extraction has a safe fallback for edge cases.apps/desktop/src/renderer/providers/AuthProvider/index.ts (1)
1-1: LGTM!Clean barrel export. The removal of
useAuthfrom the public API aligns with the migration to token-based authentication viaauthClient.useSession().apps/desktop/src/renderer/lib/electron-trpc.ts (1)
1-13: LGTM!Well-structured Electron IPC tRPC client setup:
- Correctly uses
createTRPCReactwith the sharedAppRoutertypeabortOnUnmount: trueprevents orphaned requests when components unmount- No Node.js modules imported (renderer-safe)
- Uses alias imports per project conventions
Based on learnings, this correctly follows the guideline to use tRPC for Electron IPC as defined in
src/lib/trpc.apps/desktop/src/renderer/routes/sign-in/page.tsx (2)
17-18: LGTM on session handling!The
isPendingstate is now properly destructured and handled, addressing the previous review feedback about UI flickering during session hydration.
24-31: Good loading state implementation.Clean loading spinner while session is being fetched prevents premature rendering of the sign-in form.
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts (4)
33-45: LGTM on singleton API client with dynamic auth.Clean implementation using tRPC's
headerscallback to fetch the token at request time. This ensures fresh tokens are used for each request without coupling to React's render cycle.
65-71: Good pattern: centralized dynamic headers object.Defining the
headersobject once and reusing it across all collections follows DRY principles and ensures consistent auth behavior.
184-203: Clean API simplification.Removing the
tokenparameter fromgetCollectionsand relying ongetAuthToken()internally simplifies the public API. Callers no longer need to manage token threading.
47-63: No changes needed —@electric-sql/clientfully supports function-valued headers.The
Authorizationheader defined as a function is a supported pattern in Electric SQL'sshapeOptions. The client resolves function-based headers (sync or async) at request time, allowing dynamic token values. The code is correct as written.apps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsx (1)
1-38: LGTM! Well-structured Electron TRPC provider.The provider correctly wires the Electron IPC tRPC client with TanStack Query. The configuration choices (
networkMode: "always",retry: false) are appropriate for IPC communication. Based on learnings, this properly uses tRPC for Electron interprocess communication as defined insrc/lib/trpc.One minor consideration: the module-level
queryClientsingleton works well for the Electron desktop app but could complicate testing if you need isolated query clients per test.apps/desktop/src/renderer/index.tsx (1)
12-12: LGTM!Clean migration to use
electronQueryClientfrom the new provider. The router context integration remains consistent with the previous pattern.Also applies to: 24-24
apps/desktop/src/renderer/routes/layout.tsx (1)
5-5: LGTM!Clean swap from
TRPCProvidertoElectronTRPCProvider. The provider hierarchy is correct—ElectronTRPCProviderwrapsAuthProvider, ensuring tRPC is available for the auth hydration flow.Also applies to: 12-20
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (1)
40-48: Subscription called unconditionally at component top-level.The
useSubscriptionhook is called outside of auseEffector conditional. While this follows React hooks rules (it's called unconditionally), verify thatelectronTrpc.auth.onTokenChanged.useSubscriptionproperly handles cleanup on unmount to avoid memory leaks.Also, note that this subscription will be active even for unauthenticated users viewing the loading spinner, which is fine but worth being aware of.
apps/desktop/src/renderer/routes/_authenticated/layout.tsx (1)
11-35: LGTM! Clean session-based authentication guard.The simplified authentication flow using
authClient.useSession()is straightforward and handles the loading, error, and unauthenticated states appropriately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/index.tsx`:
- Around line 29-34: The router.subscribe("onResolved", ...) handler currently
sends window.location.href to posthog.capture("$pageview") which can leak local
file paths in Electron dev and also risks duplicate subscriptions during HMR;
change the payload to use the route-only value (e.g., window.location.hash or
the event's toLocation.href) instead of window.location.href, and return/dispose
the subscription so HMR can clean it up (use the subscription handle returned by
router.subscribe("onResolved") and call its unsubscribe/dispose in the
module.hot.dispose or equivalent cleanup).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/index.tsx
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/index.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/index.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/index.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/index.tsx
🧠 Learnings (4)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/index.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/index.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/index.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/renderer/index.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/index.tsx (2)
apps/desktop/src/renderer/providers/ElectronTRPCProvider/index.ts (1)
electronQueryClient(3-3)apps/desktop/src/lib/trpc/index.ts (1)
router(47-47)
⏰ 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). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/renderer/index.tsx (2)
12-13: ConfirmelectronQueryClientis a singleton and is the correct TanStack Query client type.If
electronQueryClientis created on import, ensure it won’t be recreated per HMR reload (and that it’s aQueryClient, not a tRPC client). This avoids cache loss and subtle query duplication. Based on coding guidelines/learnings (renderer + IPC conventions).
20-27: Router context switch looks fine; verify downstream route context typing.
context: { queryClient: electronQueryClient }is a good integration point, but make sure all route loaders/components that readcontext.queryClientstill typecheck and don’t assume the oldqueryClientimport.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
saddlepaddle
force-pushed
the
auth-refactor-rebased
branch
from
c1c2050 to
c37dc6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/auth/index.ts`:
- Around line 88-93: The signOut publicProcedure currently swallows errors in
the empty catch when calling fs.unlink(TOKEN_FILE); update the catch to inspect
the caught error: if err.code === 'ENOENT' quietly ignore (file already absent),
otherwise log the error with context (include TOKEN_FILE and the error) using
the module's logger (or console.error if no logger exists) so permission/disk
errors are visible; keep the behavior of returning { success: true } after
handling/logging.
In `@apps/desktop/src/renderer/routes/_authenticated/layout.tsx`:
- Around line 24-25: The redirect flash happens because isSignedIn uses
!!session?.user which is false while authClient.useSession() is still hydrating
(session undefined); update the code that decides redirects to first check the
session loading state (either by using the hook's provided isLoading/isFetching
flag from authClient.useSession() or by explicitly checking session ===
undefined) and only perform redirect logic when loading is false and session is
resolved, e.g., compute isLoading from authClient.useSession() and replace
immediate !!session?.user-based redirects with a guard like if (!isLoading &&
!session?.user) to avoid flashing to /sign-in.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`:
- Around line 11-12: The code is reading activeOrganizationId from the wrong
nested path; update the access in CollectionsProvider by changing the variable
initialization that uses session?.session?.activeOrganizationId to read
session?.activeOrganizationId instead (look for the const activeOrganizationId
in CollectionsProvider component) and make the same fix in OrganizationDropdown
(WorkspaceSidebarHeader) where activeOrganizationId is similarly read; ensure
all uses reference session?.activeOrganizationId and not
session?.session?.activeOrganizationId.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (1)
22-38: Critical: Infinite loading spinner when no stored token exists.The hydration logic at lines 30-38 has a bug: if
storedTokenisnullorundefined(first-time user or after sign-out), the conditionstoredToken && !isHydratedis never true, sosetIsHydrated(true)is never called. The app will display a loading spinner indefinitely.Use
isFetchedfrom the query result to determine when the query has completed, regardless of whether a token was found:🐛 Proposed fix
- const { data: storedToken } = electronTrpc.auth.getStoredToken.useQuery( + const { data: storedToken, isFetched } = electronTrpc.auth.getStoredToken.useQuery( undefined, { refetchOnWindowFocus: false, refetchOnReconnect: false, }, ); useEffect(() => { - if (storedToken && !isHydrated) { - if (storedToken.token && storedToken.expiresAt) { + if (isFetched && !isHydrated) { + if (storedToken?.token && storedToken?.expiresAt) { setAuthToken(storedToken.token); refetchSession(); } setIsHydrated(true); } - }, [storedToken, isHydrated, refetchSession]); + }, [storedToken, isFetched, isHydrated, refetchSession]);apps/desktop/src/renderer/index.tsx (1)
29-33: Avoid sendingwindow.location.hrefto analytics; add HMR cleanup.This was previously flagged: in Electron development,
window.location.hrefexposes local file paths (e.g.,file:///Users/...#/route) which is PII. Additionally, the subscription lacks cleanup, causing duplicate$pageviewevents during HMR.Use
window.location.hashor the router event'stoLocation.hrefinstead, and store the unsubscribe function for HMR disposal.Proposed fix
-router.subscribe("onResolved", () => { +const unsub = router.subscribe("onResolved", () => { posthog.capture("$pageview", { - $current_url: window.location.href, + $current_url: window.location.hash || "/", }); }); + +if (import.meta.hot) { + import.meta.hot.dispose(() => unsub()); +}
🧹 Nitpick comments (12)
apps/desktop/src/renderer/react-query/projects/useReorderProjects.ts (1)
14-17: Consider parallel cache invalidation for better performance.The two invalidation calls are independent and could run concurrently.
♻️ Suggested optimization
return electronTrpc.projects.reorder.useMutation({ ...options, onSuccess: async (...args) => { - await utils.workspaces.getAllGrouped.invalidate(); - await utils.projects.getRecents.invalidate(); + await Promise.all([ + utils.workspaces.getAllGrouped.invalidate(), + utils.projects.getRecents.invalidate(), + ]); await options?.onSuccess?.(...args); }, });apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/page.tsx (1)
11-17: Consider inlining or extractingLoadingSpinner.Per coding guidelines, each file should contain one component. Since
LoadingSpinneris trivial, consider either inlining it directly in the return statement or extracting it to a shared location if used elsewhere.♻️ Option: Inline the spinner
-function LoadingSpinner() { - return ( - <div className="flex h-full w-full items-center justify-center"> - <Spinner className="size-5" /> - </div> - ); -} - function WorkspaceIndexPage() { // ... existing code ... - return <LoadingSpinner />; + return ( + <div className="flex h-full w-full items-center justify-center"> + <Spinner className="size-5" /> + </div> + ); }apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
66-70: Extract magic number to a named constant.The 10-minute state expiry duration is a magic number. Per coding guidelines, extract hardcoded values to named constants at module top for clarity and maintainability.
Suggested fix
At the top of the file, add:
const STATE_EXPIRY_MS = 10 * 60 * 1000; // 10 minutesThen update the cleanup logic:
- // Clean up old states (older than 10 minutes) - const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + // Clean up old states + const expiryThreshold = Date.now() - STATE_EXPIRY_MS; for (const [s, ts] of stateStore) { - if (ts < tenMinutesAgo) stateStore.delete(s); + if (ts < expiryThreshold) stateStore.delete(s); }apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts (1)
54-57: Align Authorization header handling with tRPC pattern — omit header when no token exists.The Electric SQL shapeOptions currently return an empty string
""whengetAuthToken()is null, which sends an Authorization header with an empty value. This differs from the tRPC client in the same file (line 37), which omits the header entirely by returning{}. Most servers expect either no Authorization header or a valid Bearer token—an empty Authorization header may be treated unexpectedly.Change the empty string to
undefinedto omit the header when unauthenticated, matching HTTP best practices and the existing tRPC pattern:Suggested fix
Authorization: () => { const token = getAuthToken(); - return token ? `Bearer ${token}` : ""; + return token ? `Bearer ${token}` : undefined; },apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/ProjectsSettings/ProjectsSettings.tsx (1)
16-20: Consider stabilizing the effect dependency.The
useEffectdepends ongroups, which may trigger on every query refetch even when data is semantically unchanged. If this causes unintended expansion resets, consider using a more stable dependency.Optional: More stable dependency
+ const groupIds = groups.map((g) => g.project.id).join(","); + // Expand all projects by default when groups are loaded useEffect(() => { if (groups.length > 0) { setExpandedProjects(new Set(groups.map((g) => g.project.id))); } - }, [groups]); + }, [groupIds]);apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts (1)
14-18: Consider parallel cache invalidation for better performance.The sequential
awaitcalls work correctly but could be parallelized since neither invalidation depends on the other.♻️ Optional: Parallelize invalidations
onSuccess: async (...args) => { - await utils.workspaces.getAll.invalidate(); - await utils.workspaces.getAllGrouped.invalidate(); + await Promise.all([ + utils.workspaces.getAll.invalidate(), + utils.workspaces.getAllGrouped.invalidate(), + ]); await options?.onSuccess?.(...args); },apps/desktop/src/renderer/routes/_authenticated/layout.tsx (1)
51-58: Consider adding error handler for menu subscription.The
onInitProgresssubscription has anonErrorhandler, but themenu.subscribesubscription does not. For consistency and to avoid silent failures:♻️ Add error handler
electronTrpc.menu.subscribe.useSubscription(undefined, { onData: (event) => { if (event.type === "open-settings") { const section = event.data.section || "account"; navigate({ to: `/settings/${section}` as "/settings/account" }); } }, + onError: (error) => { + console.error("[menu-subscription] Subscription error:", error); + }, });apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/OrganizationDropdown.tsx (2)
57-60: Consider adding error handling for sign-out operations.The sign-out flow has no error handling. If
authClient.signOut()throws, the user gets no feedback. Additionally,signOutMutation.mutate()is fire-and-forget without awaiting or handling potential failures.Per coding guidelines, errors should not be swallowed silently.
♻️ Suggested improvement
const handleSignOut = async () => { - await authClient.signOut(); - signOutMutation.mutate(); + try { + await authClient.signOut(); + signOutMutation.mutate(); + } catch (error) { + console.error("[auth/signOut] Sign out failed:", error); + } };
51-55: Consider error handling for organization switching.If
authClient.organization.setActivefails, the user receives no feedback. Consider wrapping in try-catch with appropriate error handling or toast notification.apps/desktop/src/renderer/react-query/workspaces/useDeleteWorktree.ts (1)
21-26: Consider removing redundant callback handlers.The
onSuccessandonErrorhandlers only forward tooptionscallbacks. Since...optionsis spread into the mutation config, these handlers are redundant unless you plan to add hook-specific logic later.♻️ Simplified version
return electronTrpc.workspaces.deleteWorktree.useMutation({ ...options, onSettled: async (...args) => { // Invalidate worktree queries to refresh the list await utils.workspaces.getWorktreesByProject.invalidate(); await options?.onSettled?.(...args); }, - onSuccess: async (...args) => { - await options?.onSuccess?.(...args); - }, - onError: async (...args) => { - await options?.onError?.(...args); - }, });apps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsx (1)
1-3: Inconsistent import path style.Line 2 uses the alias path
renderer/lib/electron-trpc, while Line 3 uses a relative path../../lib/trpc-client. Per coding guidelines, prefer alias paths consistently.Suggested fix
import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { electronTrpc } from "renderer/lib/electron-trpc"; -import { electronReactClient } from "../../lib/trpc-client"; +import { electronReactClient } from "renderer/lib/trpc-client";apps/desktop/src/renderer/react-query/presets/index.ts (1)
60-63: Consider parallelizing independent invalidations.The two
invalidate()calls are independent and could run concurrently.Optional optimization
onSuccess: async (...args) => { - await utils.settings.getTerminalPresets.invalidate(); - await utils.settings.getDefaultPreset.invalidate(); + await Promise.all([ + utils.settings.getTerminalPresets.invalidate(), + utils.settings.getDefaultPreset.invalidate(), + ]); await options?.onSuccess?.(...args); },
| signOut: publicProcedure.mutation(async () => { | ||
| await authService.signOut(); | ||
| try { | ||
| await fs.unlink(TOKEN_FILE); | ||
| } catch {} | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
Silent error swallowing in signOut.
The empty catch {} block silently swallows errors when unlinking the token file. Per coding guidelines, never swallow errors silently—at minimum log them with context. While the file may legitimately not exist (e.g., already signed out), other errors (permissions, disk issues) should be logged.
Suggested fix
signOut: publicProcedure.mutation(async () => {
try {
await fs.unlink(TOKEN_FILE);
- } catch {}
+ } catch (err) {
+ // ENOENT is expected if already signed out; log other errors
+ if ((err as NodeJS.ErrnoException).code !== "ENOENT") {
+ console.error("[auth/signOut] Failed to remove token file:", err);
+ }
+ }
return { success: true };
}),📝 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.
| signOut: publicProcedure.mutation(async () => { | |
| await authService.signOut(); | |
| try { | |
| await fs.unlink(TOKEN_FILE); | |
| } catch {} | |
| return { success: true }; | |
| }), | |
| signOut: publicProcedure.mutation(async () => { | |
| try { | |
| await fs.unlink(TOKEN_FILE); | |
| } catch (err) { | |
| // ENOENT is expected if already signed out; log other errors | |
| if ((err as NodeJS.ErrnoException).code !== "ENOENT") { | |
| console.error("[auth/signOut] Failed to remove token file:", err); | |
| } | |
| } | |
| return { success: true }; | |
| }), |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/auth/index.ts` around lines 88 - 93, The
signOut publicProcedure currently swallows errors in the empty catch when
calling fs.unlink(TOKEN_FILE); update the catch to inspect the caught error: if
err.code === 'ENOENT' quietly ignore (file already absent), otherwise log the
error with context (include TOKEN_FILE and the error) using the module's logger
(or console.error if no logger exists) so permission/disk errors are visible;
keep the behavior of returning { success: true } after handling/logging.
| const { data: session } = authClient.useSession(); | ||
| const isSignedIn = !!session?.user; |
There was a problem hiding this comment.
Potential redirect flash during session hydration.
The current check !!session?.user will evaluate to false while the session is loading (session === undefined), causing an immediate redirect to /sign-in before the session has a chance to hydrate.
Consider adding a loading state check to prevent the redirect flash:
🐛 Suggested fix to handle loading state
const { data: session } = authClient.useSession();
+ const isLoading = session === undefined;
const isSignedIn = !!session?.user;
const navigate = useNavigate();
const utils = electronTrpc.useUtils();
// ... subscriptions ...
+ if (isLoading) {
+ return null; // Or a loading spinner
+ }
+
if (!isSignedIn) {
return <Navigate to="/sign-in" replace />;
}🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/layout.tsx` around lines 24 -
25, The redirect flash happens because isSignedIn uses !!session?.user which is
false while authClient.useSession() is still hydrating (session undefined);
update the code that decides redirects to first check the session loading state
(either by using the hook's provided isLoading/isFetching flag from
authClient.useSession() or by explicitly checking session === undefined) and
only perform redirect logic when loading is false and session is resolved, e.g.,
compute isLoading from authClient.useSession() and replace immediate
!!session?.user-based redirects with a guard like if (!isLoading &&
!session?.user) to avoid flashing to /sign-in.
| const { data: session } = authClient.useSession(); | ||
| const activeOrganizationId = session?.session?.activeOrganizationId; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the authClient file mentioned in the imports
find . -type f -name "*auth-client*" | head -20Repository: superset-sh/superset
Length of output: 110
🏁 Script executed:
# Also look at the CollectionsProvider file to see the actual code context
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx | head -30Repository: superset-sh/superset
Length of output: 1150
🏁 Script executed:
# Search for other usages of authClient.useSession in the codebase to understand the pattern
rg "authClient\.useSession" --type ts --type tsx -A 2 -B 1Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Read the auth-client.ts file to understand the useSession implementation
cat -n apps/desktop/src/renderer/lib/auth-client.tsRepository: superset-sh/superset
Length of output: 1140
🏁 Script executed:
# Search for useSession usages with better file type
rg "useSession" --type ts -A 3 -B 1 | head -100Repository: superset-sh/superset
Length of output: 6920
🏁 Script executed:
# Search for activeOrganizationId usage in the codebase to understand the correct structure
rg "activeOrganizationId" --type ts | head -50Repository: superset-sh/superset
Length of output: 1789
🏁 Script executed:
# Look for session type definitions or interfaces
rg "session\?" --type ts -A 1 | grep -E "(user|organization|activeOrg)" | head -20Repository: superset-sh/superset
Length of output: 2126
🏁 Script executed:
# Look for type definitions or interfaces related to session in better-auth or the codebase
rg "interface.*[Ss]ession" --type ts | head -20Repository: superset-sh/superset
Length of output: 502
🏁 Script executed:
# Search for where session structure is documented or defined
rg "data.*session" --type ts -B 2 -A 2 | grep -E "(type|interface|const.*=)" | head -30Repository: superset-sh/superset
Length of output: 3106
🏁 Script executed:
# Check the packages/auth directory which might have server-side auth setup
find packages/auth -type f -name "*.ts" | head -10Repository: superset-sh/superset
Length of output: 144
🏁 Script executed:
# Check the auth server setup to understand session structure
head -150 packages/auth/src/server.tsRepository: superset-sh/superset
Length of output: 2792
🏁 Script executed:
# Also look for better-auth session type definitions in node_modules or docs
rg "organizationClient\|activeOrganizationId" --type ts -B 5 -A 5 | head -80Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify the same issue exists in OrganizationDropdown.tsx (the other file with this pattern)
grep -n "activeOrganizationId" apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/OrganizationDropdown.tsxRepository: superset-sh/superset
Length of output: 179
Fix the session property path for activeOrganizationId.
The property access session?.session?.activeOrganizationId has incorrect double nesting. Based on the Better Auth server configuration, activeOrganizationId is added as a direct property of the session object, not nested. The correct path is session?.activeOrganizationId, consistent with how other session properties like session?.user are accessed throughout the codebase.
Change line 12 from:
const activeOrganizationId = session?.session?.activeOrganizationId;
to:
const activeOrganizationId = session?.activeOrganizationId;
Note: The same issue exists in apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarHeader/OrganizationDropdown.tsx (line 37) and should be fixed there as well.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`
around lines 11 - 12, The code is reading activeOrganizationId from the wrong
nested path; update the access in CollectionsProvider by changing the variable
initialization that uses session?.session?.activeOrganizationId to read
session?.activeOrganizationId instead (look for the const activeOrganizationId
in CollectionsProvider component) and make the same fix in OrganizationDropdown
(WorkspaceSidebarHeader) where activeOrganizationId is similarly read; ensure
all uses reference session?.activeOrganizationId and not
session?.session?.activeOrganizationId.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts (1)
26-34: ThesubscribeStaticsubscription with emptyworkspaceIdwill be non-functional.The backend router implementation queries for a workspace by ID using
where(eq(workspaces.id, input.workspaceId))and returns an empty cleanup function when no workspace is found. Passing{ workspaceId: "" }will fail to find any workspace, causing the subscription to silently do nothing. The comment claims this "subscribes to all static port changes," but the backend doesn't support a global subscription—it only supports workspace-specific subscriptions. Provide a valid workspace ID or refactor the subscription logic to support the intended behavior.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/index.ts`:
- Around line 211-223: The protocol handler registered in protocol.handle for
PROTOCOL_SCHEME builds filePath using path.normalize which allows path
traversal; change it to resolve the full absolute paths (use path.resolve with
__dirname + "../renderer" and the requested pathname), verify the resolved
filePath is contained within the renderer root (e.g., compare
path.relative(rendererRoot, resolvedPath) or ensure
resolvedPath.startsWith(rendererRoot)), and if the check fails return a safe
error/404 response instead of fetching; also add try/catch around the net.fetch
to detect file-not-found or fetch errors and log a clear error via your logger
before returning an appropriate error response.
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 43-51: The electronTrpc.auth.onTokenChanged.useSubscription call
is missing error handling and the sign-out mutation doesn't clear the in-memory
token; add an onError handler to
electronTrpc.auth.onTokenChanged.useSubscription (consistent with
workspaces.onInitProgress) that logs/errors via your logger instead of
swallowing exceptions, and update the sign-out mutation's onSuccess callback
(the account settings mutation) to call setAuthToken(null) and
setIsHydrated(false) (and refetchSession if needed) so the in-memory authClient
is cleared when a user signs out.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx (1)
24-24: Duplicated constant with inconsistent value.
GITHUB_STATUS_STALE_TIMEis defined here as 5 minutes, butWorkspaceSidebar/WorkspaceListItem/constants.tsexports the same constant with a value of 30 seconds. This inconsistency could cause confusing caching behavior between different parts of the UI.Consider importing the shared constant instead of duplicating it.
♻️ Proposed fix
-const GITHUB_STATUS_STALE_TIME = 5 * 60 * 1000; // 5 minutes +import { GITHUB_STATUS_STALE_TIME } from "../../WorkspaceSidebar/WorkspaceListItem/constants";If the 5-minute value is intentional for this specific use case (lazy-loaded on hover), consider renaming the constant to differentiate it, or consolidate both usages to a single shared value.
apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (1)
73-78: Consider extracting magic numbers to named constants.The values
5and50are used for pagination logic. While the code is readable, extracting these to named constants at module level would align with coding guidelines.♻️ Suggested refactor
+const INITIAL_PROJECT_COUNT = 5; +const LOAD_MORE_COUNT = 50; + export function StartView() { // ... - const [visibleCount, setVisibleCount] = useState(50); + const [visibleCount, setVisibleCount] = useState(LOAD_MORE_COUNT); // ... - const hasMoreProjects = recentProjects.length > 5; + const hasMoreProjects = recentProjects.length > INITIAL_PROJECT_COUNT; const displayedProjects = showAllProjects ? recentProjects.slice(0, visibleCount) - : recentProjects.slice(0, 5); + : recentProjects.slice(0, INITIAL_PROJECT_COUNT);And update the reset logic on line 160:
- if (showAllProjects) setVisibleCount(50); + if (showAllProjects) setVisibleCount(LOAD_MORE_COUNT);apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (1)
3-4: Use consistent alias paths for imports.Line 3 uses the alias
renderer/lib/auth-client, but line 4 uses the relative path../../lib/electron-trpc. Per coding guidelines, prefer aliases for consistency.Suggested fix
import { authClient, setAuthToken } from "renderer/lib/auth-client"; -import { electronTrpc } from "../../lib/electron-trpc"; +import { electronTrpc } from "renderer/lib/electron-trpc";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/desktop/src/lib/trpc/routers/tasks/index.tsapps/desktop/src/lib/trpc/routers/user/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/main/lib/auth/auth.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/renderer/hooks/useWorkspaceShortcuts.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/react-query/workspaces/useOpenWorktree.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/page.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceInitEffects.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/BranchSwitcher/BranchSwitcher.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/ChangesHeader/ChangesHeader.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/FileItem/FileItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsx
💤 Files with no reviewable changes (5)
- apps/desktop/src/lib/trpc/routers/user/index.ts
- apps/desktop/src/lib/trpc/routers/tasks/index.ts
- apps/desktop/src/main/lib/auth/index.ts
- apps/desktop/src/main/lib/api-client.ts
- apps/desktop/src/main/lib/auth/auth.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/BranchSwitcher/BranchSwitcher.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/page.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx
- apps/desktop/src/renderer/hooks/useWorkspaceShortcuts.ts
- apps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
- apps/desktop/src/renderer/react-query/workspaces/useOpenWorktree.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/FileItem/FileItem.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceInitEffects.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/ChangesHeader/ChangesHeader.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.tsapps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsxapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/StartView/index.tsxapps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsxapps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
Applied to files:
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx
🧬 Code graph analysis (8)
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx (2)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/constants.ts (1)
GITHUB_STATUS_STALE_TIME(9-9)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsx (1)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsx (1)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
apps/desktop/src/main/index.ts (2)
apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts (1)
handleAuthCallback(57-70)apps/desktop/src/shared/constants.ts (1)
PROTOCOL_SCHEME(30-31)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts (1)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (1)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (2)
apps/desktop/src/renderer/lib/auth-client.ts (2)
authClient(21-33)setAuthToken(7-9)apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx (1)
apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)
⏰ 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). (6)
- GitHub Check: Deploy Web
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (20)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/WorkspaceHoverCard/WorkspaceHoverCard.tsx (1)
9-9: LGTM!The migration from
trpctoelectronTrpccorrectly follows the Electron IPC communication pattern using the dedicated tRPC client. The query parameters and enabled condition remain appropriately configured. Based on learnings, this aligns with the guideline to use tRPC for Electron interprocess communication.Also applies to: 26-30
apps/desktop/src/renderer/screens/main/components/StartView/StartTopBar.tsx (1)
1-7: LGTM!The migration from
trpctoelectronTrpcis correct and aligns with the PR's objective of standardizing Electron IPC-based tRPC communication across renderer components. The import pathrenderer/lib/electron-trpcis appropriate for the renderer process, and the query usage remains functionally equivalent. Based on learnings, this follows the established pattern for Electron interprocess communication.apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx (2)
16-16: LGTM on the migration toelectronTrpc.The import correctly uses the Electron-specific TRPC client for IPC communication, aligning with the project's architecture for Electron interprocess communication. Based on learnings, this follows the requirement to use tRPC as defined in
src/lib/trpcfor Electron IPC.
45-55: Query implementation is correct.The
electronTrpcquery usage properly:
- Guards with
enabledto prevent unnecessary requests until hover occurs- Only fetches for worktree workspaces with valid IDs
- Uses
staleTimeto avoid refetching fresh dataapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspacesListView.tsx (3)
8-8: LGTM!The import correctly uses
electronTrpcfrom the renderer's electron-trpc module, which aligns with the coding guidelines requiring tRPC for Electron IPC. Based on learnings, this is the correct pattern for Electron interprocess communication.
23-49: Clean migration to electronTrpc hooks.All tRPC hook usages are correctly migrated:
useUtils()for cache invalidationuseQuery()for data fetching with sensible defaultsuseQueries()for parallel worktree fetchinguseMutation()with proper error handling via toastThe cache invalidation in
onSuccessand error feedback inonErrorare well-implemented.
159-163: Safe usage ofMath.maxdue to grouping logic.While
Math.max(...[])returns-Infinity, the grouping logic at lines 137-146 ensures each group has at least one workspace before being added to the map, making this safe.apps/desktop/src/renderer/screens/main/components/StartView/index.tsx (5)
5-5: LGTM!The migration to
electronTrpccorrectly follows the coding guidelines for Electron IPC communication. The import path uses the configured alias as expected.
15-17: LGTM!The queries correctly use
electronTrpcfor Electron IPC communication, following the established pattern. The default empty array forrecentProjectsis good defensive coding that prevents undefined errors during initial render.
29-58: LGTM!The function has comprehensive error handling covering all cases (canceled, error in result, needsGitInit, success, and mutation errors). The early return pattern keeps the code readable.
60-71: LGTM!The function correctly uses object parameters
{ projectId }as per coding guidelines, and has appropriate error handling.
80-242: LGTM!The JSX is well-structured with:
- Proper accessibility attributes (
aria-labelledby,aria-label)- Correct
type="button"on all buttons- Loading states properly disabling interactive elements
- Clean dialog state management
apps/desktop/src/main/index.ts (3)
7-7: LGTM!The
netandprotocolimports are correctly added to support the new custom protocol handler.
9-12: LGTM!Auth functions correctly moved to tRPC router utilities, aligning with the guideline to use tRPC for Electron IPC. Based on learnings, this is the expected pattern.
43-53: LGTM!The refactor from
authService.handleAuthCallbackto the directhandleAuthCallbackfunction is clean. The function correctly handles success/error cases with appropriate logging.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts (2)
1-24: LGTM!The migration from
trpctoelectronTrpccorrectly follows the coding guidelines for Electron IPC communication. The import uses the proper alias path, and all queries are consistently updated to use the Electron-backed tRPC client.
36-52: LGTM!The ports query and subscription are correctly migrated to use
electronTrpc. The subscription properly handles add/remove events and updates the Zustand store, following the project's state management guidelines.apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx (3)
21-26: LGTM!Using
isSuccessto gate hydration logic correctly handles both token-present and no-token scenarios, addressing the previous infinite loading issue.
28-40: LGTM!The hydration logic correctly handles all cases: waits for query completion via
isSuccess, conditionally sets the token if present, and always marks hydration complete regardless of token existence.
53-63: LGTM!The conditional rendering correctly shows a loading state during hydration and renders children directly once complete. The approach aligns with the documented flow.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Register custom protocol to serve app files in production | ||
| // This provides a stable origin (superset://app or superset-dev://app) for Better Auth CORS | ||
| if (process.env.NODE_ENV !== "development") { | ||
| protocol.handle(PROTOCOL_SCHEME, (request) => { | ||
| // Parse URL to extract pathname (e.g., superset://app/index.html#/ -> /index.html) | ||
| const parsedUrl = new URL(request.url); | ||
| const pathname = parsedUrl.pathname; | ||
| const filePath = path.normalize( | ||
| path.join(__dirname, "../renderer", pathname), | ||
| ); | ||
| return net.fetch(`file://${filePath}`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Path traversal vulnerability in protocol handler.
The current implementation doesn't validate that the resolved path stays within the renderer directory. A malicious URL like superset://app/../main/index.js could access files outside the intended directory since path.normalize doesn't prevent directory traversal.
🔒 Proposed fix to validate path containment
if (process.env.NODE_ENV !== "development") {
+ const rendererDir = path.join(__dirname, "../renderer");
protocol.handle(PROTOCOL_SCHEME, (request) => {
- // Parse URL to extract pathname (e.g., superset://app/index.html#/ -> /index.html)
const parsedUrl = new URL(request.url);
const pathname = parsedUrl.pathname;
- const filePath = path.normalize(
- path.join(__dirname, "../renderer", pathname),
- );
+ const filePath = path.resolve(rendererDir, pathname.slice(1) || "index.html");
+
+ // Prevent path traversal - ensure path stays within renderer directory
+ if (!filePath.startsWith(rendererDir)) {
+ return new Response("Forbidden", { status: 403 });
+ }
+
return net.fetch(`file://${filePath}`);
});
}Additionally, consider adding error handling for file-not-found scenarios to provide better debugging information.
📝 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.
| // Register custom protocol to serve app files in production | |
| // This provides a stable origin (superset://app or superset-dev://app) for Better Auth CORS | |
| if (process.env.NODE_ENV !== "development") { | |
| protocol.handle(PROTOCOL_SCHEME, (request) => { | |
| // Parse URL to extract pathname (e.g., superset://app/index.html#/ -> /index.html) | |
| const parsedUrl = new URL(request.url); | |
| const pathname = parsedUrl.pathname; | |
| const filePath = path.normalize( | |
| path.join(__dirname, "../renderer", pathname), | |
| ); | |
| return net.fetch(`file://${filePath}`); | |
| }); | |
| } | |
| // Register custom protocol to serve app files in production | |
| // This provides a stable origin (superset://app or superset-dev://app) for Better Auth CORS | |
| if (process.env.NODE_ENV !== "development") { | |
| const rendererDir = path.join(__dirname, "../renderer"); | |
| protocol.handle(PROTOCOL_SCHEME, (request) => { | |
| const parsedUrl = new URL(request.url); | |
| const pathname = parsedUrl.pathname; | |
| const filePath = path.resolve(rendererDir, pathname.slice(1) || "index.html"); | |
| // Prevent path traversal - ensure path stays within renderer directory | |
| if (!filePath.startsWith(rendererDir)) { | |
| return new Response("Forbidden", { status: 403 }); | |
| } | |
| return net.fetch(`file://${filePath}`); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/index.ts` around lines 211 - 223, The protocol handler
registered in protocol.handle for PROTOCOL_SCHEME builds filePath using
path.normalize which allows path traversal; change it to resolve the full
absolute paths (use path.resolve with __dirname + "../renderer" and the
requested pathname), verify the resolved filePath is contained within the
renderer root (e.g., compare path.relative(rendererRoot, resolvedPath) or ensure
resolvedPath.startsWith(rendererRoot)), and if the check fails return a safe
error/404 response instead of fetching; also add try/catch around the net.fetch
to detect file-not-found or fetch errors and log a clear error via your logger
before returning an appropriate error response.
| electronTrpc.auth.onTokenChanged.useSubscription(undefined, { | ||
| onData: (data) => { | ||
| if (data?.token && data?.expiresAt) { | ||
| setAuthToken(data.token); | ||
| setIsHydrated(true); | ||
| refetchSession(); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the onTokenChanged subscription implementation and related code
# Find files defining onTokenChanged in the backend/main process
echo "=== Searching for onTokenChanged subscription definition ==="
rg -n "onTokenChanged" --type=ts --type=tsx -g '!**/node_modules/**' -B2 -A5
echo -e "\n=== Searching for sign-out and token clearing logic ==="
rg -n "signOut|sign-out|logout|clearToken|clear.*token" --type=ts --type=tsx -g '!**/node_modules/**' -g '!**/*.test.*' -B2 -A3
echo -e "\n=== Checking AuthProvider component ==="
fd -t f "AuthProvider.tsx" --type f | xargs cat -nRepository: superset-sh/superset
Length of output: 2709
🏁 Script executed:
#!/bin/bash
# Check the tRPC router setup for auth to understand the subscription
echo "=== Looking for tRPC auth router setup ==="
find . -type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "onTokenChanged|auth.*router|createTRPCRouter" | grep -v node_modules | head -20Repository: superset-sh/superset
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Find tRPC observable usage and subscription patterns
echo "=== Searching for observable pattern usage in subscriptions ==="
rg -n "observable|Observable|isObservable" --type=ts --type=tsx -g '!**/node_modules/**' -B2 -A3
echo -e "\n=== Checking for useSubscription implementations ==="
rg -n "useSubscription" --type=ts --type=tsx -g '!**/node_modules/**' -B1 -A8Repository: superset-sh/superset
Length of output: 254
🏁 Script executed:
#!/bin/bash
# Read the auth router implementation
cat -n ./apps/desktop/src/lib/trpc/routers/auth/index.ts
echo -e "\n=== Searching for sign-out implementation ==="
find . -path ./node_modules -prune -o -type f \( -name "*.ts" -o -name "*.tsx" \) -print | xargs grep -l "signOut\|sign-out\|logout" 2>/dev/null | grep -E "(auth|sign)" | head -10Repository: superset-sh/superset
Length of output: 3486
🏁 Script executed:
#!/bin/bash
# Look at the structure of trpc routers and how subscriptions work
ls -la ./apps/desktop/src/lib/trpc/routers/
echo -e "\n=== Check for error handling in useSubscription ==="
find . -path ./node_modules -prune -o -type f \( -name "*.ts" -o -name "*.tsx" \) -print | xargs grep -n "onError" 2>/dev/null | grep -i "subscription\|trpc" | head -20Repository: superset-sh/superset
Length of output: 3511
🏁 Script executed:
#!/bin/bash
# Find all uses of auth router subscription to understand error handling patterns
find . -path ./node_modules -prune -o -type f \( -name "*.ts" -o -name "*.tsx" \) -print | xargs grep -B5 -A10 "\.useSubscription" 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 10132
🏁 Script executed:
#!/bin/bash
# Check the auth-functions to understand token and event handling
cat -n ./apps/desktop/src/lib/trpc/routers/auth/utils/auth-functions.ts 2>/dev/null || echo "File not found, searching for it..."
find . -path ./node_modules -prune -o -name "auth-functions*" -type f -print
echo -e "\n=== Check sign-out flow in the UI ==="
cat -n ./apps/desktop/src/renderer/routes/_authenticated/settings/account/page.tsx | head -100Repository: superset-sh/superset
Length of output: 6148
🏁 Script executed:
#!/bin/bash
# Look for where authEvents.emit is called with "token-saved"
rg -n "authEvents\." --type=ts --type=tsx -B2 -A2 | head -50
echo -e "\n=== Check if sign-out triggers any token event ==="
rg -n "token-saved\|tokenSaved\|token-cleared\|tokenCleared" --type=ts --type=tsx -B2 -A2Repository: superset-sh/superset
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Check if authClient.useSession is invalidated or refetched on sign-out
grep -r "invalidate\|refetch" ./apps/desktop/src/renderer/routes/_authenticated/settings/account/page.tsx -B2 -A2
echo -e "\n=== Check if there's token cleanup in other places ==="
rg "setAuthToken.*null" --type=ts --type=tsx -B2 -A2
echo -e "\n=== Check the auth-client to understand session handling ==="
find . -path ./node_modules -prune -o -name "auth-client*" -type f -print | head -5Repository: superset-sh/superset
Length of output: 273
🏁 Script executed:
#!/bin/bash
# Look at the electron-trpc implementation to understand error handling
cat -n ./apps/desktop/src/lib/electron-trpc.ts 2>/dev/null | head -80 || cat -n ./apps/desktop/src/lib/trpc/index.ts 2>/dev/null | head -80
echo -e "\n=== Check if useSubscription supports onError on this implementation ==="
rg -n "useSubscription" ./apps/desktop/src/lib -B2 -A10 | head -60Repository: superset-sh/superset
Length of output: 137
🏁 Script executed:
#!/bin/bash
# Check auth-client implementation
cat -n ./apps/desktop/src/renderer/lib/auth-client.ts | head -100
echo -e "\n=== Look for electron-trpc setup ==="
find ./apps/desktop/src/lib -name "*.ts" -o -name "*.tsx" | xargs ls -la
echo -e "\n=== Check electron-trpc configuration ==="
cat -n ./apps/desktop/src/lib/electron-trpc.ts 2>/dev/null | head -80Repository: superset-sh/superset
Length of output: 9404
🏁 Script executed:
#!/bin/bash
# Check if signOut mutation has any additional cleanup
rg -n "signOut\|onSuccess\|onError" ./apps/desktop/src/renderer/routes/_authenticated/settings/account/ -B2 -A5
echo -e "\n=== Search for where token is cleared in renderer ==="
rg -n "setAuthToken" --type=ts --type=tsx -B2 -A2Repository: superset-sh/superset
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check the electron-trpc file itself
cat -n ./apps/desktop/src/lib/trpc/index.ts
echo -e "\n=== Look for electron-trpc in renderer ==="
find ./apps/desktop/src/renderer -name "*electron*" -o -name "*trpc*" | head -10
echo -e "\n=== Check how signOut is handled in the renderer ==="
rg -n "signOut\|sign-out" ./apps/desktop/src/renderer --type=ts --type=tsx -B3 -A5 | head -80Repository: superset-sh/superset
Length of output: 1929
🏁 Script executed:
#!/bin/bash
# Check electron-trpc.ts and trpc-client.ts
echo "=== electron-trpc.ts ==="
cat -n ./apps/desktop/src/renderer/lib/electron-trpc.ts
echo -e "\n=== trpc-client.ts ==="
cat -n ./apps/desktop/src/renderer/lib/trpc-client.ts | head -80Repository: superset-sh/superset
Length of output: 1495
🏁 Script executed:
#!/bin/bash
# Search for all sign-out related code in renderer
find ./apps/desktop/src/renderer -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "signOut\|sign-out" {} \;
echo -e "\n=== Check the account settings again for complete signOut flow ==="
cat -n ./apps/desktop/src/renderer/routes/_authenticated/settings/account/page.tsx | tail -20Repository: superset-sh/superset
Length of output: 953
🏁 Script executed:
#!/bin/bash
# Verify if there's any cleanup after signOut mutation completes
cat -n ./apps/desktop/src/renderer/routes/_authenticated/settings/account/page.tsx
echo -e "\n=== Check if there's onSuccess/onError for signOut mutation ==="
rg -n "signOutMutation\|auth\.signOut" ./apps/desktop/src/renderer/routes/_authenticated/settings/account/ -B2 -A10Repository: superset-sh/superset
Length of output: 2839
🏁 Script executed:
#!/bin/bash
# Check if there's any invalidate/refetch on signOut success in a hook or similar
rg -n "invalidate.*auth\|refetch.*auth\|signOut.*invalidate" ./apps/desktop/src/renderer -A3 -B2
echo -e "\n=== Verify if token should be cleared on sign-out ==="
grep -r "setAuthToken" ./apps/desktop/src/renderer -B2 -A2 | head -40
echo -e "\n=== Check if there are any useEffect hooks in AuthProvider that respond to session changes ==="
cat -n ./apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx | grep -A20 "useEffect"Repository: superset-sh/superset
Length of output: 4498
Add error handling and fix incomplete sign-out flow.
The subscription lacks an onError handler, which violates the coding guideline to never silently swallow errors. This is inconsistent with other subscriptions in the codebase (e.g., workspaces.onInitProgress) that include error handlers.
Additionally, sign-out is incomplete: the signOut mutation (account settings page) deletes the token file but does NOT clear the in-memory token via setAuthToken(null). The onTokenChanged subscription only listens to "token-saved" events from the backend; sign-out doesn't emit this event. This means users remain logged in to the in-memory authClient even after signing out. The mutation's onSuccess callback must explicitly clear the token.
Suggested fix
electronTrpc.auth.onTokenChanged.useSubscription(undefined, {
onData: (data) => {
if (data?.token && data?.expiresAt) {
setAuthToken(data.token);
setIsHydrated(true);
refetchSession();
}
},
+ onError: (error) => {
+ console.error("[AuthProvider/onTokenChanged] Subscription error:", error);
+ },
});Also update the sign-out mutation in account settings:
const signOutMutation = electronTrpc.auth.signOut.useMutation({
- onSuccess: () => toast.success("Signed out"),
+ onSuccess: () => {
+ setAuthToken(null);
+ refetchSession();
+ toast.success("Signed out");
+ },
});🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx` around
lines 43 - 51, The electronTrpc.auth.onTokenChanged.useSubscription call is
missing error handling and the sign-out mutation doesn't clear the in-memory
token; add an onError handler to
electronTrpc.auth.onTokenChanged.useSubscription (consistent with
workspaces.onInitProgress) that logs/errors via your logger instead of
swallowing exceptions, and update the sign-out mutation's onSuccess callback
(the account settings mutation) to call setAuthToken(null) and
setIsHydrated(false) (and refetchSession if needed) so the in-memory authClient
is cleared when a user signs out.
saddlepaddle
force-pushed
the
auth-refactor-rebased
branch
from
99f2b85 to
d3c5bbc
Compare
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.