Pertempto commented

(Migrated from github.com)

No description provided.

github-actions[bot] commented

2025-12-11 22:29:12 -05:00

(Migrated from github.com)

File: supabase/migrations/20251121133948_replace_auth_trigger_with_signup_rpc.sql

Actionable change: Do NOT add auth_user_id uuid not null in one step on a populated table. Change migration to: (1) add auth_user_id uuid NULL, (2) backfill values for existing rows (or create matching auth.users test rows in seed), (3) add FK and NOT NULL constraint in a subsequent migration. Current ALTER TABLE ... ADD COLUMN ... NOT NULL will fail on non-empty DBs.
Also: the migration adds a FK/unique and then validates constraint — ensure ordering after backfill so validate constraint won't fail.

Please split this migration and add a short comment in the file describing the multi-step plan so reviewers/CI understand the intended rollout.

File: `supabase/migrations/20251121133948_replace_auth_trigger_with_signup_rpc.sql` - Actionable change: Do NOT add `auth_user_id uuid not null` in one step on a populated table. Change migration to: (1) add `auth_user_id uuid NULL`, (2) backfill values for existing rows (or create matching `auth.users` test rows in seed), (3) add FK and `NOT NULL` constraint in a subsequent migration. Current `ALTER TABLE ... ADD COLUMN ... NOT NULL` will fail on non-empty DBs. - Also: the migration adds a FK/unique and then validates constraint — ensure ordering after backfill so `validate constraint` won't fail. Please split this migration and add a short comment in the file describing the multi-step plan so reviewers/CI understand the intended rollout.

github-actions[bot] commented

2025-12-11 22:30:09 -05:00

(Migrated from github.com)

File: supabase/migrations/20251121145635_add_validate_invite_function.sql

Actionable change: Remove pg_sleep(5) from validate_invite_code. Sleeping inside DB functions ties up Postgres worker processes and risks resource exhaustion under load. Alternatives:
- Enforce rate-limits at the edge/app layer (Cloudflare / API gateway) and remove sleep here, or
- Implement an in-DB rate limiter (token-bucket / attempts table) that atomically increments/checks attempts per IP or per auth id.
Actionable change: Avoid returning the invitee name from this RPC (it leaks information about existing members). Return only a minimal response (e.g. { valid: true }) or a masked value if necessary.

File: `supabase/migrations/20251121145635_add_validate_invite_function.sql` - Actionable change: Remove `pg_sleep(5)` from `validate_invite_code`. Sleeping inside DB functions ties up Postgres worker processes and risks resource exhaustion under load. Alternatives: - Enforce rate-limits at the edge/app layer (Cloudflare / API gateway) and remove sleep here, or - Implement an in-DB rate limiter (token-bucket / attempts table) that atomically increments/checks attempts per IP or per auth id. - Actionable change: Avoid returning the invitee `name` from this RPC (it leaks information about existing members). Return only a minimal response (e.g. `{ valid: true }`) or a masked value if necessary.

github-actions[bot] commented

2025-12-11 22:30:25 -05:00

(Migrated from github.com)

Changes Requested

Please address the following high-impact items before merging:

Split the auth_user_id rollout: add nullable column, backfill existing rows (or seed matching auth.users), then add FK/unique + set NOT NULL in a follow-up migration. Include comments describing the multi-step rollout.
Remove pg_sleep(5) from validate_invite_code and complete_signup. Implement rate-limiting at the edge/app layer or use a non-blocking DB pattern (attempts table / atomic counter) instead.
Stop returning invitee name (or any identifying data) from validate_invite_code; return a minimal validation response only.
Fix src/middleware.ts: define cookieHeader from request headers before creating the server Supabase client, only call RPCs when user exists, and wrap RPC calls in try/catch with graceful handling (redirect to /auth/welcome or allow limited access).
Restore essential npm scripts in package.json (or add npm proxies to just) so contributors and tools that expect npm run continue to work.

Summary of Changes

Adds onboarding UI and new React components (OnboardingWizard, ErrorAlert, refactors Button with forwardRef).
Replaces automatic auth trigger with explicit RPCs and migrations: validate_invite_code, complete_signup, get_user_by_auth_id.
Adds auth_user_id to user table and updates seeds to create user/contact rows manually (migration currently makes column NOT NULL + FK in one step).
Moves developer tooling to just (adds justfile), updates CI to call just, and removes npm scripts from package.json.
Adds Cloudflare Workers deploy workflow and tweaks Supabase config (statement_timeout, enable_signup).

Overall Feedback

Negative: There are critical operational/security risks in the DB changes and RPCs that must be fixed before merging. Adding a NOT NULL FK column on a populated table and using pg_sleep() inside Postgres functions will break production rollouts and can lead to resource exhaustion. Returning invitee names from an early validation RPC leaks membership info. The middleware currently relies on an undefined cookieHeader and calls RPCs unconditionally which can throw in middleware.
Positive: The frontend onboarding UX and component refactors are well-organized and improve developer ergonomics. Moving to explicit RPCs and deterministic seeds is the right direction when migrations and rate-limiting are implemented safely. Documentation and CI improvements show good attention to DX. Nice work on the UI polish and separation of concerns!

Please @Pertempto when you push the updates and request another review. Thank you! :)

