fix: move api calls to backend for better security#4
Conversation
❌ Deploy Preview for gnosis-split failed.
|
WalkthroughAdds a server-side Supabase singleton, Zod validation schemas, new SvelteKit REST API endpoints for splits (collection and item CRUD + participants PATCH), migrates client storage to call those APIs (removing realtime Supabase usage), updates call sites to pass wallet/address, and changes build/config (Bun, Netlify, PWA disabled). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as SvelteKit API
participant DB as Supabase DB
rect rgb(220,255,235)
Note over Client,API: Fetch splits scoped to an address
Client->>API: GET /api/splits?address=0x...
API->>API: getSupabase() (singleton)
API->>DB: SELECT * FROM splits WHERE payer_address=0x... ORDER BY created_at DESC
DB-->>API: rows (snake_case)
API->>API: map rows → Split (camelCase)
API-->>Client: 200 JSON
end
rect rgb(255,240,220)
Note over Client,API: Create split
Client->>API: POST /api/splits {SplitCreate}
API->>API: validate with SplitCreateSchema
API->>DB: INSERT mapped fields
DB-->>API: inserted row
API->>API: map row → Split (camelCase)
API-->>Client: 201 JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (6)
src/lib/supabase.ts (1)
30-30: Consider documenting the removal of real-time subscriptions.The
unsubscribefunction now returns a no-op. If real-time updates were previously a feature, consider adding a comment or documenting this behavioral change for consumers of this store.src/routes/api/splits/+server.ts (1)
17-27: Extract duplicate mapping logic to a helper function.The DB-to-Split mapping logic is duplicated between GET and POST handlers. Consider extracting it to improve maintainability:
function mapRowToSplit(row: Database['public']['Tables']['splits']['Row']): Split { return { id: row.id, description: row.description, totalAmount: row.total_amount, date: row.date, payerAddress: row.payer_address, participants: row.participants as unknown as Split['participants'], payments: row.payments as unknown as Split['payments'], createdAt: row.created_at as unknown as Split['createdAt'], sourceTxId: row.source_tx_id || undefined }; }This would also reduce duplication with
src/routes/api/splits/[id]/+server.ts.Also applies to: 58-68
src/lib/storage.ts (2)
20-27: Inconsistent error handling patterns across functions.The error handling varies significantly:
getSplits: catches and returns[]saveSplit: throws on error (no try/catch)getSplit: catches and returnsundefinedupdateSplit/deleteSplit: throws on error (no try/catch)Consider standardizing the approach—either all functions throw on failure (letting callers handle errors), or all functions return a result type that indicates success/failure.
Also applies to: 47-54, 63-70, 94-100, 117-121
150-153: Redundant error logging before throw.Logging and then immediately throwing with the same message is redundant. The caller will likely handle or log the thrown error.
if (!response.ok) { - console.error('Failed to update participants'); throw new Error('Failed to update participants'); }src/routes/api/splits/[id]/+server.ts (2)
7-7: Unnecessary throws for route parameters.SvelteKit guarantees
params.idexists for this route pattern ([id]). These throws are unreachable and could be replaced with type assertions or removed entirely.If you want to validate the ID format, consider returning a proper HTTP response instead of throwing:
- if (!params.id) throw Error('Error when getting split'); + // params.id is guaranteed by SvelteKit route matchingAlso applies to: 40-40, 84-84, 104-104
111-111: Avoidas anytype casting.The
as anycast bypasses TypeScript's type safety. Sinceparticipantsis typed asParticipant[], consider using proper type handling:- .update({ participants: participants as any }) + .update({ participants: participants as unknown as Json })Or import/define the proper JSON type from your Supabase types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.env.exampleis excluded by!.env.*
📒 Files selected for processing (5)
src/lib/server/supabase.ts(1 hunks)src/lib/storage.ts(7 hunks)src/lib/supabase.ts(1 hunks)src/routes/api/splits/+server.ts(1 hunks)src/routes/api/splits/[id]/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/server/supabase.ts (1)
src/lib/supabase-types.ts (1)
Database(9-67)
src/routes/api/splits/[id]/+server.ts (3)
src/routes/api/splits/+server.ts (1)
GET(6-34)src/lib/server/supabase.ts (1)
supabase(5-8)src/lib/types.ts (2)
Split(40-50)Participant(34-38)
🪛 GitHub Actions: Netlify Deploy
src/lib/server/supabase.ts
[error] 2-2: Build failed during vite build. SUPABASE_URL is not exported by the environment module '$env/static/private', imported by 'src/lib/server/supabase.ts'.
🔇 Additional comments (2)
src/lib/supabase.ts (1)
12-22: LGTM - Migration to fetch-based API.The replacement of direct Supabase queries with fetch calls to the backend API aligns with the PR objective to improve security. Error handling is consistent with the rest of the codebase.
src/lib/storage.ts (1)
6-6: LGTM - Centralized API base path.Good practice to centralize the API endpoint path for maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/routes/api/splits/+server.ts (1)
7-35: GET /api/splits looks good; consider narrowing selected columns.The query, mapping to
Split, and error handling are sound. As a small optimization and to reduce accidental schema drift, you could replaceselect('*')with an explicit column list for just the fields you map intoSplit.src/routes/api/splits/[id]/+server.ts (2)
7-38: Differentiate DB errors from 404 in GET /api/splits/[id].
if (error || !data) { return json({ error: 'Split not found' }, { status: 404 }); }treats all Supabase errors as “not found,” which can hide real server/DB issues. Consider returning 500 whenerroris truthy and 404 only when there’s no error but!data.
99-117: DELETE returns 200 even when no split exists.
delete().eq('id', params.id)will not treat “0 rows affected” as an error, so this handler always returns 200 on success, even for non-existent ids. If you want stricter REST semantics, consider requestingcountor chaining aselect()so you can return 404 when nothing was deleted.src/lib/validation.ts (1)
1-32: Zod schemas look solid; consider slightly stricter string/date validation.The schemas align well with the
Splitdomain and how the API routes use them. As optional hardening you could:
- Add
.trim()to user-entered strings (e.g. addresses, description, payerAddress) to avoid whitespace-only input.- Use a stricter date schema (e.g. an ISO date/datetime helper) and, if relevant, regexes for EVM-style addresses / tx hashes.
These are nice-to-have refinements; the current setup is already a good baseline.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)src/lib/validation.ts(1 hunks)src/routes/api/splits/+server.ts(1 hunks)src/routes/api/splits/[id]/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/api/splits/+server.ts (4)
src/routes/api/splits/[id]/+server.ts (1)
GET(7-38)src/lib/server/supabase.ts (1)
supabase(5-8)src/lib/types.ts (1)
Split(40-50)src/lib/validation.ts (1)
SplitCreateSchema(14-22)
src/routes/api/splits/[id]/+server.ts (4)
src/routes/api/splits/+server.ts (1)
GET(7-35)src/lib/server/supabase.ts (1)
supabase(5-8)src/lib/types.ts (1)
Split(40-50)src/lib/validation.ts (2)
SplitUpdateSchema(24-24)ParticipantsUpdateSchema(26-28)
🔇 Additional comments (2)
package.json (1)
58-59: Add zod dependency — looks consistent with usage.The new
"zod": "^4.1.13"dependency aligns with the validation layer insrc/lib/validation.tsand the API routes; no config issues from this change.src/routes/api/splits/[id]/+server.ts (1)
40-97: Unable to verify review comment—file not found in repository.The file
src/routes/api/splits/[id]/+server.tsand referenced schemas (SplitUpdateSchema,SplitCreateSchema) could not be located in the current repository. Verification cannot proceed without access to the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
svelte.config.js (1)
20-22: Remove the redundantprivatePrefixconfiguration.The
env.privatePrefix: ''setting explicitly defines the default SvelteKit behavior. By default, SvelteKit usespublicPrefix: "PUBLIC_"andprivatePrefix: "", which means any environment variable not starting withPUBLIC_is treated as private. Explicitly settingprivatePrefix: ''adds no functional value and can be removed for a cleaner configuration.
|
🚀 Deployed on https://gnosis-split.netlify.app |
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/routes/api/splits/[id]/+server.ts (1)
50-50: Fix: Callflatten()as a method on Zod error objects.Lines 50 and 142 reference
validationResult.error.flattenwithout calling it as a method. This will include the function reference itself in the response rather than the flattened error structure.Apply this diff to fix both occurrences:
if (!validationResult.success) { return json( - { error: 'Validation failed', details: validationResult.error.flatten }, + { error: 'Validation failed', details: validationResult.error.flatten() }, { status: 400 } ); }Note: This issue was flagged in previous reviews but still exists in the current code at lines 50 and 142.
Also applies to: 142-142
🧹 Nitpick comments (2)
src/routes/api/splits/[id]/+server.ts (2)
101-120: Consider returning 404 when deleting non-existent split.The DELETE handler returns success even when the split doesn't exist, as PostgreSQL DELETE succeeds regardless of whether rows match. For REST API clarity, consider checking existence first.
Apply this diff to verify the split exists before deleting:
export const DELETE: RequestHandler = async ({ params }) => { if (!params.id) throw Error('Error when deleting split'); try { const supabase = getSupabase(); + + const { data: existingSplit } = await supabase + .from('splits') + .select('id') + .eq('id', params.id) + .single(); + + if (!existingSplit) { + return json({ error: 'Split not found' }, { status: 404 }); + } + const { error } = await supabase .from('splits') .delete() .eq('id', params.id);Note: This adds an extra database round-trip. If idempotent DELETE behavior (always returning 200 for non-existent resources) is preferred, the current implementation is acceptable.
22-32: Extract duplicate DB-to-Split mapping into a helper function.The transformation from database row to
Splitobject is duplicated in the GET (lines 22-32) and PUT (lines 82-92) handlers. Extract this into a reusable helper function to improve maintainability.Add a helper function at the top of the file:
function mapDbRowToSplit(data: any): Split { return { id: data.id, description: data.description, totalAmount: data.total_amount, date: data.date, payerAddress: data.payer_address, participants: data.participants as unknown as Split['participants'], payments: data.payments as unknown as Split['payments'], createdAt: data.created_at as unknown as Split['createdAt'], sourceTxId: data.source_tx_id || undefined }; }Then use it in both handlers:
- const split: Split = { - id: data.id, - description: data.description, - totalAmount: data.total_amount, - date: data.date, - payerAddress: data.payer_address, - participants: data.participants as unknown as Split['participants'], - payments: data.payments as unknown as Split['payments'], - createdAt: data.created_at as unknown as Split['createdAt'], - sourceTxId: data.source_tx_id || undefined - }; + const split = mapDbRowToSplit(data);Also applies to: 82-92
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/server/supabase.ts(1 hunks)src/routes/api/splits/+server.ts(1 hunks)src/routes/api/splits/[id]/+server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/api/splits/+server.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/server/supabase.ts (1)
src/lib/supabase-types.ts (1)
Database(9-67)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netlify.toml(1 hunks)src/lib/server/supabase.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/server/supabase.ts
🔇 Additional comments (1)
netlify.toml (1)
5-7: Environment variable setup is correctly implemented for security.Server-side Supabase credentials (
SUPABASE_URL,SUPABASE_SECRET_API_KEY) are properly isolated:
- Imported via
$env/dynamic/privateinsrc/lib/server/supabase.ts, ensuring they remain server-side only- No
VITE_prefix exposes them to the client bundle- API routes securely access credentials through the
getSupabase()functionnetlify.tomlcontains only non-sensitive flags (VITE_USE_SUPABASE,VITE_MOCK_GNOSIS_PAY)No changes needed.
ical10
force-pushed
the
fix/improve-api-security
branch
from
bc24c3a to
d09d8f6
Compare
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/routes/api/splits/+server.ts (2)
1-37: Protect Supabase service-role access with proper authentication/authorization.
getSupabase()is initialized withSUPABASE_SECRET_API_KEY(service-role key), which bypasses RLS. Both the GET and POST handlers expose full read/write access on/api/splitswith no authentication or authorization, so any caller can list and create splits with elevated privileges.You should either (a) enforce authentication/authorization before any Supabase calls (e.g., derive the user from SvelteKit
locals, validate permissions, and scope queries) or (b) switch this route to an anon key with RLS-enforced policies appropriate for untrusted clients.Also applies to: 39-89
44-48: CallvalidationResult.error.flatten()to return structured errors.Here you return the
flattenfunction reference instead of its result:return json( { error: 'Validation failed', details: validationResult.error.flatten }, { status: 400 } );This will serialize a function, not the error tree. Invoke it:
- if (!validationResult.success) { - return json( - { error: 'Validation failed', details: validationResult.error.flatten }, - { status: 400 } - ); - } + if (!validationResult.success) { + return json( + { error: 'Validation failed', details: validationResult.error.flatten() }, + { status: 400 } + ); + }
🧹 Nitpick comments (2)
src/routes/api/splits/+server.ts (1)
39-42: Consider treating invalid JSON bodies as client errors (400), not 500.If
request.json()throws (malformed JSON), the catch block returns a 500, even though it’s a bad request. You could special-case JSON parse failures and return a 400 with a clearer error message, then reserve 500 for genuine server-side issues.Also applies to: 85-88
src/lib/storage.ts (1)
20-31: Consider injectingfetchfor better SSR and future auth integration.These helpers call the global
fetchdirectly. If you later protect/api/splitswith cookies/session-based auth, server-side calls from this module may not automatically forward the right credentials. Long term, it may be cleaner to accept afetchargument (from SvelteKit load functions/endpoints) or to keep API calls inside route/load files so you always useevent.fetch.Also applies to: 51-59, 67-73, 98-105, 121-126, 148-157
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/server/supabase.ts(1 hunks)src/lib/storage.ts(7 hunks)src/routes/api/splits/+server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/server/supabase.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/api/splits/+server.ts (4)
src/routes/api/splits/[id]/+server.ts (1)
GET(7-39)src/lib/server/supabase.ts (1)
getSupabase(7-20)src/lib/types.ts (1)
Split(40-50)src/lib/validation.ts (1)
SplitCreateSchema(14-22)
🔇 Additional comments (1)
src/lib/storage.ts (1)
6-31: API-based storage refactor looks consistent and preserves existing semantics.The new
API_BASEplus fetch-based implementations for list/read/create/update/delete/participants keep the localStorage fallback intact, handle HTTP errors in a consistent way, and align with the new/api/splitsroutes. Return types and control flow (throw vs. fallback) are coherent with the previous behavior.Also applies to: 51-59, 67-73, 98-105, 121-126, 148-157
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
vite.config.ts (1)
9-35: Confirm intentional PWA shutdown and consider an env-driven toggle instead of commenting out the pluginRight now PWA is effectively fully disabled (no service worker/manifest/runtime caching) while the
VitePWAimport and config stay in comments. If this is just a temporary workaround for Netlify adapter issues, you might prefer an env/adapter-based toggle so you can keep PWA in local/prod builds but switch it off only where it breaks, e.g.:const disablePWA = process.env.NETLIFY === 'true'; export default defineConfig({ plugins: [ tailwindcss(), sveltekit(), !disablePWA && VitePWA({ /* existing config */ }) ].filter(Boolean) });If the plan is to drop PWA longer term, also consider removing the unused plugin import/config to avoid confusion for future readers.
netlify.toml (1)
4-7: Ensure sensitive credentials are not hardcoded in netlify.toml.While the build environment configuration looks reasonable, ensure that sensitive values like
SUPABASE_SECRET_API_KEYandSUPABASE_URLare stored securely (e.g., Netlify Site settings or GitHub Actions secrets) rather than hardcoded in version-controlled files.src/lib/server/supabase.ts (1)
5-24: MakegetSupabase’s return type non-nullable to match runtime behaviorAt runtime this function either throws (when env vars are missing) or returns an initialized client, so callers never actually see
null. However, because_supabaseis typed asReturnType<typeof createClient<Database>> | nulland returned directly, TypeScript will infergetSupabase()as returning a nullable type, forcing unnecessary null checks or!at call sites.You can tighten the typing so the public API is always a concrete client while still using a lazy singleton internally:
-import { createClient } from '@supabase/supabase-js'; +import { createClient, type SupabaseClient } from '@supabase/supabase-js'; @@ -let _supabase: ReturnType<typeof createClient<Database>> | null = null; +let _supabase: SupabaseClient<Database> | undefined; @@ -export function getSupabase() { +export function getSupabase(): SupabaseClient<Database> { if (!_supabase) { @@ - _supabase = createClient<Database>(url, key); + _supabase = createClient<Database>(url, key); } - return _supabase; + return _supabase; }This keeps the runtime semantics identical but gives downstream code a cleaner, non-nullable type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netlify.toml(1 hunks)src/lib/server/supabase.ts(1 hunks)vite.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/server/supabase.ts (1)
src/lib/supabase-types.ts (1)
Database(9-67)
🔇 Additional comments (2)
netlify.toml (2)
6-6: Verify VITE_USE_SUPABASE flag semantics aligns with the security objective.Setting
VITE_USE_SUPABASE = "true"in the build environment appears inconsistent with the PR objective to move API calls from the client to the backend. For a security-focused migration, the client should not directly access Supabase. Verify:
- What does this flag control in the codebase?
- Should it be
falseto disable client-side Supabase access?- Or is it intended to enable optional client Supabase fallback logic?
7-7: Verify VITE_MOCK_GNOSIS_PAY flag is appropriate for production builds.Setting
VITE_MOCK_GNOSIS_PAY = "true"in the Netlify build environment will enable mocking in production. Verify if this is intentional or if it should be conditionally set based on deployment environment (preview vs. production).
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/server/supabase.ts (1)
1-3: Fix build failure from$env/static/privateimport (PRIVATE_SUPABASE_URL not exported).The Netlify build is failing because
$env/static/privateonly exports variables that exist at build time, andPRIVATE_SUPABASE_URL/PRIVATE_SUPABASE_SERVICE_ROLE_KEYare not defined in that environment. That makes this import a hard compile‑time error (see the pipeline failure).You have two main options:
Keep
$env/static/private(stricter, preferred if you control all envs)
- Ensure
PRIVATE_SUPABASE_URLandPRIVATE_SUPABASE_SERVICE_ROLE_KEYare set:
- In local
.envfiles.- In GitHub Actions / Netlify environment variables (names must match exactly).
- This keeps the current code and your runtime check as‑is.
Switch to
$env/dynamic/privateto avoid build‑time coupling
- This removes the compile‑time export requirement while still letting your runtime check guard against missing values:
- import { PRIVATE_SUPABASE_URL, PRIVATE_SUPABASE_SERVICE_ROLE_KEY } from '$env/static/private';
- import { env } from '$env/dynamic/private';
import type { Database } from '$lib/supabase-types';
@@
- const supabaseUrl = PRIVATE_SUPABASE_URL;
- const supabaseKey = PRIVATE_SUPABASE_SERVICE_ROLE_KEY;
- const supabaseUrl = env.PRIVATE_SUPABASE_URL;
- const supabaseKey = env.PRIVATE_SUPABASE_SERVICE_ROLE_KEY;
Your `if (!supabaseUrl || !supabaseKey)` guard then handles misconfiguration at runtime with a clear error. Given the current hard build failure, I’d strongly recommend either ensuring these env vars are defined in Netlify before merging or adopting the dynamic env import. Also applies to: 9-16 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>netlify.toml (1)</summary><blockquote> `1-8`: **Verify Netlify publish directory against SvelteKit/Netlify defaults.** This config is internally consistent (Bun version + VITE flags look fine), but SvelteKit’s Netlify adapter and Netlify docs both assume `publish = "build"` by default. Unless you’ve explicitly configured the adapter to output to `.svelte-kit/build`, consider aligning with the default to avoid surprising deploy behavior: ```diff [build] - command = "bun run build" - publish = ".svelte-kit/build" + command = "bun run build" + publish = "build"If you keep
.svelte-kit/build, double‑check that your Netlify UI “Publish directory” matches this value.src/lib/server/supabase.ts (1)
5-24: Tighten Supabase client typing andgetSupabasereturn type.Right now
_supabaseis typed asReturnType<typeof createClient<Database>> | null, so TypeScript will infergetSupabase()can returnnulleven though the function guarantees initialization before returning. This makes call sites noisier than necessary and theReturnTypeusage is more complex than needed.You can simplify and get a precise non‑null return type by using the
SupabaseClienttype and annotating the function explicitly:-import { createClient } from '@supabase/supabase-js'; +import { createClient, type SupabaseClient } from '@supabase/supabase-js'; @@ -let _supabase: ReturnType<typeof createClient<Database>> | null = null; +let _supabase: SupabaseClient<Database> | null = null; @@ -export function getSupabase() { +export function getSupabase(): SupabaseClient<Database> { @@ - return _supabase; + return _supabase!; }This keeps the lazy singleton behavior while giving callers a clean, non‑nullable
SupabaseClient<Database>type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.env.exampleis excluded by!.env.*
📒 Files selected for processing (5)
netlify.toml(1 hunks)package.json(2 hunks)src/lib/server/supabase.ts(1 hunks)src/routes/api/splits/+server.ts(1 hunks)svelte.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- svelte.config.js
- package.json
- src/routes/api/splits/+server.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/server/supabase.ts (1)
src/lib/supabase-types.ts (1)
Database(9-67)
🪛 GitHub Actions: Netlify Deploy
src/lib/server/supabase.ts
[error] 2-2: Build failed: Import from '$env/static/private' references PRIVATE_SUPABASE_URL which is not exported by the virtual env. Ensure PRIVATE_SUPABASE_URL is correctly exposed (or adjust env config). Command: bun run build (vite build).
|
🚀 Deployed on https://gnosis-split.netlify.app |
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.env.exampleis excluded by!.env.*
📒 Files selected for processing (2)
.github/workflows/netlify-deploy.yml(1 hunks)src/lib/server/supabase.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/netlify-deploy.yml
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/server/supabase.ts (1)
src/lib/supabase-types.ts (1)
Database(9-67)
🔇 Additional comments (1)
src/lib/server/supabase.ts (1)
7-24: Verify thatPUBLIC_SUPABASE_PUBLISHABLE_KEYis an anon key and document thatservice_rolekeys must never use thePUBLIC_prefixThe lazy initialization, explicit env checks, and auth configuration are solid. However, the security concern about environment variable exposure is valid and important.
In SvelteKit, variables exported from
$env/static/publicwith thePUBLIC_prefix are statically injected into client-side code at build time—they become part of the browser bundle and are fully visible. This is safe only for non-sensitive data.For Supabase:
- Anon/publishable keys (low privilege, read-only with Row Level Security) are safe to expose publicly
- Service_role/secret keys (bypass RLS, powerful) must never be exposed and should only exist in private env vars (
$env/static/privateor secrets manager)Confirm that:
PUBLIC_SUPABASE_PUBLISHABLE_KEYis indeed an anon/publishable key with minimal, user-safe privileges- Any service_role or secret keys are stored in non-
PUBLIC_env vars and used only server-side (if at all)- Update
.env.exampleand documentation to explicitly state thatPUBLIC_SUPABASE_PUBLISHABLE_KEYmust be an anon/publishable key and never a service_role key
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/routes/api/splits/[id]/+server.ts (1)
61-65: CallvalidationResult.error.flatten()so validation details are returned correctlyIn both PUT and PATCH handlers you return
validationResult.error.flatteninstead of invoking it:{ error: 'Validation failed', details: validationResult.error.flatten }
flattenis a method in Zod 4; without()the response will serialize without the structured error details, making debugging harder.Update both cases to:
{ error: 'Validation failed', details: validationResult.error.flatten() }This matches the Zod 4 API.
Also applies to: 153-157
🧹 Nitpick comments (3)
src/routes/split/payment/+page.svelte (1)
34-38: Use the same user address when reloading the split after paymentYou correctly pass
$walletAddress || participantAddrtogetSpliton initial load, but the post‑payment refresh usesgetSplit(split.id)without an address. For the API-backed path this issues/api/splits/{id}without theaddressquery param, bypassing the address-based checks in the GET handler and making future tightening of that check harder.Consider reusing the same address you used at load time, e.g. store it in a local
const viewerAddress = $walletAddress || participantAddr;and pass it to bothgetSplitcalls.Also applies to: 87-96
src/routes/splits/+page.svelte (1)
6-7: Consider reacting to wallet address changes, not just on mount
loadSplits()now scopes to$address, but it’s only invoked inonMount. If the user connects or switches a wallet after the page has mounted, the list won’t refresh.Consider adding a reactive effect (e.g.
$effector derived store subscription) that callsloadSplits()when$addresschanges to a truthy value.Also applies to: 21-31
src/routes/api/splits/[id]/+server.ts (1)
19-21: Differentiate database errors from “not found” in responsesIn several places:
- GET:
if (error || !data) { return json({ error: 'Split not found' }, { status: 404 }); }- PUT:
if (!data)⇒ 404 (good), but DB errors handled separately.- PATCH:
if (fetchError || !existingSplit)⇒ 404.Treating DB errors as 404 “not found” hides real server problems and makes observability/debugging harder. Consider:
- For each handler, first check
if (error || fetchError)⇒ log and return 500.- Then check
if (!data || !existingSplit)⇒ 404.This keeps API semantics accurate while still returning 404 for genuinely missing splits.
Also applies to: 86-93, 146-148
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/lib/server/supabase.ts(1 hunks)src/lib/storage.ts(5 hunks)src/lib/supabase.ts(1 hunks)src/routes/api/splits/+server.ts(1 hunks)src/routes/api/splits/[id]/+server.ts(1 hunks)src/routes/split/[id]/+page.svelte(1 hunks)src/routes/split/payment/+page.svelte(1 hunks)src/routes/splits/+page.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/api/splits/+server.ts
- src/lib/supabase.ts
- src/lib/server/supabase.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/api/splits/[id]/+server.ts (4)
src/routes/api/splits/+server.ts (1)
GET(7-57)src/lib/server/supabase.ts (1)
getSupabase(7-25)src/lib/types.ts (2)
Participant(34-38)Split(40-50)src/lib/validation.ts (2)
SplitUpdateSchema(24-24)ParticipantsUpdateSchema(26-28)
src/lib/storage.ts (1)
src/lib/types.ts (1)
Split(40-50)
🔇 Additional comments (1)
src/lib/storage.ts (1)
8-37: Address‑scopedgetSplitslooks goodThe new
getSplits(userAddress?)behavior—filtering bypayerAddressin localStorage and using/api/splits?address=...when on the API path—matches the address‑scoped backend and keeps a safe fallback on error ([]). No issues from my side here.
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/stores/wallet.ts (1)
13-21: Initial seeding fromappKitlooks good; confirm initialization timing and consider minor DRYing
- Using
getAccount?.()/getState?.()once to seedappKitStorebefore subscriptions is a good improvement; it avoids a “blank” store on first render and keeps SSR-safe access behind thebrowser && appKitguard.- One thing to double‑check: this
if (browser && appKit)runs once at module import time. If$lib/appkitinitializesappKitlazily (e.g., via aninitAppKit()call after this store is imported), this block (includingsubscribeState/subscribeAccountbelow) would never run. IfappKitis guaranteed to be constructed eagerly in its module, you’re fine; otherwise you may want to move this wiring into an explicitinitWalletStore(appKit)or similar.Optional polish:
- To avoid the initial state drifting over time, you could extract a shared
initialStateand reuse it both inwritable(initialState)and here when callingset, but that’s purely a convenience refactor.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/stores/wallet.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/stores/wallet.ts (1)
src/lib/appkit.ts (1)
appKit(37-37)
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/split/[id]/+page.svelte (1)
36-36: Address-scoped API alignment is now complete.The past review comment flagged inconsistent address-scoping, but this version now correctly passes
$walletAddresstocreateSplitStore, and all subsequentgetSplitandupdateSplitcalls (lines 38, 76, 169, 172) include the address parameter. This ensures consistent authorization checks across all data operations.src/lib/storage.ts (1)
83-111: Address-scoping throughupdateSplitis now implemented.The past review comment requested threading
userAddressthroughgetSplitandupdateSplit. The current implementation correctly acceptsuserAddress?: string(line 83), passes it togetSplit(id, userAddress)(line 100), and includes it in the API URL query parameter (line 104). This establishes the consistent authorization pattern requested.
🧹 Nitpick comments (2)
src/routes/split/[id]/+page.svelte (1)
76-76: Simplify the$walletAddress || undefinedpattern.The
|| undefinedis redundant since$walletAddresswhen null or undefined is already falsy and functions acceptstring | undefined. Bothnullandundefinedwork as "no address provided."Simplify to:
- const loaded = await getSplit(id, $walletAddress || undefined); + const loaded = await getSplit(id, $walletAddress);Apply the same simplification at lines 169 and 172.
Also applies to: 169-169, 172-172
src/lib/storage.ts (1)
24-36: Standardize error handling across API operations.Error handling is inconsistent across Supabase-mode functions:
getSplitsandgetSplituse try-catch and return safe defaults (empty array, undefined)- Write operations (
saveSplit,updateSplit,deleteSplit,updateSplitParticipants) throw errors directlyWhile this may be intentional (reads fail gracefully, writes fail loudly), it creates unpredictable error behavior for calling code.
Consider either:
- Wrap all write operations in try-catch and log errors before throwing (consistent with defensive patterns), or
- Document the error contract clearly: reads return safe defaults, writes throw and must be caught by callers.
If choosing option 1, apply this pattern to write operations:
export async function saveSplit(split: Omit<Split, 'id' | 'createdAt'>): Promise<Split | undefined> { // ... non-Supabase path ... try { const response = await fetch(API_BASE, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(split) }); if (!response.ok) throw new Error('Failed to create split'); return await response.json(); } catch (error) { console.error('Failed to save split:', error); throw error; // or return undefined for safe default } }Also applies to: 56-63, 72-80, 100-111, 128-132, 155-164
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/blockscoutVerifier.ts(1 hunks)src/lib/storage.ts(5 hunks)src/routes/split/[id]/+page.svelte(3 hunks)src/routes/split/payment/+page.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/split/payment/+page.svelte
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/storage.ts (1)
src/lib/types.ts (1)
Split(40-50)
1 participant
This PR includes:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.