fix(platform): handle unhandled promise rejections in query handlers#655
Merged
Conversation
Wrap async operations (storage.getUrl, db.get, runQuery) in try/catch blocks across branding, conversations, integrations, members, and team_members queries to prevent unhandled rejections from crashing reactive queries. Extract inline handlers into named testable functions where applicable.
Replace blanket catch-all blocks with targeted UnauthorizedError checks so unexpected failures (e.g. DB connection errors) propagate instead of being silently swallowed. Add diagnostic console.warn logging to remaining graceful-degradation catches and remove unnecessary try-catch wrappers in transform_conversation. Add test coverage for auth edge cases and error re-throwing.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughThis pull request introduces comprehensive error handling across multiple Convex query modules in the platform service. It adds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/convex/integrations/__tests__/queries.test.ts`:
- Around line 116-254: Add tests for the getByName export mirroring the existing
get/list suites: import the getByName handler from ../queries and add tests that
(1) return null when mockedGetAuthUser resolves to null, (2) return null when
mockedGetAuthUser exists but mockedGetOrgMember rejects with UnauthorizedError,
(3) re-throw when mockedGetOrgMember rejects with a non-authorization Error, and
(4) on success return the integration with iconUrl (and null iconUrl when
ctx.storage.getUrl rejects); reuse the same mocks used in this file
(mockedGetAuthUser, mockedGetIntegration or mockedGetIntegrationByName,
mockedGetOrgMember, ctx.storage.getUrl) so the tests mirror the patterns in the
get and list handler tests.
In `@services/platform/convex/members/queries.ts`:
- Around line 130-140: The assignments displayName = userResult?.name and email
= userResult?.email can pass null/non-string values to the response; update the
code after the ctx.runQuery call (the call using
components.betterAuth.adapter.findOne and the local variable userResult) to
normalize values so displayName and email are either strings or undefined (e.g.,
set displayName = typeof userResult?.name === 'string' ? userResult.name :
undefined and same for email) before returning, ensuring compatibility with
v.optional(v.string()) validation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5295f1d-26a4-49b5-a289-fd8d5555d3e9
📒 Files selected for processing (8)
services/platform/convex/branding/__tests__/queries.test.tsservices/platform/convex/branding/queries.tsservices/platform/convex/integrations/__tests__/queries.test.tsservices/platform/convex/integrations/queries.tsservices/platform/convex/members/__tests__/queries.test.tsservices/platform/convex/members/queries.tsservices/platform/convex/team_members/__tests__/queries.test.tsservices/platform/convex/team_members/queries.ts
Comment on lines
+116
to
+254
| describe('get handler', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('returns null when not authenticated', async () => { | ||
| mockedGetAuthUser.mockResolvedValue(null); | ||
| const ctx = createMockCtx(); | ||
| const { get } = await import('../queries'); | ||
| const handler = (get as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { integrationId: 'int_1' }); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('returns null when unauthorized', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| mockedGetIntegration.mockResolvedValue(makeIntegrationDoc()); | ||
| mockedGetOrgMember.mockRejectedValue(new UnauthorizedError()); | ||
| const ctx = createMockCtx(); | ||
| const { get } = await import('../queries'); | ||
| const handler = (get as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { integrationId: 'int_1' }); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('re-throws non-authorization errors from getOrganizationMember', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| mockedGetIntegration.mockResolvedValue(makeIntegrationDoc()); | ||
| mockedGetOrgMember.mockRejectedValue(new Error('DB failure')); | ||
| const ctx = createMockCtx(); | ||
| const { get } = await import('../queries'); | ||
| const handler = (get as unknown as { handler: Function }).handler; | ||
|
|
||
| await expect(handler(ctx, { integrationId: 'int_1' })).rejects.toThrow( | ||
| 'DB failure', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns integration with icon URL on success', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| const doc = makeIntegrationDoc({ iconStorageId: 'storage_icon' }); | ||
| mockedGetIntegration.mockResolvedValue(doc); | ||
| mockedGetOrgMember.mockResolvedValue({} as never); | ||
| const ctx = createMockCtx(); | ||
| ctx.storage.getUrl.mockResolvedValue('https://storage.example.com/icon'); | ||
| const { get } = await import('../queries'); | ||
| const handler = (get as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { integrationId: 'int_1' }); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| _id: 'int_1', | ||
| iconUrl: 'https://storage.example.com/icon', | ||
| }); | ||
| }); | ||
|
|
||
| it('returns null iconUrl when storage fetch fails', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| const doc = makeIntegrationDoc({ iconStorageId: 'storage_icon' }); | ||
| mockedGetIntegration.mockResolvedValue(doc); | ||
| mockedGetOrgMember.mockResolvedValue({} as never); | ||
| const ctx = createMockCtx(); | ||
| ctx.storage.getUrl.mockRejectedValue(new Error('Storage unavailable')); | ||
| const { get } = await import('../queries'); | ||
| const handler = (get as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { integrationId: 'int_1' }); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| _id: 'int_1', | ||
| iconUrl: null, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('list handler', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('returns empty array when not authenticated', async () => { | ||
| mockedGetAuthUser.mockResolvedValue(null); | ||
| const ctx = createMockCtx(); | ||
| const { list } = await import('../queries'); | ||
| const handler = (list as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { organizationId: 'org_1' }); | ||
|
|
||
| expect(result).toEqual([]); | ||
| }); | ||
|
|
||
| it('returns empty array when unauthorized', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| mockedGetOrgMember.mockRejectedValue(new UnauthorizedError()); | ||
| const ctx = createMockCtx(); | ||
| const { list } = await import('../queries'); | ||
| const handler = (list as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { organizationId: 'org_1' }); | ||
|
|
||
| expect(result).toEqual([]); | ||
| }); | ||
|
|
||
| it('re-throws non-authorization errors from getOrganizationMember', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| mockedGetOrgMember.mockRejectedValue(new Error('System error')); | ||
| const ctx = createMockCtx(); | ||
| const { list } = await import('../queries'); | ||
| const handler = (list as unknown as { handler: Function }).handler; | ||
|
|
||
| await expect(handler(ctx, { organizationId: 'org_1' })).rejects.toThrow( | ||
| 'System error', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns integrations with icon URLs', async () => { | ||
| mockedGetAuthUser.mockResolvedValue({ userId: 'user_1' }); | ||
| mockedGetOrgMember.mockResolvedValue({} as never); | ||
| const docs = [ | ||
| makeIntegrationDoc({ _id: 'int_1', iconStorageId: 'sid_1' }), | ||
| makeIntegrationDoc({ _id: 'int_2' }), | ||
| ]; | ||
| mockedListIntegrations.mockResolvedValue(docs); | ||
| const ctx = createMockCtx(); | ||
| ctx.storage.getUrl.mockResolvedValueOnce('https://storage.example.com/i1'); | ||
| const { list } = await import('../queries'); | ||
| const handler = (list as unknown as { handler: Function }).handler; | ||
|
|
||
| const result = await handler(ctx, { organizationId: 'org_1' }); | ||
|
|
||
| expect(result).toHaveLength(2); | ||
| expect(result[0].iconUrl).toBe('https://storage.example.com/i1'); | ||
| expect(result[1].iconUrl).toBeNull(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add getByName handler tests for the new auth/error branches.
This suite validates get and list, but getByName received equivalent UnauthorizedError/rethrow logic in this PR and currently has no direct coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/integrations/__tests__/queries.test.ts` around lines
116 - 254, Add tests for the getByName export mirroring the existing get/list
suites: import the getByName handler from ../queries and add tests that (1)
return null when mockedGetAuthUser resolves to null, (2) return null when
mockedGetAuthUser exists but mockedGetOrgMember rejects with UnauthorizedError,
(3) re-throw when mockedGetOrgMember rejects with a non-authorization Error, and
(4) on success return the integration with iconUrl (and null iconUrl when
ctx.storage.getUrl rejects); reuse the same mocks used in this file
(mockedGetAuthUser, mockedGetIntegration or mockedGetIntegrationByName,
mockedGetOrgMember, ctx.storage.getUrl) so the tests mirror the patterns in the
get and list handler tests.
Comment on lines
+130
to
+140
| try { | ||
| const userResult = await ctx.runQuery( | ||
| components.betterAuth.adapter.findOne, | ||
| { | ||
| model: 'user', | ||
| where: [{ field: '_id', value: member.userId, operator: 'eq' }], | ||
| }, | ||
| ); | ||
| displayName = userResult?.name; | ||
| email = userResult?.email; | ||
| } catch (error) { |
Contributor
There was a problem hiding this comment.
Normalize displayName/email to strings before returning.
displayName = userResult?.name and email = userResult?.email can leak null/non-string values into a response validated by v.optional(v.string()), causing runtime validation failures.
💡 Proposed fix
- displayName = userResult?.name;
- email = userResult?.email;
+ displayName =
+ typeof userResult?.name === 'string' ? userResult.name : undefined;
+ email =
+ typeof userResult?.email === 'string' ? userResult.email : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/members/queries.ts` around lines 130 - 140, The
assignments displayName = userResult?.name and email = userResult?.email can
pass null/non-string values to the response; update the code after the
ctx.runQuery call (the call using components.betterAuth.adapter.findOne and the
local variable userResult) to normalize values so displayName and email are
either strings or undefined (e.g., set displayName = typeof userResult?.name ===
'string' ? userResult.name : undefined and same for email) before returning,
ensuring compatibility with v.optional(v.string()) validation.
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
storage.getUrl,db.get,runQuery) in try/catch blocks across branding, integrations, members, and team_members queries to prevent unhandled rejections from crashing reactive queriesUnauthorizedError, re-throwing unexpected errors so they propagate properly instead of being silently swallowedconsole.warnlogging to remaining graceful-degradation catchesTest plan
storage.getUrlordb.getfails transiently🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests