fix(studio): batch of production Sentry crash fixes#47695
Conversation
…s), round 2 Same non-array/malformed-API-response bug class as #47460 (FE-3748), for issues that weren't caught in that pass. Fixes SUPABASE-APP-JTP Fixes SUPABASE-APP-B3M Fixes SUPABASE-APP-K2D Fixes SUPABASE-APP-K2H Fixes SUPABASE-APP-JCD Fixes SUPABASE-APP-JV9 Fixes SUPABASE-APP-H6Y Fixes SUPABASE-APP-K2F Fixes SUPABASE-APP-JVQ Resolves FE-3862
Fixes SUPABASE-APP-K2J Fixes SUPABASE-APP-K2E Fixes SUPABASE-APP-JE3 Fixes SUPABASE-APP-J89 Related to FE-3862
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
| // auth-js swaps `session.user` for a proxy that throws on any property access when `userStorage` | ||
| // is configured but no user is persisted yet. `__isUserNotAvailableProxy` is the one property it allows. | ||
| const isSessionUserAvailable = | ||
| !!sessionUser && | ||
| (sessionUser as { __isUserNotAvailableProxy?: boolean }).__isUserNotAvailableProxy !== true | ||
| const userEmail = isSessionUserAvailable | ||
| ? (sessionUser?.user_metadata?.email ?? sessionUser?.email) | ||
| : undefined | ||
| const params = useParams() |
There was a problem hiding this comment.
This one needs more checking and validation. Claude helped with this one!
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds defensive null/array safety checks across several studio components and data-fetching modules: table formatting guards against non-array inputs, log date pickers use new parsing utilities, several queries normalize responses to EMPTY_ARR, and feature-flag/proxy and route-param handling are hardened against undefined or unexpected shapes. ChangesDefensive Safety Guards
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🎭 Playwright Test Results
Details
Flaky testsFeatures › sql-editor.spec.ts › SQL Editor › should check if SQL editor is working as expected Skipped testsFeatures › auth-users.spec.ts › should show web3 users as enabled when the matching web3 provider is enabled |
| }) | ||
|
|
||
| const formattedViews = views.map((x) => { | ||
| const formattedViews = (Array.isArray(views) ? views : []).map((x) => { |
There was a problem hiding this comment.
these patches are alright for now, but im actually just wondering whether this guard should be at the react query level (looks a bit hacky otherwise, cause we're already trying to guard with default values + the data from the react query is expected to be an array if the request is successful)
trying to figure out when views (or the other ones) will return as a non-array too - we're currently using the pg-meta endpoints for views and materialized views so perhaps the next step is to refactor them to use direct SQL instead for better control
There was a problem hiding this comment.
stack traces point to views (and i'd assume materialized views as well) - so perhaps tables and foreign tables (both using raw SQL) are alright
| export const parseSelectedDate = (value?: string | null): Date | null => { | ||
| if (!value) return null | ||
| const parsed = dayjs(value) | ||
| return parsed.isValid() ? parsed.toDate() : null | ||
| } | ||
|
|
There was a problem hiding this comment.
this is just a thought but im wondering if we can merge this with resolveLogTimestamp given how similar they are
perhaps the API could be resolveLogTimestamp({ value?: string, defaultToNull: boolean })
| (q) => q.isError || (q.data !== undefined && q.data.pagination.count > q.data.projects.length) | ||
| (q) => | ||
| q.isError || | ||
| (q.data !== undefined && (q.data.pagination?.count ?? 0) > q.data.projects.length) |
There was a problem hiding this comment.
i'm just wondering when pagination would be undefined though if data is not undefined (which would imply that the data was successfully retrieved)
at least from an API response types POV - pagination is expected from the response
| } | ||
|
|
||
| return data | ||
| return Array.isArray(data) ? data : EMPTY_ARR |
There was a problem hiding this comment.
this also seems like a patch but it's probably worth also checking why data is not getting returned as an array from the API - the API types expect that data should be an array
There was a problem hiding this comment.
the error is from here which imo would only happen if this request is successfully but data is somehow not an array (which is unexpected)
| export const useIsAnalyticsBucketsEnabled = ({ projectRef }: { projectRef?: string }) => { | ||
| const { data } = useProjectStorageConfigQuery({ projectRef }) | ||
| const isIcebergCatalogEnabled = !!data?.features.icebergCatalog?.enabled | ||
| const isIcebergCatalogEnabled = !!data?.features?.icebergCatalog?.enabled |
There was a problem hiding this comment.
hmm likewise - the API types expect features to be present in data
so i'm wondering if for this case specifically, there's some projects that somehow don't have the features property in their storage config (and if so should be enforced via API types)
## Context Just extracting the fixes which I think are applicable from this [PR](#47695) Main files are - `apps/studio/hooks/analytics/useLogsQuery.tsx` - `packages/common/auth.tsx` - `packages/common/feature-flags.tsx` ## Changes involved - Adjust `useLogsQuery` to accept an object as prop, rather than 4 individual params - This one doesn't address any Sentry issues, but is just a improvement to the function's API imo, more readable - Adjust how user email is retrieved in `feature-flags` - Related Sentry issue [here](https://supabase.sentry.io/issues/7592718607/?project=5459134) - The error is a bit vague, but Claude's attempt to fix looks alright in general IMO - Minimally verified that feature flags are loading as expected still <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved log-related screens and queries for more reliable loading and filtering across the app. * Fixed profile and account data handling so identity details are retrieved more consistently. * Improved authentication handling to better recognize missing user data and keep the app stable. * Updated feature flag personalization to use more accurate account information. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Follow-up to #47460 (FE-3748) — same non-array/malformed-API-response bug class, for issues that weren't caught in that pass, plus a few unrelated one-offs found while triaging.
Resolves FE-3862
Issues fixed
pagination.count/projects.lengthon malformed org projects responseuseStatusPageBannerVisibility.ts+ normalizeorg-projects-infinite-query.tspagination.counton undefinedprojectsarray inprojects-infinite-query.ts?.after.featuresinproject-storage-config-query.tst?.someis not a functionservice-status-query.tsto always return an arrayh?.findis not a functiont.mapis not a functionTables.utils.tsformatAllEntitiesm.findis not a functionbranches-query.tsto always return an array?.afteruser_metadatainfeature-flags.tsxsession.userQ.toLowerCaseis not a functionqrouter param to a single string in Logs ExplorerDatein the Logs date pickerTest plan
tsc --noEmitcleaneslintclean on all changed filesproject-storage-config-query.test.tsx,Logs.Datepickers.test.tsx)Summary by CodeRabbit