Conversation
ignaciodob
added
the
do-not-merge
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughAdds entitlement gating for enhanced MFA security in the auth form with conditional UI updates, extends API type definitions to include validation errors and deprecate legacy fields, and introduces cloud marketplace contract linking eligibility endpoint alongside schema refinements for disk throughput and feature keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)apps/studio/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
Files:
apps/studio/**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-12-12T11:28:04.537ZApplied to files:
📚 Learning: 2025-12-11T17:04:40.037ZApplied to files:
📚 Learning: 2025-12-11T17:04:40.037ZApplied to files:
📚 Learning: 2025-12-12T05:20:17.409ZApplied to files:
📚 Learning: 2025-12-12T11:27:51.417ZApplied to files:
🧬 Code graph analysis (1)apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx (2)
⏰ 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). (1)
🔇 Additional comments (8)
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 |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx (1)
610-617: Bug: Wrong loading state variable used.The
loadingprop referencesisUpdatingPhoneFormbut should useisUpdatingSecurityForm. This will cause the button to show a loading spinner based on the phone form's submission state instead of the security form's state.🔎 Proposed fix
<Button type="primary" htmlType="submit" disabled={ !canUpdateConfig || isUpdatingSecurityForm || !securityForm.formState.isDirty } - loading={isUpdatingPhoneForm} + loading={isUpdatingSecurityForm} > Save changes </Button>Additionally, for consistency with the SMS MFA save button (line 529), consider adding
!hasAccessToEnhanceSecurityto the disabled condition as defense in depth:disabled={ - !canUpdateConfig || isUpdatingSecurityForm || !securityForm.formState.isDirty + !canUpdateConfig || isUpdatingSecurityForm || !securityForm.formState.isDirty || !hasAccessToEnhanceSecurity }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsxpackages/api-types/types/api.d.tspackages/api-types/types/platform.d.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/studio/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.{ts,tsx}: Assign complex conditions to descriptive variables instead of using multiple conditions in a single expression
Use consistent naming conventions for boolean variables:isprefix for state/identity,hasprefix for possession,canprefix for capability/permission,shouldprefix for conditional behavior
Derive boolean state from existing state instead of storing it separately
Use early returns for guard clauses instead of deeply nested conditionals
Extract complex logic into custom hooks when logic becomes reusable or complex
Return objects from custom hooks instead of arrays for better extensibility and clearer API
Use discriminated unions for complex state management instead of multiple independent state fields
Avoid type casting (e.g.,as any,as Type); instead validate values at runtime using zod schemas
Files:
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
apps/studio/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.tsx: Components should ideally be under 200-300 lines; break down large components with multiple distinct UI sections, complex conditional rendering, or multiple unrelated useState hooks
Extract repeated JSX patterns into reusable components instead of copying similar JSX blocks
Use consistent loading/error/success pattern: handle loading state first with early returns, then error state, then empty state, then render success state
Keep state as local as possible and only lift up when needed
Group related state using objects or reducers instead of multiple useState calls, preferring react-hook-form for form state management
Name event handlers consistently: useonprefix for prop callbacks andhandleprefix for internal handlers
Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Use appropriate conditional rendering patterns: && for simple show/hide, ternary for binary choice, early returns or extracted components for multiple conditions
Avoid nested ternaries in JSX; use separate conditions or early returns instead
Use useMemo for expensive computations when the computation is genuinely expensive and the value is passed to memoized children
Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Files:
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: In supabase/supabase, use the existing 'do-not-merge' label to block merges for entitlement-gated UI changes until the backend entitlement is live and API types are regenerated.
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: For entitlement-gated changes (e.g., adding 'security.questionnaire'), apply a 'do-not-merge' label until backend entitlements are confirmed live, and avoid merging UI-only changes that rely on new entitlements.
📚 Learning: 2025-12-12T11:28:04.537Z
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: For entitlement-gated changes (e.g., adding 'security.questionnaire'), apply a 'do-not-merge' label until backend entitlements are confirmed live, and avoid merging UI-only changes that rely on new entitlements.
Applied to files:
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Build forms with react-hook-form and zod for validation
Applied to files:
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Use Switch component with checked and onCheckedChange props for toggle form fields
Applied to files:
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
📚 Learning: 2025-12-12T05:20:17.409Z
Learnt from: joshenlim
Repo: supabase/supabase PR: 41258
File: apps/studio/pages/project/[ref]/storage/vectors/buckets/[bucketId].tsx:9-9
Timestamp: 2025-12-12T05:20:17.409Z
Learning: In apps/studio/**/*.{ts,tsx}, use named imports for DefaultLayout: import { DefaultLayout } from 'components/layouts/DefaultLayout' instead of default import. This is the new practice being adopted across the studio app.
Applied to files:
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
📚 Learning: 2025-12-12T11:27:51.417Z
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:27:51.417Z
Learning: For changes tied to entitlements (e.g., adding a new entitlement like 'security.questionnaire'), do not merge until backend entitlements are confirmed live. Apply a do-not-merge label on such PRs and ensure UI changes that rely on new entitlements are not merged in isolation. In reviews, verify entitlement availability in backend before approving and, if needed, hold UI-only changes until entitlement is active.
Applied to files:
packages/api-types/types/api.d.tspackages/api-types/types/platform.d.ts
🧬 Code graph analysis (1)
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx (2)
apps/studio/hooks/misc/useCheckEntitlements.ts (1)
useCheckEntitlements(58-114)apps/studio/components/ui/UpgradeToPro.tsx (1)
UpgradeToPro(25-65)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (8)
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx (3)
123-124: LGTM!The entitlement check follows the same pattern as the existing
auth.mfa_phonecheck and correctly destructures the needed values with appropriate naming.
288-288: LGTM!Correctly extends the loading guard to include the new entitlement loading state, ensuring the form doesn't render until all entitlement data is resolved.
586-603: LGTM!The entitlement gating correctly disables the toggle and shows the upgrade prompt when the user lacks access. The implementation follows the same pattern as the existing SMS MFA upgrade prompt.
packages/api-types/types/api.d.ts (3)
286-292: Pagination docs for organization projects are clear and accurateThe added note about offset-based pagination (offset + limit semantics) is precise and matches typical API behavior, improving consumer understanding without changing surface.
309-313: Guidance to prefer organization-scoped/v1/organizations/{slug}/projectsis helpfulThe extra note on
/v1/projectsbeing less precise and suggesting the org-scoped endpoint clarifies intended usage and should reduce misuse of the legacy listing API.
3278-3366: Upgrade eligibility deprecations +validation_errorsunion are well structuredMarking
objects_to_be_dropped,unsupported_extensions, anduser_defined_objects_in_internal_schemasas deprecated while introducing the typedvalidation_errorsdiscriminated union (keyed bytype) keeps backward compatibility and gives clients a richer, strongly-typed error surface. The union members and discriminators look consistent and ergonomic for narrowing on the client side.packages/api-types/types/platform.d.ts (2)
230-249: Verify PR scope and auto-generated nature of this file.This PR adds a cloud marketplace contract linking eligibility endpoint, which appears unrelated to the MFA enhanced entitlement feature described in the PR title and objectives. Additionally, this
.d.tsfile appears to be auto-generated from an OpenAPI specification.Please confirm:
- Are these type definitions auto-generated? If so, this entire file should be regenerated from the platform API spec rather than manually edited.
- Should the unrelated cloud marketplace changes be included in this PR, or do they represent concurrent platform API changes that will be picked up when types are regenerated?
Based on learnings, this PR correctly uses the 'do-not-merge' label to block merging until the backend entitlement is live per the dependent platform PR #28639.
6962-6962: LGTM: Core entitlement key added correctly.The
auth.mfa_enhanced_securityfeature key is correctly added to the entitlement type union, which aligns with the PR objectives for gating MFA enhanced security features.Based on learnings, ensure this PR remains blocked with the 'do-not-merge' label until the dependent platform PR #28639 is merged and the backend entitlement is confirmed live.
🎭 Playwright Test Results
Details
Skipped testsFeatures › sql-editor.spec.ts › SQL Editor › snippet favourite works as expected |
apps/studio/components/interfaces/Auth/MfaAuthSettingsForm/MfaAuthSettingsForm.tsx
Outdated
Show resolved
Hide resolved
ignaciodob
force-pushed
the
chore/mfa-enhanced-entitlement
branch
from
ed6f82c to
5e5be0e
Compare
ignaciodob
removed
the
do-not-merge
Braintrust eval report
|
3 participants
Depends on https://github.com/supabase/platform/pull/28639
Changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.