feat(email): use graphql for block email - #5499
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a paginated GraphQL query for email threads and messages. Maps GraphQL results to REST-compatible API models, including permissions, labels, contacts, attachments, and message metadata. Updates thread queries with GraphQL or REST transport selection, shared result types, selectors, pagination state, and refetch behavior. Adds tests for mapping, pagination, missing threads, error codes, permissions, labels, and attachment variants. decouples 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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 |
dev-rb
changed the title
…il-use-graphql-content
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/lib/queries/email/graphql/mapper.ts (1)
29-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
.otherwise()instead of.exhaustive()for a server-driven enum.
accessLeveloriginates from the GraphQL schema. If the server adds a new access level before the client regenerates types,.exhaustive()throws at runtime and the whole thread page fails to map. Line 26 already establishes view-only as the safe default. Applying the same default here keeps the mapper resilient while still failing type-check when the generated enum changes.♻️ Proposed refactor
return match<GraphqlEntityAccessLevel, AccessLevel>(permission.accessLevel) .with('VIEW', () => 'view') .with('COMMENT', () => 'comment') .with('EDIT', () => 'edit') .with('OWNER', () => 'owner') - .exhaustive(); + .otherwise(() => 'view');As per path instructions: "Use
matchfromts-patternfor exhaustive switch/case logic." The suggestion keepsmatchand only changes the fallback behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/queries/email/graphql/mapper.ts` around lines 29 - 34, Update the match expression mapping permission.accessLevel to use an otherwise fallback returning the existing view-only default instead of exhaustive(). Preserve the VIEW, COMMENT, EDIT, and OWNER mappings while ensuring unknown server-provided access levels resolve to view without throwing.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/lib/queries/email/graphql/thread-fetch.test.ts`:
- Around line 18-43: Align the thread test and implementation on one contract:
in apps/web/src/lib/queries/email/graphql/thread-fetch.test.ts:18-43, import and
test the intended existing symbol, or add the per-page fetch helper; in
apps/web/src/lib/queries/email/graphql/thread.ts:50-92, export that helper and
translate GraphQL error codes to ThrownResultError, or explicitly remove that
requirement and test the facade’s untranslated CombinedError behavior.
In `@apps/web/src/lib/queries/email/graphql/thread.ts`:
- Around line 68-78: Update getNextPageParam to avoid calling threadFromPage
when the latest page has a null emailThread: return undefined so the
fetch/select path handles the error. Derive the next offset from lastPageParam
plus the last page’s message count instead of reducing and recomputing all
pages, while preserving the DEFAULT_THREAD_MESSAGES_LIMIT boundary.
- Around line 50-92: Update createGraphqlEmailThreadQuery to translate
GraphQL/urql CombinedError results into urql-solid ThrownResultError instances,
preserving GraphQL extensions such as code === 'GONE'. Apply the mapping in the
query error path so consumers receive the facade’s expected error contract while
successful page mapping remains unchanged.
---
Nitpick comments:
In `@apps/web/src/lib/queries/email/graphql/mapper.ts`:
- Around line 29-34: Update the match expression mapping permission.accessLevel
to use an otherwise fallback returning the existing view-only default instead of
exhaustive(). Preserve the VIEW, COMMENT, EDIT, and OWNER mappings while
ensuring unknown server-provided access levels resolve to view without throwing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a67c2e03-60e6-4992-bcb9-6fc8b6285ce2
⛔ Files ignored due to path filters (1)
apps/web/src/lib/service-clients/service-storage/graphql/generated/graphql.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**
📒 Files selected for processing (7)
apps/web/src/lib/core/util/whenSettled.tsapps/web/src/lib/queries/email/graphql/mapper.test.tsapps/web/src/lib/queries/email/graphql/mapper.tsapps/web/src/lib/queries/email/graphql/thread-fetch.test.tsapps/web/src/lib/queries/email/graphql/thread.tsapps/web/src/lib/queries/email/thread.tsapps/web/src/lib/service-clients/service-storage/graphql/email-thread.graphql
| import { fetchGraphqlEmailThreadPage } from './thread'; | ||
|
|
||
| describe('fetchGraphqlEmailThreadPage', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mocks.query.mockReturnValue({ toPromise: mocks.toPromise }); | ||
| }); | ||
|
|
||
| it('fetches a cache-and-network page and maps the thread', async () => { | ||
| const graphqlThread = { id: 'thread-1' }; | ||
| const mappedThread = { db_id: 'thread-1', messages: [] }; | ||
| mocks.toPromise.mockResolvedValue({ | ||
| data: { user: { emailThread: graphqlThread } }, | ||
| }); | ||
| mocks.mapThread.mockReturnValue(mappedThread); | ||
|
|
||
| await expect(fetchGraphqlEmailThreadPage('thread-1', 20, 20)).resolves.toBe( | ||
| mappedThread | ||
| ); | ||
| expect(mocks.query).toHaveBeenCalledWith( | ||
| expect.anything(), | ||
| { threadId: 'thread-1', offset: 20, limit: 20 }, | ||
| { requestPolicy: 'cache-and-network' } | ||
| ); | ||
| expect(mocks.mapThread).toHaveBeenCalledWith(graphqlThread); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The tests and the implementation define two different GraphQL thread contracts. thread-fetch.test.ts targets a per-page helper named fetchGraphqlEmailThreadPage that calls client.query directly and translates GraphQL error codes into ThrownResultError. thread.ts implements only a reactive createGraphqlEmailThreadQuery factory and translates only the null-thread case. The suite therefore fails at import, and the error-code behavior is untested.
apps/web/src/lib/queries/email/graphql/thread-fetch.test.ts#L18-L43: import an existing symbol; either targetcreateGraphqlEmailThreadQueryor the new fetch helper once it exists.apps/web/src/lib/queries/email/graphql/thread.ts#L50-L92: export the per-page fetch helper with GraphQL error-code translation, or drop that requirement and document the untranslatedCombinedErrorpath for the facade.
📍 Affects 2 files
apps/web/src/lib/queries/email/graphql/thread-fetch.test.ts#L18-L43(this comment)apps/web/src/lib/queries/email/graphql/thread.ts#L50-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/lib/queries/email/graphql/thread-fetch.test.ts` around lines 18
- 43, Align the thread test and implementation on one contract: in
apps/web/src/lib/queries/email/graphql/thread-fetch.test.ts:18-43, import and
test the intended existing symbol, or add the per-page fetch helper; in
apps/web/src/lib/queries/email/graphql/thread.ts:50-92, export that helper and
translate GraphQL error codes to ThrownResultError, or explicitly remove that
requirement and test the facade’s untranslated CombinedError behavior.
| export function createGraphqlEmailThreadQuery<TData = GraphqlEmailThreadPages>( | ||
| threadId: Accessor<string>, | ||
| options: Accessor<GraphqlEmailThreadQueryOptions<TData>> | ||
| ): GraphqlEmailThreadQuery<TData> { | ||
| return createUrqlInfiniteQuery< | ||
| EmailThreadPageQuery, | ||
| EmailThreadPageQueryVariables, | ||
| number, | ||
| TData | ||
| >(() => ({ | ||
| query: EmailThreadPageDocument, | ||
| client: getGraphqlSoupClient(), | ||
| initialPageParam: 0, | ||
| variables: (offset) => ({ | ||
| threadId: threadId(), | ||
| offset, | ||
| limit: DEFAULT_THREAD_MESSAGES_LIMIT, | ||
| }), | ||
| getNextPageParam: (lastPage, pages) => { | ||
| if ( | ||
| threadFromPage(lastPage).messages.length < DEFAULT_THREAD_MESSAGES_LIMIT | ||
| ) { | ||
| return undefined; | ||
| } | ||
| return pages.reduce( | ||
| (sum, page) => sum + threadFromPage(page).messages.length, | ||
| 0 | ||
| ); | ||
| }, | ||
| enabled: options().enabled && threadId().length > 0, | ||
| requestPolicy: 'cache-and-network', | ||
| keepPreviousData: false, | ||
| select: ({ pages, pageParams }) => { | ||
| const mapped = { | ||
| pages: pages.map((page) => | ||
| mapGraphqlEmailThreadPage(threadFromPage(page)) | ||
| ), | ||
| pageParams: [...pageParams], | ||
| }; | ||
| return options().select?.(mapped) ?? (mapped as TData); | ||
| }, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect error handling contract for GraphQL email thread errors.
fd -t f 'urql-solid' -x echo {}
rg -n --type=ts -C4 'ThrownResultError|graphQLErrors|CombinedError' apps/web/src/lib/queries/email apps/web/src/lib/urql-solid 2>/dev/null
rg -n --type=ts -C4 "'GONE'|NOT_FOUND" apps/web/src/lib/queries/emailRepository: macro-inc/macro
Length of output: 32855
Translate GraphQL result errors to ThrownResultError from urql-solid.
createGraphqlEmailThreadQuery exposes urql CombinedError values through the query result. The thread fetch test expects codes like extensions.code === 'GONE' to arrive as ThrownResultError, not urql errors. Map GraphQL result errors in createGraphqlEmailThreadQuery so the facade and tests share the same error contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/lib/queries/email/graphql/thread.ts` around lines 50 - 92,
Update createGraphqlEmailThreadQuery to translate GraphQL/urql CombinedError
results into urql-solid ThrownResultError instances, preserving GraphQL
extensions such as code === 'GONE'. Apply the mapping in the query error path so
consumers receive the facade’s expected error contract while successful page
mapping remains unchanged.
| getNextPageParam: (lastPage, pages) => { | ||
| if ( | ||
| threadFromPage(lastPage).messages.length < DEFAULT_THREAD_MESSAGES_LIMIT | ||
| ) { | ||
| return undefined; | ||
| } | ||
| return pages.reduce( | ||
| (sum, page) => sum + threadFromPage(page).messages.length, | ||
| 0 | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not throw from getNextPageParam.
threadFromPage throws ThrownResultError when page.user.emailThread is null. getNextPageParam runs after a successful fetch, outside the fetch promise. A thread that becomes inaccessible during pagination then produces an unhandled throw in pagination bookkeeping instead of query error state. Return undefined for a null thread and let the fetch/select path report the error.
Also note the reduce recomputes lengths for all pages on every page load. Deriving the next offset from lastPageParam + lastPage messages length is cheaper and equivalent.
🛠️ Proposed fix
- getNextPageParam: (lastPage, pages) => {
- if (
- threadFromPage(lastPage).messages.length < DEFAULT_THREAD_MESSAGES_LIMIT
- ) {
- return undefined;
- }
- return pages.reduce(
- (sum, page) => sum + threadFromPage(page).messages.length,
- 0
- );
- },
+ getNextPageParam: (lastPage, pages) => {
+ const messages = lastPage.user.emailThread?.messages;
+ if (!messages || messages.length < DEFAULT_THREAD_MESSAGES_LIMIT) {
+ return undefined;
+ }
+ return pages.reduce(
+ (sum, page) => sum + (page.user.emailThread?.messages.length ?? 0),
+ 0
+ );
+ },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/lib/queries/email/graphql/thread.ts` around lines 68 - 78,
Update getNextPageParam to avoid calling threadFromPage when the latest page has
a null emailThread: return undefined so the fetch/select path handles the error.
Derive the next offset from lastPageParam plus the last page’s message count
instead of reducing and recomputing all pages, while preserving the
DEFAULT_THREAD_MESSAGES_LIMIT boundary.
1 participant
No description provided.