# Changes Requested Please address the following high-impact items before merging: - Split the `auth_user_id` rollout: add nullable column, backfill existing rows (or seed matching `auth.users`), then add FK/unique + set NOT NULL in a follow-up migration. Include comments describing the multi-step rollout. - Remove `pg_sleep(5)` from `validate_invite_code` and `complete_signup`. Implement rate-limiting at the edge/app layer or use a non-blocking DB pattern (attempts table / atomic counter) instead. - Stop returning invitee `name` (or any identifying data) from `validate_invite_code`; return a minimal validation response only. - Fix `src/middleware.ts`: define `cookieHeader` from request headers before creating the server Supabase client, only call RPCs when `user` exists, and wrap RPC calls in try/catch with graceful handling (redirect to `/auth/welcome` or allow limited access). - Restore essential `npm` scripts in `package.json` (or add `npm` proxies to `just`) so contributors and tools that expect `npm run` continue to work. ## Summary of Changes - Adds onboarding UI and new React components (`OnboardingWizard`, `ErrorAlert`, refactors `Button` with forwardRef). - Replaces automatic auth trigger with explicit RPCs and migrations: `validate_invite_code`, `complete_signup`, `get_user_by_auth_id`. - Adds `auth_user_id` to `user` table and updates seeds to create user/contact rows manually (migration currently makes column NOT NULL + FK in one step). - Moves developer tooling to `just` (adds `justfile`), updates CI to call `just`, and removes `npm` scripts from `package.json`. - Adds Cloudflare Workers deploy workflow and tweaks Supabase config (statement_timeout, enable_signup). ## Overall Feedback - Negative: There are critical operational/security risks in the DB changes and RPCs that must be fixed before merging. Adding a NOT NULL FK column on a populated table and using `pg_sleep()` inside Postgres functions will break production rollouts and can lead to resource exhaustion. Returning invitee names from an early validation RPC leaks membership info. The middleware currently relies on an undefined `cookieHeader` and calls RPCs unconditionally which can throw in middleware. - Positive: The frontend onboarding UX and component refactors are well-organized and improve developer ergonomics. Moving to explicit RPCs and deterministic seeds is the right direction when migrations and rate-limiting are implemented safely. Documentation and CI improvements show good attention to DX. Nice work on the UI polish and separation of concerns! Please @Pertempto when you push the updates and request another review. Thank you! :)

github-actions[bot] commented

2025-12-12 06:32:26 -05:00

(Migrated from github.com)

File: supabase/schemas/02_users.sql

Actionable change (migration safety): The schema now declares auth_user_id uuid unique not null references auth.users(id) directly. For production rollout, do NOT add a NOT NULL FK column in one step on populated tables. Instead:
- Add auth_user_id uuid nullable first,
- Backfill values for existing rows (or seed matching auth.users entries),
- Add the FK + unique index and then NOT NULL constraint in a subsequent migration.
Actionable change (consistency): Remove duplicate index creation — you create idx_user_auth_user_id here and also in the migration file. Keep index creation in migrations only to avoid schema drift.

Please update the schema/migrations to a safe, multi-step rollout.

File: `supabase/schemas/02_users.sql` - Actionable change (migration safety): The schema now declares `auth_user_id uuid unique not null references auth.users(id)` directly. For production rollout, do NOT add a NOT NULL FK column in one step on populated tables. Instead: - Add `auth_user_id uuid` nullable first, - Backfill values for existing rows (or seed matching `auth.users` entries), - Add the FK + unique index and then `NOT NULL` constraint in a subsequent migration. - Actionable change (consistency): Remove duplicate index creation — you create `idx_user_auth_user_id` here and also in the migration file. Keep index creation in migrations only to avoid schema drift. Please update the schema/migrations to a safe, multi-step rollout.

github-actions[bot] commented

2025-12-12 06:43:27 -05:00

(Migrated from github.com)

File: src/middleware.ts

Actionable change: Middleware calls get_user_by_auth_id RPC before ensuring the request is authenticated and before consistently passing request cookies to createSupabaseServerClient.
- Use the same cookie/header values when creating the server supabase client: createSupabaseServerClient(context.cookies, cookieHeader) (you already added that param in import but must ensure it's used everywhere consistently).
- Only call the get_user_by_auth_id RPC when user is present; check for user first. If the RPC fails, handle the error gracefully and avoid leaking internal errors — return a server-side redirect to /auth/welcome or let the request proceed with limited access.

Please reorder logic so authentication is checked first, then create the server client with request cookies, then call the RPC. Add error handling around the RPC (try/catch) and log fewer internal details in production.

File: `src/middleware.ts` - Actionable change: Middleware calls `get_user_by_auth_id` RPC before ensuring the request is authenticated and before consistently passing request cookies to `createSupabaseServerClient`. - Use the same cookie/header values when creating the server supabase client: `createSupabaseServerClient(context.cookies, cookieHeader)` (you already added that param in import but must ensure it's used everywhere consistently). - Only call the `get_user_by_auth_id` RPC when `user` is present; check for `user` first. If the RPC fails, handle the error gracefully and avoid leaking internal errors — return a server-side redirect to `/auth/welcome` or let the request proceed with limited access. Please reorder logic so authentication is checked first, then create the server client with request cookies, then call the RPC. Add error handling around the RPC (try/catch) and log fewer internal details in production.

Rows
Columns

feat: add basic sign up flow #36

Changes Requested

Summary of Changes

Overall Feedback