refactor(desktop): remove Cloudflare Electric proxy, revert to API proxy#1501
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes the electric-proxy Cloudflare Worker and its JWT auth/where-clause logic, centralizes Authorization header handling in desktop collections, and updates NEXT_PUBLIC_ELECTRIC_URL defaults and related build/setup configuration to point directly at the Electric API endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Desktop as Desktop App
participant Proxy as electric-proxy (Worker)
participant JWKS as JWKS
participant Electric as Electric API
Desktop->>Proxy: GET /v1/shape?table=... (Authorization: Bearer <token>)
Proxy->>JWKS: verifyJWT(token)
JWKS-->>Proxy: claims (organizationIds)
Proxy->>Electric: GET /v1/shape?... (with WHERE clause & optional secret)
Electric-->>Proxy: shape data
Proxy-->>Desktop: proxied shape data (Vary: Authorization)
sequenceDiagram
participant Desktop as Desktop App
participant Electric as Electric API
Desktop->>Electric: GET /api/electric/v1/shape?table=... (Authorization: Bearer <token> via electricHeaders)
Electric-->>Desktop: shape data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts (1)
332-335:⚠️ Potential issue | 🟡 MinorStale JSDoc: references
getAuthToken()but Electric headers now usegetJwt().Proposed fix
/** * Get collections for an organization, creating them if needed. * Collections are cached per org for instant switching. - * Auth token is read dynamically via getAuthToken() - no need to pass it. + * JWT is read dynamically via getJwt() for Electric sync headers. + * API client uses getAuthToken() for tRPC calls. */
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx`:
- Around line 36-52: The onTokenChanged subscription handler
(electronTrpc.auth.onTokenChanged.useSubscription) resets auth state and calls
fetchJwt() on token rotation but never re-enables the JWT rendering gate; add a
call to setNeedsJwt(true) in the branch where data?.token && data?.expiresAt is
handled (before calling fetchJwt()) so the gate (needsJwt && !isJwtReady)
prevents children rendering until the JWT is cached; update the handler near
setAuthToken/clearJwt/setIsHydrated/refetchSession to call setNeedsJwt(true)
prior to fetchJwt().
In
`@apps/desktop/src/renderer/providers/AuthProvider/hooks/useJwtRefresh/useJwtRefresh.ts`:
- Around line 46-48: The catch block in useJwtRefresh currently only calls
setJwt(null) when fetchJwt throws, leaving isReady false and causing
AuthProvider (isJwtReady) to hang; update the catch handler in the fetchJwt flow
to also call setIsReady(true) (and keep setJwt(null)) so the initial failure
still flips readiness; locate the error path around fetchJwt, setJwt and
setIsReady in useJwtRefresh and add the setIsReady(true) call in that catch
branch.
- Around line 38-44: The refresh loop can spin when expiresAt - Date.now() -
JWT_REFRESH_BUFFER_MS <= 0; update the logic in useJwtRefresh so the computed
refreshIn for scheduling fetchJwt is floored to a sensible minimum (e.g.,
MIN_REFRESH_DELAY_MS = 30_000) instead of allowing 0; specifically, after
computing refreshIn use something like const delay = Math.max(refreshIn,
MIN_REFRESH_DELAY_MS) and pass delay to setTimeout, keeping the existing
clearTimeout(refreshTimerRef.current) and using fetchJwt as before (symbols:
expiresAt, JWT_REFRESH_BUFFER_MS, refreshIn, refreshTimerRef.current, fetchJwt).
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`:
- Around line 69-74: The current electricHeaders.Authorization function builds
an Authorization string that becomes "Bearer " when getJwt() returns null;
update the electricHeaders implementation so Authorization returns a proper
bearer string only when a token exists (e.g., return `Bearer ${token}` when
token is truthy) and otherwise return an empty value acceptable to the Electric
client (prefer undefined or empty string depending on client support) to avoid
sending the malformed "Bearer " header; locate and change the Authorization
arrow function in electricHeaders and adjust to return token ? `Bearer ${token}`
: undefined (or empty string if the client requires it).
🧹 Nitpick comments (1)
apps/desktop/src/renderer/providers/AuthProvider/hooks/useJwtRefresh/useJwtRefresh.ts (1)
7-14:atobdoes not handle base64url encoding used by JWTs.JWTs use base64url (with
-/_instead of+//and no padding).atobexpects standard base64 and will throw for payloads containing those characters. Thetry/catchprovides a safe fallback (returnsnull, skipping auto-refresh), so this won't crash — but refresh scheduling would silently stop working for affected tokens.A small normalization before decoding would make this robust:
Proposed fix
function parseJwtExp(token: string): number | null { try { - const payload = JSON.parse(atob(token.split(".")[1])); + const base64Url = token.split(".")[1]; + const base64 = base64Url.replace(/-/g, "+").replace(/_/g, "/"); + const payload = JSON.parse(atob(base64)); return typeof payload.exp === "number" ? payload.exp * 1000 : null; } catch {
| electronTrpc.auth.onTokenChanged.useSubscription(undefined, { | ||
| onData: async (data) => { | ||
| if (data?.token && data?.expiresAt) { | ||
| setAuthToken(null); | ||
| clearJwt(); | ||
| await authClient.signOut({ fetchOptions: { throw: false } }); | ||
| setAuthToken(data.token); | ||
| setIsHydrated(true); | ||
| refetchSession(); | ||
| fetchJwt(); | ||
| } else if (data === null) { | ||
| setAuthToken(null); | ||
| clearJwt(); | ||
| refetchSession(); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
needsJwt is not set to true in the onTokenChanged handler, so JWT gating won't re-engage on token rotation.
When a new token arrives via the subscription (lines 38-46), fetchJwt() is called but setNeedsJwt(true) is not. Since the rendering gate on line 54 is needsJwt && !isJwtReady, children may render briefly before the JWT is cached — meaning collections could issue requests with a null JWT during that window.
If this is intentional (tolerable brief gap during token rotation), a short comment would help future readers. Otherwise, adding setNeedsJwt(true) before fetchJwt() would re-engage the gate.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx` around
lines 36 - 52, The onTokenChanged subscription handler
(electronTrpc.auth.onTokenChanged.useSubscription) resets auth state and calls
fetchJwt() on token rotation but never re-enables the JWT rendering gate; add a
call to setNeedsJwt(true) in the branch where data?.token && data?.expiresAt is
handled (before calling fetchJwt()) so the gate (needsJwt && !isJwtReady)
prevents children rendering until the JWT is cached; update the handler near
setAuthToken/clearJwt/setIsHydrated/refetchSession to call setNeedsJwt(true)
prior to fetchJwt().
| if (expiresAt) { | ||
| const refreshIn = Math.max( | ||
| expiresAt - Date.now() - JWT_REFRESH_BUFFER_MS, | ||
| 0, | ||
| ); | ||
| if (refreshTimerRef.current) clearTimeout(refreshTimerRef.current); | ||
| refreshTimerRef.current = setTimeout(fetchJwt, refreshIn); |
There was a problem hiding this comment.
Tight refresh loop when token lifetime ≤ JWT_REFRESH_BUFFER_MS.
If the server issues tokens with a lifetime shorter than 5 minutes, refreshIn computes to 0 every time, and setTimeout(fetchJwt, 0) fires immediately — creating a hot loop of token requests. Consider adding a minimum delay floor (e.g., 30 seconds) to prevent this.
Proposed fix
+ const MIN_REFRESH_INTERVAL_MS = 30_000;
const refreshIn = Math.max(
expiresAt - Date.now() - JWT_REFRESH_BUFFER_MS,
- 0,
+ MIN_REFRESH_INTERVAL_MS,
);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/providers/AuthProvider/hooks/useJwtRefresh/useJwtRefresh.ts`
around lines 38 - 44, The refresh loop can spin when expiresAt - Date.now() -
JWT_REFRESH_BUFFER_MS <= 0; update the logic in useJwtRefresh so the computed
refreshIn for scheduling fetchJwt is floored to a sensible minimum (e.g.,
MIN_REFRESH_DELAY_MS = 30_000) instead of allowing 0; specifically, after
computing refreshIn use something like const delay = Math.max(refreshIn,
MIN_REFRESH_DELAY_MS) and pass delay to setTimeout, keeping the existing
clearTimeout(refreshTimerRef.current) and using fetchJwt as before (symbols:
expiresAt, JWT_REFRESH_BUFFER_MS, refreshIn, refreshTimerRef.current, fetchJwt).
| } catch { | ||
| setJwt(null); | ||
| } |
There was a problem hiding this comment.
isReady is never set to true if the initial fetchJwt throws, causing an infinite spinner.
The catch block calls setJwt(null) but omits setIsReady(true). Since AuthProvider gates rendering on isJwtReady (which maps to isReady), a network error during the first token fetch will leave the app stuck on the loading spinner indefinitely.
Compare with lines 28-30 where the "no token" path correctly sets isReady.
Proposed fix
} catch {
setJwt(null);
+ setIsReady(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.
| } catch { | |
| setJwt(null); | |
| } | |
| } catch { | |
| setJwt(null); | |
| setIsReady(true); | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/providers/AuthProvider/hooks/useJwtRefresh/useJwtRefresh.ts`
around lines 46 - 48, The catch block in useJwtRefresh currently only calls
setJwt(null) when fetchJwt throws, leaving isReady false and causing
AuthProvider (isJwtReady) to hang; update the catch handler in the fetchJwt flow
to also call setIsReady(true) (and keep setJwt(null)) so the initial failure
still flips readiness; locate the error path around fetchJwt, setJwt and
setIsReady in useJwtRefresh and add the setIsReady(true) call in that catch
branch.
saddlepaddle
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.superset/setup.sh (1)
119-121:⚠️ Potential issue | 🟡 MinorStale Caddy dependency check — consider removing.
Lines 119-121 still warn when
caddyis not found ("HTTP/2 proxy for Electric won't work"), but this PR removes the Caddy-based Electric proxy entirely. This warning is now misleading for developers setting up their workspace.Proposed fix
- if ! command -v caddy &> /dev/null; then - warn "caddy not found — HTTP/2 proxy for Electric won't work (Run: brew install caddy && caddy trust)" - fi
Each of 12+ Electric collections was independently calling authClient.token() (a network request) on every sync cycle. Centralizes JWT lifecycle in a useJwtRefresh hook mounted in AuthProvider, with synchronous getJwt() for collections to read.
Remove the Cloudflare Worker Electric proxy and all JWT-based auth for Electric SQL. Revert to routing Electric requests through the Vercel API proxy (/api/electric/) with session-based bearer auth. - Delete apps/electric-proxy/ package entirely - Remove JWT token caching (useJwtRefresh hook, getJwt/setJwt, jwtClient) - Revert AuthProvider to pre-JWT state - Point collections at NEXT_PUBLIC_API_URL/api/electric/ instead of NEXT_PUBLIC_ELECTRIC_URL - Remove NEXT_PUBLIC_ELECTRIC_URL from env, Vite config, CSP, CI, turbo - Remove dev:caddy script (no longer needed) - Clean up setup.sh (remove electric-proxy .dev.vars, Caddyfile gen, CADDY_ELECTRIC_PORT/ELECTRIC_PROXY_PORT)
Caddy is needed for HTTP/2 in local dev to avoid the browser's 6-connection limit with 10+ Electric SSE streams. Update it to proxy to the API server (which handles session auth) instead of the removed Cloudflare Worker.
saddlepaddle
force-pushed
the
fix-electric-token
branch
from
3c68ed8 to
ee492e5
Compare
Client code shouldn't know the proxy path. The env var now points directly at the Electric endpoint so collections.ts just appends /v1/shape, matching the old pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`:
- Line 26: The current construction of electricUrl by concatenating
env.NEXT_PUBLIC_ELECTRIC_URL risks double paths; update collections.ts to build
the URL safely using the URL API (e.g., set electricUrl = new
URL("/api/electric/v1/shape", env.NEXT_PUBLIC_ELECTRIC_URL).href) and/or tighten
the env schema for NEXT_PUBLIC_ELECTRIC_URL to be origin-only (validate via new
URL(envVal).origin === envVal or a zod refine) so you never append to a value
that already contains a path; reference electricUrl and
env.NEXT_PUBLIC_ELECTRIC_URL when making these changes.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/index.html (1)
14-18:%NEXT_PUBLIC_ELECTRIC_URL%in CSP is now redundant with%NEXT_PUBLIC_API_URL%.Since
NEXT_PUBLIC_ELECTRIC_URLnow defaults to the same origin asNEXT_PUBLIC_API_URL(https://api.superset.sh), the separate CSP placeholder is redundant. It's harmless, but you could simplify by removing it fromconnect-srcif Electric requests always go through the API origin going forward.
|
|
||
| const columnMapper = snakeCamelMapper(); | ||
| const electricUrl = `${env.NEXT_PUBLIC_ELECTRIC_URL}/v1/shape`; | ||
| const electricUrl = `${env.NEXT_PUBLIC_ELECTRIC_URL}/api/electric/v1/shape`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "NEXT_PUBLIC_ELECTRIC_URL" --type-add 'env:*.env*' -g '*.env*' -g '*.ts' -g '*.tsx' -g '*.json' -g '*.sh' -C2Repository: superset-sh/superset
Length of output: 2502
🏁 Script executed:
# Also check if there are any env template files or documentation
find . -type f \( -name ".env*" -o -name "*.example" -o -name "*env*template*" \) | head -20Repository: superset-sh/superset
Length of output: 78
🏁 Script executed:
# Check the collections.ts file to see the full context
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | head -40Repository: superset-sh/superset
Length of output: 1598
Add validation to ensure NEXT_PUBLIC_ELECTRIC_URL is origin-only or use safe URL construction.
The env var is validated only as z.url() with default "https://api.superset.sh", but this allows misconfiguration with a path (e.g., https://api.superset.sh/api). Appending /api/electric/v1/shape directly would then produce a malformed URL like https://api.superset.sh/api/api/electric/v1/shape. Either restrict validation to origin-only values or use safe URL construction (e.g., new URL("/api/electric/v1/shape", env.NEXT_PUBLIC_ELECTRIC_URL).href).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`
at line 26, The current construction of electricUrl by concatenating
env.NEXT_PUBLIC_ELECTRIC_URL risks double paths; update collections.ts to build
the URL safely using the URL API (e.g., set electricUrl = new
URL("/api/electric/v1/shape", env.NEXT_PUBLIC_ELECTRIC_URL).href) and/or tighten
the env schema for NEXT_PUBLIC_ELECTRIC_URL to be origin-only (validate via new
URL(envVal).origin === envVal or a zod refine) so you never append to a value
that already contains a path; reference electricUrl and
env.NEXT_PUBLIC_ELECTRIC_URL when making these changes.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
CSP connect-src with a path doesn't match sub-paths in Chromium. Extract just the origin (scheme+host+port) for CSP whitelisting.
1 participant
Summary
apps/electric-proxy/) and all JWT-based auth for Electric SQL/api/electric/) with session-based bearer authNEXT_PUBLIC_ELECTRIC_URLenv var, Caddyfile,dev:caddyscript, and all related config from Vite, CSP, CI, turbo, and setup.shauth-client.ts(removejwtClient,getJwt/setJwt) andAuthProvider(removeuseJwtRefreshhook)Test plan
bun run typecheck --filter=@superset/desktoppassesbun run lint:fixcleanbun test— 1213 pass, 0 failSummary by CodeRabbit