fix: non null assertion linter issues#3397
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis change set systematically removes non-null assertion operators ( Changes
Sequence Diagram(s)No sequence diagrams generated as the changes are primarily refactoring, error handling, and type safety improvements without significant control flow modifications or new features. Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learningsapps/dashboard/lib/trpc/routers/key/rbac/roles/schema-with-helpers.ts (4)internal/clickhouse/src/verifications.ts (2)⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ogzhanolguncu
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (65)
apps/api/src/integration/keys_updated_at_actually_updates.ts(3 hunks)apps/api/src/integration/sdk/create_key_with_permissions.ts(2 hunks)apps/api/src/pkg/db.ts(1 hunks)apps/api/src/pkg/key_migration/handler.ts(3 hunks)apps/api/src/pkg/middleware/metrics.ts(1 hunks)apps/api/src/pkg/ratelimit/client.ts(1 hunks)apps/api/src/pkg/ratelimit/do_client.ts(3 hunks)apps/api/src/pkg/util/retry.ts(1 hunks)apps/api/src/routes/legacy_keys_createKey.ts(2 hunks)apps/api/src/routes/v1_keys_whoami.ts(0 hunks)apps/dashboard/app/(app)/apis/[apiId]/select.tsx(0 hunks)apps/dashboard/app/(app)/identities/page.tsx(2 hunks)apps/dashboard/app/(app)/layout.tsx(1 hunks)apps/dashboard/app/(app)/settings/billing/client.tsx(5 hunks)apps/dashboard/app/(app)/settings/billing/page.tsx(2 hunks)apps/dashboard/app/(app)/settings/team/client.tsx(1 hunks)apps/dashboard/app/(app)/settings/team/invitations.tsx(2 hunks)apps/dashboard/app/(app)/settings/team/invite.tsx(2 hunks)apps/dashboard/app/(app)/settings/team/members.tsx(1 hunks)apps/dashboard/app/(app)/settings/vercel/client.tsx(3 hunks)apps/dashboard/app/(app)/settings/vercel/page.tsx(1 hunks)apps/dashboard/app/api/webhooks/stripe/route.ts(2 hunks)apps/dashboard/app/auth/actions.ts(1 hunks)apps/dashboard/app/auth/join/route.ts(1 hunks)apps/dashboard/app/integrations/vercel/callback/client.tsx(1 hunks)apps/dashboard/app/integrations/vercel/callback/exchange-code.tsx(1 hunks)apps/dashboard/app/integrations/vercel/callback/page.tsx(2 hunks)apps/dashboard/app/integrations/vercel/callback/workspace.tsx(1 hunks)apps/dashboard/app/new/keys.tsx(1 hunks)apps/dashboard/app/new/page.tsx(4 hunks)apps/dashboard/components/navbar-popover.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx(2 hunks)apps/dashboard/components/navigation/sidebar/team-switcher.tsx(2 hunks)apps/dashboard/components/navigation/sidebar/usage-banner.tsx(2 hunks)apps/dashboard/lib/auth.ts(1 hunks)apps/dashboard/lib/auth/server.ts(1 hunks)apps/dashboard/lib/auth/workos.ts(5 hunks)apps/dashboard/lib/searchparams.tsx(0 hunks)apps/dashboard/lib/trpc/routers/api/delete.ts(2 hunks)apps/dashboard/lib/trpc/routers/authorization/roles/keys/schema-with-helpers.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/createRootKey.ts(2 hunks)apps/dashboard/lib/trpc/routers/rbac.ts(6 hunks)apps/dashboard/lib/trpc/routers/rbac/upsertPermission.ts(1 hunks)apps/dashboard/lib/trpc/routers/stripe/createSubscription.ts(3 hunks)apps/dashboard/lib/trpc/routers/stripe/updateSubscription.ts(2 hunks)apps/dashboard/lib/trpc/routers/vercel.ts(11 hunks)apps/dashboard/lib/trpc/trpc.ts(3 hunks)apps/dashboard/pages/api/v1/github/verify.ts(3 hunks)apps/dashboard/providers/PostHogProvider.tsx(2 hunks)apps/workflows/src/workflows/invoicing.ts(9 hunks)biome.json(2 hunks)internal/clickhouse/src/keys/active_keys.ts(1 hunks)internal/clickhouse/src/logs.ts(1 hunks)internal/clickhouse/src/ratelimits.ts(1 hunks)internal/clickhouse/src/verifications.ts(1 hunks)internal/db/drizzle.config.ts(1 hunks)internal/keys/src/util.ts(1 hunks)internal/ui/src/components/button.tsx(1 hunks)packages/cache/src/examples/encryption.ts(2 hunks)packages/cache/src/examples/namespaces.ts(2 hunks)packages/cache/src/examples/stale-while-revalidate.ts(1 hunks)packages/cache/src/examples/tiered.ts(1 hunks)packages/cache/src/stores/memory.ts(1 hunks)packages/cache/src/tiered.ts(1 hunks)packages/rbac/src/queries.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/routes/v1_keys_whoami.ts
- apps/dashboard/lib/searchparams.tsx
- apps/dashboard/app/(app)/apis/[apiId]/select.tsx
🔇 Additional comments (101)
biome.json (2)
28-28: LGTM! Enforcing non-null assertion rule globally.This rule enforcement will improve type safety across the codebase by preventing unsafe non-null assertions.
130-147: Well-designed overrides for the non-null assertion rule.The overrides appropriately allow non-null assertions in test files, tools, and legacy versioned files where they might be more acceptable or necessary.
apps/api/src/pkg/middleware/metrics.ts (1)
154-154: Good removal of non-null assertion with proper null handling.Removing the non-null assertion is correct here since
key.forWorkspaceIdcan be null, and the function already handles null values properly with the null coalescing operator at line 163 (return val ?? "").apps/api/src/integration/sdk/create_key_with_permissions.ts (1)
32-33: LGTM: Appropriate use of lint suppression comment.The lint suppression comment clearly indicates this non-null assertion is intentionally kept. In test contexts where the setup controls the conditions, this approach is acceptable.
internal/keys/src/util.ts (1)
12-15: LGTM: Safer handling of optional parameter.Removing the non-null assertion on
opts.prefixis correct since it's defined as optional in the function signature. The KeyV1 constructor should handle undefined values appropriately.apps/dashboard/providers/PostHogProvider.tsx (2)
10-11: LGTM: Appropriate handling of required environment variable.The lint suppression comment clearly documents that this non-null assertion is intentional. For required environment variables that the application cannot function without, this approach is acceptable.
56-59: LGTM: Improved type annotation formatting.The multi-line type annotation improves readability without changing functionality.
internal/ui/src/components/button.tsx (1)
345-349: LGTM: Defensive programming with optional chaining.While the early guard clause
if (!props.keyboard || isClickDisabled)already prevents execution whenprops.keyboardis undefined, using optional chaining provides additional safety and makes the code more defensive against future changes.apps/api/src/pkg/db.ts (1)
60-60: LGTM: Safer error message access.Using optional chaining instead of non-null assertion is more defensive. In the current logic,
lastErrorshould always be defined at this point due to the retry loop structure, but optional chaining provides additional safety.packages/cache/src/tiered.ts (1)
52-57: Excellent refactor to improve type safety.The extraction of
res.valto a local variablevaland explicit undefined check is a great improvement over the previous non-null assertion pattern. This makes the code more readable and safer while maintaining the same logic flow.packages/cache/src/stores/memory.ts (1)
88-88: Good consistency improvement.Removing the non-null assertion is appropriate here since the existence of
unstableEvictOnSetis already validated in the outer conditional on line 73. This change improves consistency with the codebase's move away from non-null assertions.internal/clickhouse/src/logs.ts (1)
280-284: Excellent defensive programming improvement.The explicit runtime check with a descriptive error message is much better than the previous non-null assertion. This change prevents silent failures and makes debugging easier when an unknown interval is encountered.
packages/rbac/src/queries.ts (1)
39-39: Appropriate removal of unnecessary non-null assertion.The non-null assertion was redundant here since lines 36-38 explicitly ensure
acc[rule]exists as an array. TypeScript can infer the type safety from the explicit initialization, making this change both safe and cleaner.apps/dashboard/app/(app)/identities/page.tsx (1)
27-27: Good defensive programming with safe fallbacks.The removal of the non-null assertion combined with the
?? 0fallback when passing to the Results component is a good pattern. This allows for more flexible internal handling while ensuring downstream components always receive valid numeric values.Also applies to: 57-57
apps/dashboard/lib/trpc/routers/authorization/roles/keys/schema-with-helpers.ts (1)
45-55: Excellent use of type predicates for improved type safety!The replacement of non-null assertions with a type predicate function is a great improvement. This approach:
- Provides compile-time type safety by narrowing the type
- Makes the null-filtering logic explicit and self-documenting
- Eliminates the risk of runtime errors from non-null assertions
apps/dashboard/components/navbar-popover.tsx (1)
170-173: Verify the behavior whenrowVirtualizermethods return undefined.While optional chaining is safer than non-null assertions, in this context
rowVirtualizershould always be defined whenuseVirtualis true. The optional chaining could potentially cause undefined behavior:
rowVirtualizer?.getTotalSize()could returnundefined, leading toheight: "undefinedpx"rowVirtualizer?.getVirtualItems()could returnundefined, causing the.map()call to failConsider adding explicit runtime checks instead:
- height: `${rowVirtualizer?.getTotalSize()}px`, + height: `${rowVirtualizer?.getTotalSize() ?? 0}px`,- {rowVirtualizer?.getVirtualItems().map((virtualRow) => { + {(rowVirtualizer?.getVirtualItems() ?? []).map((virtualRow) => {Or verify that the current approach handles these edge cases properly.
apps/dashboard/lib/auth/server.ts (1)
59-63: Great improvement in error handling and configuration validation.The explicit runtime check after initialization is much safer than relying on non-null assertions. This approach:
- Provides clear error messaging when initialization fails
- Helps identify configuration issues more easily
- Prevents potential runtime errors from returning null instances
apps/dashboard/app/integrations/vercel/callback/exchange-code.tsx (2)
23-28: Good optimization and safety improvement!Caching the environment variables and adding explicit validation provides several benefits:
- Performance: Avoids repeated calls to
vercelIntegrationEnv()- Safety: Prevents API calls with undefined credentials
- Debugging: Clear error message helps identify configuration issues
36-37: LGTM - Using cached environment variables.The use of cached
envvalues is consistent with the validation added above and improves performance.apps/dashboard/app/new/keys.tsx (1)
84-103: LGTM! Excellent defensive programming improvements.The explicit null/undefined checks for
firstPart,prefix, andsuffixremove unsafe assumptions about string splitting behavior. The fallback logic properly handles edge cases while maintaining the masking functionality.apps/workflows/src/workflows/invoicing.ts (3)
65-68: Good refactoring for clarity and safety.Extracting
stripeCustomerIdto a local constant improves readability and removes repeated property access. The explicit null check prevents potential runtime errors.
130-131: Consistent pattern for subscription property access.Extracting subscription properties (
plan,verifications,ratelimits,support) to local constants follows the same pattern asstripeCustomerIdand improves code consistency.Also applies to: 147-148, 172-173, 197-198
301-301: Safe removal of non-null assertion.Removing the non-null assertion on
tier.centsPerUnitaligns with the PR objective. The conditional check on line 292 already ensurestier.centsPerUnitis truthy before this code executes.apps/dashboard/pages/api/v1/github/verify.ts (2)
118-123: Good defensive programming with early return.The explicit check for
ws.orgIdprevents potential runtime errors when callinggetUsers. The early return with a 201 status maintains the API contract while handling the edge case gracefully.
233-250: Excellent refactoring for error resilience.The new implementation using
Promise.allwith individual try-catch blocks provides better concurrency and graceful error handling. Failed user fetches no longer break the entire operation, and the filtering ensures only valid users are returned.apps/dashboard/lib/auth/workos.ts (4)
73-74: Good documentation for static property.The comment warns about implicit external access to the static
instanceproperty, which helps prevent accidental modifications.
150-153: Explicit validation improves reliability.Adding the explicit check for
refreshResult.sealedSessionwith a clear error message prevents potential runtime errors and improves debugging when session refresh fails.
303-303: Comprehensive validation for organization switching.The enhanced condition now validates all required properties (
authenticated,session, andsealedSession) before proceeding, ensuring safe access to these properties.
353-354: Appropriate use of linter ignore comment.The biome-ignore comment indicates this non-null assertion is intentionally safe, as the
orgMap.get()call is guaranteed to find the organization due to the priorPromise.allconstruction.apps/dashboard/app/auth/join/route.ts (1)
37-43: Good defensive programming for invitation acceptance.The explicit check for
organizationIdprevents attempting to accept invitations or switch organizations when the required ID is missing. The error logging helps with debugging while maintaining the redirect flow.apps/dashboard/lib/trpc/routers/rbac/upsertPermission.ts (1)
57-58: LGTM! Lint suppression is appropriately used here.The non-null assertion on
ctx.user!.idis likely safe in this TRPC router context, as authentication middleware typically ensures the user exists. However, consider making the comment more descriptive about why this assertion is safe (e.g., "Protected route ensures user exists").apps/dashboard/app/(app)/layout.tsx (1)
39-39: Good improvement in null safety!Removing the non-null assertion allows the
AppSidebarcomponent to properly handle potentially null quotas, which is safer than forcing a non-null type. This aligns well with the related component updates mentioned in the AI summary.apps/dashboard/app/(app)/settings/team/invitations.tsx (1)
25-25: LGTM! Cleaner code by trusting the type system.Removing the non-null assertions on
organization.idmakes the code cleaner and relies on the TypeScript type system. This should be safe assuming theOrganizationtype properly definesidas a required field.Let me verify that the Organization type has
idas a required field:#!/bin/bash # Search for Organization type definition to confirm id is required ast-grep --pattern 'type Organization = { $$$ }' ast-grep --pattern 'interface Organization { $$$ }'Also applies to: 82-82
apps/api/src/pkg/ratelimit/client.ts (1)
135-136: Lint suppression is justified by the validation logic.The non-null assertion on
res[0].val!is safe here because:
- The code checks
res.length > 0before accessingres[0]- Previous validation ensures all results are successful (have
.val)The suppression comment could be more descriptive, but the assertion is sound given the surrounding validation logic.
packages/cache/src/examples/namespaces.ts (2)
26-29: LGTM: Appropriate lint suppressions for environment variablesThe lint ignore comments are well-justified for environment variables in example code. The comment "Safe to leave" clearly indicates these are intentional non-null assertions where the developer is expected to provide the required environment variables.
65-69: LGTM: Improved code formattingThe multi-line formatting enhances readability without affecting functionality.
apps/api/src/pkg/util/retry.ts (1)
39-40: LGTM: Justified lint suppression for guaranteed non-null valueThe lint ignore comment is appropriate here since the loop structure guarantees that
resultwill be assigned at least once before reaching the return statement. The non-null assertion is safe in this context.apps/dashboard/app/(app)/settings/team/client.tsx (1)
101-103: LGTM: Safe removal of non-null assertionsThe non-null assertions are safely removed since the early return at lines 55-57 ensures that
organizationis defined when these lines execute. This change improves type safety without introducing runtime risks.apps/dashboard/app/(app)/settings/team/members.tsx (1)
111-111: LGTM: Safe removal of non-null assertionThe non-null assertion is safely removed since the
organizationparameter is typed asOrganization(non-nullable) in the component props. The TypeScript type system ensures thatorganizationcannot be null when this component is used.apps/dashboard/lib/auth.ts (1)
56-56: LGTM: Safe removal of non-null assertion with proper error handlingThe non-null assertion is safely removed since the
getAuth()function guarantees thatuserIdwill be present by redirecting to the sign-in page if it's missing (lines 31-33). The inline comment correctly documents this behavior.packages/cache/src/examples/encryption.ts (2)
27-30: LGTM: Appropriate use of lint suppression for example code.The biome-ignore comments are suitable for example code where environment variables are expected to be configured. However, consider adding runtime validation if this pattern is used in production code.
39-40: LGTM: Consistent lint suppression approach.The lint suppression is consistent with the pattern used for other environment variables in this example.
apps/dashboard/app/integrations/vercel/callback/client.tsx (2)
164-167: Excellent safety improvement with clear error messaging.The guard clause prevents potential runtime errors by validating
projectIdbefore proceeding with the mutation. The toast error provides clear feedback to the user.
170-170: Good removal of non-null assertion.Removing the non-null assertion is safe here since the guard clause above ensures
projectIdis defined at this point.apps/dashboard/app/integrations/vercel/callback/page.tsx (1)
113-113: Safe removal of non-null assertion.The non-null assertion removal is safe since the explicit check above ensures
props.searchParams.nextexists.apps/dashboard/lib/trpc/routers/stripe/createSubscription.ts (3)
19-23: Good error handling consistency.The reformatted error throwing maintains consistency with other error handling patterns in the codebase.
39-44: Excellent validation for critical Stripe data.This explicit validation prevents runtime errors when creating subscriptions if the Stripe product lacks a default price. The error message is descriptive and includes the product ID for debugging.
79-79: Safe removal of non-null assertion.The non-null assertion removal is safe here since the validation above ensures
product.default_priceexists before this line is reached.apps/api/src/pkg/ratelimit/do_client.ts (2)
67-68: Appropriate lint suppression for safe non-null assertion.The lint suppression is justified here since the preceding
if (res.length > 0)check ensuresres[0]exists. The non-null assertion is actually safe in this context.
203-207: Good formatting improvement for readability.The multi-line JSON object formatting improves code readability without affecting functionality.
Also applies to: 218-222
apps/dashboard/lib/trpc/routers/key/createRootKey.ts (1)
47-48: LGTM: Safe keyAuthId handling implemented correctly.The extraction of
keyAuthIdto a local variable followed by explicit null checking is a good improvement over non-null assertions. This pattern prevents runtime errors and improves code readability.Also applies to: 67-67
apps/api/src/pkg/key_migration/handler.ts (2)
23-27: LGTM: Improved readability for ConsoleLogger instantiation.The reformatting enhances code readability while maintaining functionality.
38-38: LGTM: Proper defensive programming with nullish coalescing.Using
?? []ensures thatinArray()always receives an array instead of potentially undefined/null values, preventing runtime errors during database queries.Also applies to: 58-62
internal/clickhouse/src/ratelimits.ts (1)
114-118: LGTM: Excellent runtime validation for intervalUnit.The explicit check with a descriptive error message prevents runtime errors and makes debugging easier if an unknown interval step is encountered.
internal/clickhouse/src/keys/active_keys.ts (1)
156-160: LGTM: Consistent runtime validation pattern.The explicit validation for
msPerUnitfollows the same robust pattern used in other ClickHouse modules, ensuring consistent error handling across the codebase.apps/dashboard/app/(app)/settings/vercel/page.tsx (1)
120-148: LGTM: Comprehensive error handling improvement for user fetching.The refactored user fetching logic demonstrates excellent defensive programming:
- Filters out falsy IDs before processing
- Wraps async operations in try-catch blocks for graceful error handling
- Logs errors appropriately for debugging
- Filters out failed fetches to prevent null entries
- Provides better fallback user names
This significantly improves the robustness and error resilience of the user data handling.
apps/dashboard/app/(app)/settings/billing/page.tsx (2)
135-135: Excellent refactoring to use dedicated mapping function.The extraction of inline mapping logic to the
mapProductfunction improves code clarity and enables comprehensive validation of Stripe product data.
188-214: Outstanding defensive programming with comprehensive validation.The
mapProductfunction implements robust validation at each step:
- Validates
default_priceexistence before use- Ensures price expansion rather than string references
- Confirms
unit_amountis present- Safely parses metadata values
This approach significantly improves error detection and prevents runtime failures compared to non-null assertions.
apps/dashboard/app/auth/actions.ts (2)
92-101: Excellent environment variable validation and error handling.The explicit check for
UNKEY_ROOT_KEYwith proper error logging and user-friendly error messages significantly improves reliability. The approach maintains security by not exposing sensitive configuration details while providing clear feedback about service availability.
107-107: Good practice using cached environment variable.Using the pre-validated
unkeyRootKeyvariable eliminates the need for non-null assertions and ensures consistency with the earlier validation logic.apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx (2)
32-32: Improved type safety with explicit null handling.Changing from
quotas?: Quotastoquotas: Quotas | nullmakes the nullable nature explicit and forces proper handling of null cases throughout the component tree.
125-125: Consistent with updated type definition.Removing the non-null assertion aligns perfectly with the updated type signature that explicitly allows null values.
apps/dashboard/app/integrations/vercel/callback/workspace.tsx (2)
39-42: Excellent early validation with user-friendly fallback.The explicit check for
memberships?.datawith error logging and fallback UI prevents potential runtime errors while maintaining a good user experience.
51-66: Comprehensive session handling with robust error recovery.The enhanced validation and error handling significantly improves reliability:
- Validates session data completeness before use
- Wraps cookie operations in try-catch for graceful failure handling
- Provides clear user feedback through toast notifications
- Maintains proper error logging for debugging
This approach ensures the workspace switching process fails gracefully rather than throwing unhandled exceptions.
apps/dashboard/lib/trpc/routers/api/delete.ts (2)
45-52: Excellent explicit validation with informative error handling.The explicit check for
keyAuthIdprevents potential runtime errors and provides a clear, actionable error message. This is much safer than relying on non-null assertions and helps identify configuration issues early.
81-89: Consistent use of validated variable eliminates assertion risks.Using the pre-validated
keyAuthIdvariable throughout the transaction eliminates the need for non-null assertions while maintaining the same functional behavior.apps/dashboard/lib/trpc/routers/rbac.ts (3)
145-145: LGTM! Non-null assertion removal is safe here.The removal of non-null assertions (
!) fromctx.user.idis appropriate since these routes use therequireUsermiddleware, which should guarantee thatctx.useris defined. The direct access toctx.user.idis safer than the previous forced assertion.Also applies to: 225-225, 280-280, 358-358
813-820: Good defensive programming with explicit user validation.The explicit check for
ctx.user?.idwith a proper error throw is excellent defensive programming. This ensures the function fails gracefully if somehow a user context is missing, even though the middleware should prevent this scenario.
841-841: Consistent use of validated userId variable.Using the extracted and validated
userIdvariable maintains consistency with the validation logic above and eliminates any possibility of accessing undefined user data.apps/api/src/integration/keys_updated_at_actually_updates.ts (2)
19-19: Excellent improvement in test safety with optional chaining.Replacing non-null assertions with optional chaining (
?.) makes the test more robust by gracefully handling cases where database queries might return undefined. This prevents potential test failures due to runtime errors.Also applies to: 39-40, 54-54
41-43: Appropriate use of intentional non-null assertion.The retained non-null assertion on
createdAtMwith the lint ignore comment is reasonable here, ascreatedAtMshould always be present for existing database records in this test scenario.apps/dashboard/lib/trpc/routers/vercel.ts (1)
32-36: Improved error formatting enhances readability.The multi-line formatting of
TRPCErrorobjects with explicitcodeandmessageproperties improves code readability and maintainability. This is a good stylistic improvement.Also applies to: 38-42, 51-55, 57-61
apps/dashboard/app/(app)/settings/vercel/client.tsx (2)
35-35: Good library choice for consistent time formatting.Replacing the
mslibrary withformatDistanceToNowfromdate-fnsprovides more human-readable relative time display and likely aligns better with the project's existing dependencies. The implementation properly handles the conditional display whenupdatedAtMmight be null.Also applies to: 300-305
380-386: Excellent defensive programming prevents runtime errors.Adding the explicit check for
props.bindingbefore calling the mutation is excellent defensive programming. This prevents potential runtime errors if the binding becomes undefined due to race conditions or state updates, providing a better user experience with a clear error message.apps/dashboard/components/navigation/sidebar/team-switcher.tsx (3)
57-60: Essential validation prevents invalid session handling.Adding explicit validation for
sessionData.tokenandsessionData.expiresAtbefore proceeding is crucial for preventing runtime errors. This ensures the session switching fails gracefully with a clear error message if the backend returns incomplete data.
62-78: Robust error handling with try-catch wrapper.Wrapping the session cookie setting and cache invalidation operations in a try-catch block provides excellent error resilience. The specific error messages help with debugging, and the graceful fallback maintains a good user experience even when operations fail.
97-97: Safe dependency array without non-null assertion.Removing the non-null assertion from the
useMemodependency array is the correct approach. The function already handles the case whereuserMembershipsmight be undefined or empty, making the assertion unnecessary and potentially unsafe.apps/dashboard/lib/trpc/routers/stripe/updateSubscription.ts (5)
20-24: LGTM! Improved error formatting for consistency.The multiline error structure improves readability and follows consistent formatting patterns.
72-75: Excellent safety improvement by removing non-null assertion.The previous code used a non-null assertion on
i.plan.product!, which could cause runtime errors if the product field was null or undefined. The new implementation safely handles this case with optional chaining and explicit null checks.
84-89: Good addition of explicit validation for subscription item ID.This validation prevents potential runtime errors when calling the Stripe API with an undefined item ID, providing a clear error message for debugging.
91-96: Essential validation for product default price.This check prevents runtime errors when calling
toString()on a potentially undefineddefault_pricefield, ensuring the Stripe API call receives valid data.
98-99: Safe usage after proper validation.The code now safely uses
item.idandnewProduct.default_priceafter explicit validation checks, eliminating the need for non-null assertions.apps/dashboard/components/navigation/sidebar/usage-banner.tsx (3)
11-11: Good type update to handle null quotas.Changing the prop type to
Quotas | nullproperly reflects the actual data structure and prevents type mismatches.
21-21: Excellent addition of robust validation and error handling.The optional chaining prevents runtime errors when accessing
requestsPerMonth, and the comprehensive validation checks provide helpful debugging information while preventing the component from rendering with invalid data.Also applies to: 23-31
33-34: Good refactoring for improved readability.Extracting the percentage calculation and shouldUpgrade logic into variables makes the code more readable and maintainable.
apps/dashboard/lib/trpc/trpc.ts (3)
39-39: Safe replacement of non-null assertion with optional chaining.Using
ctx.user?.idinstead ofctx.user!.idprevents runtime errors if the user context is unexpectedly undefined.
115-116: Improved safety in rate limiting middleware.Extracting
userIdwith optional chaining and checking bothratelimitanduserIdbefore proceeding prevents potential runtime errors and makes the logic more explicit.Also applies to: 119-119
158-160: Consistent safety improvements in LLM access middleware.The same pattern of safe user ID extraction and explicit checks ensures the middleware handles undefined user contexts gracefully.
apps/dashboard/app/(app)/settings/billing/client.tsx (2)
163-172: Excellent addition of validation for currentProductId.This validation prevents the subscription update from proceeding with undefined
currentProductId, providing clear error logging and user feedback. This is a critical safety improvement that aligns perfectly with the PR objectives.
2-2: Good formatting improvements for consistency.The changes to import structure, return object formatting, mutation call formatting, and type annotations improve code readability and maintain consistent styling throughout the file.
Also applies to: 82-87, 173-176, 193-195, 321-324
apps/dashboard/app/new/page.tsx (3)
29-29: Good refactoring to extract search parameters.Extracting
apiIdandworkspaceIdto local constants improves readability and reduces repeated property access throughout the function.Also applies to: 66-66
40-43: Critical validation addition for keyAuthId.This validation prevents runtime errors by ensuring
api.keyAuthIdexists before passing it to theKeyscomponent. The error logging and 404 response provide appropriate handling for this edge case.
61-61: Safe usage after proper validation.Now that
api.keyAuthIdis validated above, it can be safely used without non-null assertions, eliminating potential runtime errors.apps/dashboard/app/(app)/settings/team/invite.tsx (4)
56-58: LGTM! Improved code formatting.The multiline formatting improves readability without changing functionality.
68-68: Excellent replacement of non-null assertion with optional chaining.This change aligns perfectly with the PR objective of removing risky non-null assertions. The optional chaining approach is much safer and prevents potential runtime errors.
73-78: Outstanding defensive programming with explicit validation.The explicit check for
user?.orgIdwith comprehensive error handling is a significant improvement over the previous non-null assertion approach. The error logging and user-friendly toast notification provide excellent developer and user experience.
79-88: Robust error handling with try-catch wrapper.The try-catch block properly handles potential mutation failures and provides consistent error reporting. This is a substantial improvement in error resilience compared to the previous implementation.
apps/dashboard/app/api/webhooks/stripe/route.ts (4)
18-21: Excellent explicit validation for Stripe environment configuration.The detailed error message clearly explains what's missing and helps with debugging configuration issues. This is a significant improvement over potential runtime failures from missing environment variables.
23-28: Outstanding environment variable validation with clear error messaging.The explicit check for
STRIPE_SECRET_KEYwith a descriptive error message is exemplary defensive programming. This prevents potential runtime errors and provides clear guidance for fixing configuration issues.
30-30: Safe usage of validated environment variable.Using the validated
stripeSecretKeyvariable instead of direct environment access is the perfect complement to the validation logic above.
86-122: Exceptional comprehensive error handling and validation.This refactoring demonstrates excellent defensive programming with:
- Try-catch wrapper for the entire operation
- Explicit validation of all required subscription data fields
- Concurrent API calls for better performance (
Promise.all)- Comprehensive validation of price and customer data
- Proper error logging and HTTP response handling
- Safe handling of union types (string vs object references)
The currency formatting and Slack alert integration are also well-implemented. This is a substantial improvement in robustness compared to the previous implementation.
mcstepp
left a comment
mcstepp
left a comment
There was a problem hiding this comment.
👍
just had a nitpick-y question, but nothing blocking
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/app/integrations/vercel/callback/page.tsx (1)
32-34: Good type safety improvement, but UI enhancement still needed.The explicit check for
props.searchParams.nextproperly prevents runtime errors by removing the non-null assertion. However, the fallback UI could still be improved as mentioned in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/settings/vercel/page.tsx(1 hunks)apps/dashboard/app/integrations/vercel/callback/page.tsx(2 hunks)apps/dashboard/app/new/keys.tsx(1 hunks)apps/dashboard/lib/trpc/routers/authorization/roles/keys/schema-with-helpers.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
Learnt from: mcstepp
PR: unkeyed/unkey#3242
File: apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/controls/components/logs-search/index.tsx:7-43
Timestamp: 2025-05-15T16:09:49.243Z
Learning: For type safety issues involving `any` type assertions, the team prefers to address these systematically with linter updates rather than fixing them individually in code reviews.
Learnt from: mcstepp
PR: unkeyed/unkey#3242
File: apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx:47-50
Timestamp: 2025-05-15T15:57:02.128Z
Learning: When reviewing code for Unkey, prefer using `Boolean()` over the double negation (`!!`) operator for boolean coercion, as their linter rules favor this pattern.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3375
File: apps/dashboard/app/(app)/settings/root-keys/components/table/hooks/use-root-keys-list-query.ts:0-0
Timestamp: 2025-06-25T20:32:10.471Z
Learning: In the Unkey codebase, ogzhanolguncu prefers strict validation with fail-fast error handling. When validation errors occur that shouldn't happen in normal operation (like invalid operators), throwing errors to crash the page is preferred over graceful error handling or console logging.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
apps/dashboard/app/integrations/vercel/callback/page.tsx (14)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3242
File: apps/dashboard/app/(app)/apis/[apiId]/_overview/components/table/components/override-indicator.tsx:50-65
Timestamp: 2025-05-15T16:26:08.666Z
Learning: In the Unkey dashboard, Next.js router (router.push) should be used for client-side navigation instead of window.location.href to preserve client state and enable smoother transitions between pages.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3311
File: apps/dashboard/components/logs/llm-search/components/search-input.tsx:14-14
Timestamp: 2025-06-10T14:21:42.413Z
Learning: In Next.js applications, importing backend modules into frontend components causes bundling issues and can expose server-side code to the client bundle. For simple constants like limits, it's acceptable to duplicate the value rather than compromise the frontend/backend architectural separation.
Learnt from: p6l-richard
PR: unkeyed/unkey#2085
File: apps/www/components/glossary/search.tsx:41-57
Timestamp: 2024-10-23T16:19:42.049Z
Learning: For the `FilterableCommand` component in `apps/www/components/glossary/search.tsx`, adding error handling and loading states to the results list is not necessary.
Learnt from: p6l-richard
PR: unkeyed/unkey#2085
File: apps/www/components/glossary/search.tsx:16-20
Timestamp: 2024-10-23T16:21:47.395Z
Learning: For the `FilterableCommand` component in `apps/www/components/glossary/search.tsx`, refactoring type definitions into an interface is not necessary at this time.
Learnt from: p6l-richard
PR: unkeyed/unkey#2085
File: apps/www/components/glossary/terms-stepper-mobile.tsx:16-20
Timestamp: 2024-10-23T16:25:33.113Z
Learning: In the `apps/www/components/glossary/terms-stepper-mobile.tsx` file, avoid suggesting to extract the term navigation logic into a custom hook, as the user prefers to keep the component straightforward.
Learnt from: Srayash
PR: unkeyed/unkey#2568
File: apps/dashboard/app/auth/sign-up/oauth-signup.tsx:25-25
Timestamp: 2024-10-25T23:53:41.716Z
Learning: In the React component `OAuthSignUp` (`apps/dashboard/app/auth/sign-up/oauth-signup.tsx`), adding a `useEffect` cleanup function to reset the `isLoading` state causes a "something went wrong" popup to appear before redirecting when a user clicks on signup.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3401
File: apps/dashboard/app/(app)/logs/filters.query-params.ts:10-0
Timestamp: 2025-06-24T13:29:10.129Z
Learning: The `buildQueryParams` function in `apps/dashboard/app/(app)/logs/filters.query-params.ts` calls `useFilters()` hook inside it, but this is valid because the function is only called from within other React hooks, maintaining the Rules of Hooks compliance.
Learnt from: chronark
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx:58-61
Timestamp: 2024-12-03T14:21:19.543Z
Learning: For the "Outcome" field in the `LogFooter` component (`apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx`), default to "N/A" instead of "VALID" when handling null values to avoid confusing customers.
Learnt from: chronark
PR: unkeyed/unkey#2180
File: apps/dashboard/lib/constants/workspace-navigations.tsx:85-88
Timestamp: 2024-10-04T20:47:34.791Z
Learning: In navigation code (e.g., `apps/dashboard/lib/constants/workspace-navigations.tsx`), prefer using `segments.at(0)` over `segments[0]` to handle cases when the `segments` array might be empty.
Learnt from: chronark
PR: unkeyed/unkey#2180
File: apps/dashboard/lib/constants/workspace-navigations.tsx:85-88
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In navigation code (e.g., `apps/dashboard/lib/constants/workspace-navigations.tsx`), prefer using `segments.at(0)` over `segments[0]` to handle cases when the `segments` array might be empty.
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2810
File: internal/ui/src/components/date-time/components/time-split.tsx:10-14
Timestamp: 2025-01-22T16:51:59.978Z
Learning: The DateTime component in internal/ui/src/components/date-time/components/time-split.tsx already includes sufficient validation through handleChange and handleBlur functions, making additional runtime validation unnecessary.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3156
File: apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/index.tsx:112-0
Timestamp: 2025-04-22T11:49:06.167Z
Learning: In the CreateKeyDialog component of Unkey, section navigation is designed to always allow progression even if validation fails, as visual indicators display the validation state of each section.
Learnt from: unrenamed
PR: unkeyed/unkey#2652
File: apps/www/components/particles.tsx:88-90
Timestamp: 2024-11-08T11:44:42.947Z
Learning: In React TypeScript components, if a function memoized with `useCallback` is not called elsewhere in the component, and a `useEffect` is used to invoke it, do not suggest removing the `useEffect` as it is necessary to execute the function.
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx:74-75
Timestamp: 2024-10-04T17:27:09.821Z
Learning: In `apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx`, the hidden `<input>` elements for `workspaceId` and `keyAuthId` work correctly without being registered with React Hook Form.
apps/dashboard/app/(app)/settings/vercel/page.tsx (3)
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
Learnt from: AkshayBandi027
PR: unkeyed/unkey#2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In `authorization/roles/[roleId]/update-role.tsx`, the tag `role-${role.id}` is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
Learnt from: chronark
PR: unkeyed/unkey#2143
File: apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx:58-61
Timestamp: 2024-12-03T14:21:19.543Z
Learning: For the "Outcome" field in the `LogFooter` component (`apps/dashboard/app/(app)/logs/components/log-details/components/log-footer.tsx`), default to "N/A" instead of "VALID" when handling null values to avoid confusing customers.
apps/dashboard/lib/trpc/routers/authorization/roles/keys/schema-with-helpers.ts (5)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3324
File: apps/dashboard/app/(app)/authorization/roles/components/table/components/actions/keys-table-action.popover.constants.tsx:17-18
Timestamp: 2025-06-19T11:48:05.070Z
Learning: In the authorization roles refactor, the RoleBasic type uses `roleId` as the property name for the role identifier, not `id`. This is consistent throughout the codebase in apps/dashboard/lib/trpc/routers/authorization/roles/query.ts.
Learnt from: AkshayBandi027
PR: unkeyed/unkey#2215
File: apps/dashboard/app/(app)/@breadcrumb/authorization/roles/[roleId]/page.tsx:28-29
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In `authorization/roles/[roleId]/update-role.tsx`, the tag `role-${role.id}` is revalidated after updating a role to ensure that the caching mechanism is properly handled for roles.
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
Learnt from: chronark
PR: unkeyed/unkey#2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
Learnt from: Flo4604
PR: unkeyed/unkey#3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.669Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
apps/dashboard/app/new/keys.tsx (2)
Learnt from: chronark
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: unkeyed/unkey#2693
File: apps/api/src/routes/v1_keys_updateKey.ts:350-368
Timestamp: 2024-11-29T15:15:47.308Z
Learning: In `apps/api/src/routes/v1_keys_updateKey.ts`, the code intentionally handles `externalId` and `ownerId` separately for clarity. The `ownerId` field will be removed in the future, simplifying the code.
🧬 Code Graph Analysis (1)
apps/dashboard/app/(app)/settings/vercel/page.tsx (1)
apps/dashboard/lib/auth/server.ts (1)
auth(67-67)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/integrations/vercel/callback/page.tsx (1)
112-112: Excellent removal of non-null assertion.The removal of the
!operator is now safe thanks to the explicit check added above. This change aligns perfectly with the PR objectives of eliminating non-null assertions in favor of safer validation patterns.apps/dashboard/app/new/keys.tsx (1)
80-103: LGTM! Excellent improvement to code safety.The refactoring of the
maskKeyfunction successfully addresses non-null assertion issues by:
- Explicit null checks: Adding
if (!firstPart)andif (!prefix || !suffix)prevents runtime errors from undefined array elements- Defensive programming: The function now gracefully handles edge cases like empty strings or malformed keys
- Maintained behavior: The masking logic remains the same for valid inputs while being more robust
The changes align perfectly with the PR objectives of improving code safety without changing functionality.
apps/dashboard/app/(app)/settings/vercel/page.tsx (1)
119-147: Excellent defensive programming and error handling improvements!The refactored user fetching logic effectively addresses the non-null assertion issues by:
- Deduplicating and filtering: Using
Setandfilter(Boolean)prevents unnecessary API calls for duplicate or falsy IDs- Robust error handling: The try-catch wrapper gracefully handles both missing users and API failures
- Type safety: The final filter ensures only valid user objects are processed
- User experience: Fallback to "Unknown User" provides clear feedback when user data is unavailable
This approach aligns well with the PR's goal of removing unsafe non-null assertions while maintaining functionality and improving code reliability.
apps/dashboard/lib/trpc/routers/authorization/roles/keys/schema-with-helpers.ts
Show resolved
Hide resolved
|
@mcstepp this is good to go |
|
cool cool, let me get some food in me and I'll take this up again |
|
🫡 🫡 |
mcstepp
left a comment
mcstepp
left a comment
There was a problem hiding this comment.
I'll allow the generic for lint bypass in the examples, but thrown errors need to actually have a useful message
2 participants
What does this PR do?
This PR fixes non null assertion issues in our codebase.
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Anything that matters should be tested. I didn't change anything critical, mostly added
throw new Error's to check in case those assertions are actually missing.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Revert