gmail: improve filter error logging and add limit check#1189
gmail: improve filter error logging and add limit check#1189
Conversation
…le object for create and update operations
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive Smart Filing feature that automatically organizes email attachments into cloud storage (Google Drive and OneDrive). It adds OAuth setup for both providers, database models for drive connections and filing records, complete filing orchestration with AI-powered folder selection, email notification workflows, and a multi-step UI for configuration and management. Changes
Sequence DiagramssequenceDiagram
participant User as User/Browser
participant App as Web App
participant AuthAPI as OAuth API<br/>(Google/Microsoft)
participant Server as Backend
participant DB as Database
participant Drive as Drive Provider<br/>(Google/OneDrive)
rect rgb(200, 220, 255)
Note over User,Drive: OAuth Drive Connection Flow
User->>App: Click "Connect Drive"
activate App
App->>Server: GET /api/google/drive/auth-url
activate Server
Server->>AuthAPI: Request OAuth consent URL
AuthAPI-->>Server: Return auth URL + state
Server->>DB: Store OAuth state in cache
Server-->>App: Return auth URL
deactivate Server
App->>User: Redirect to OAuth consent
deactivate App
User->>AuthAPI: Authenticate & authorize
AuthAPI->>User: Redirect to callback with code
User->>App: Navigate callback redirect
activate App
App->>Server: GET /api/google/drive/callback?code=...&state=...
activate Server
Server->>Server: Validate state & code
Server->>AuthAPI: Exchange code for tokens
AuthAPI-->>Server: accessToken, refreshToken
Server->>DB: Upsert DriveConnection record
Server->>DB: Mark connection as isConnected=true
Server-->>App: Redirect with success message
deactivate Server
App->>User: Show "Drive Connected"
deactivate App
end
sequenceDiagram
participant Webhook as Gmail Webhook
participant Server as Backend
participant Engine as Filing Engine
participant AI as AI Model
participant Provider as Drive Provider
participant DB as Database
participant Email as Email Service
rect rgb(220, 255, 220)
Note over Webhook,Email: Attachment Filing Workflow
Webhook->>Server: POST /api/google/webhook (has attachment)
activate Server
Server->>Server: Check filingEnabled & filingPrompt
Server->>Engine: processAttachment(email, attachment)
activate Engine
Engine->>Engine: Download & extract attachment text
Engine->>DB: Fetch saved filing folders
Engine->>AI: analyzeDocument(email, attachment, folders)
activate AI
AI-->>Engine: { action, folderId, confidence, reasoning }
deactivate AI
Engine->>Provider: Create folder if needed
Engine->>Provider: uploadFile(content, targetFolder)
Provider-->>Engine: { fileId, webUrl }
Engine->>DB: Create DocumentFiling record<br/>(status=FILED or PREVIEW)
Engine->>Email: sendFiledNotification(filing)
Email-->>Engine: Notification sent
deactivate Engine
Server-->>Webhook: 200 OK
deactivate Server
end
sequenceDiagram
participant User as User (Email)
participant Email as Email Service
participant Webhook as Filing Notification Handler
participant Server as Backend
participant Drive as Drive Provider
participant DB as Database
rect rgb(255, 240, 220)
Note over User,DB: Filing Reply & Correction Flow
User->>Email: Reply to filing notification<br/>with new folder path
Email->>Webhook: Inbound message with In-Reply-To
activate Webhook
Webhook->>Server: processFilingReply(message)
activate Server
Server->>DB: Find DocumentFiling via In-Reply-To
Server->>Server: Parse reply content<br/>extract folder path
Server->>Drive: Create folder path if needed
Server->>DB: Update DocumentFiling<br/>(folderPath, status=FILED)
Server->>Drive: Move file to new folder<br/>(if status was PREVIEW)
Server->>Email: sendCorrectionConfirmation(filing)
Email-->>Server: Confirmation sent
deactivate Server
Webhook-->>Email: Processing complete
deactivate Webhook
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
This change request has over 100 files and exceeds the review limit. Consider splitting it into smaller CRs for faster and more effective reviews |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Improve Gmail filter error logging and add a limit check in
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| return { | ||
| filings, | ||
| noAttachmentsFound: filings.length === 0, |
There was a problem hiding this comment.
noAttachmentsFound uses filings.length === 0, so it can report no attachments even when attachments existed but filing failed. Consider using messagesWithAttachments.length === 0 to only signal when none were found.
| noAttachmentsFound: filings.length === 0, | |
| noAttachmentsFound: messagesWithAttachments.length === 0, |
🚀 Want me to fix this? Reply ex: "fix it for me".
| const connection = connections[0]; | ||
| return { | ||
| driveConnection: connection, | ||
| folderId: "root", | ||
| folderPath: analysis.folderPath || "Inbox Zero Filed", | ||
| needsToCreateFolder: true, | ||
| }; |
There was a problem hiding this comment.
connections[0] may be undefined when connections is empty, leading to runtime errors. Consider guarding this path and fail fast (e.g., throw) or return a safe fallback before accessing connections[0].
| const connection = connections[0]; | |
| return { | |
| driveConnection: connection, | |
| folderId: "root", | |
| folderPath: analysis.folderPath || "Inbox Zero Filed", | |
| needsToCreateFolder: true, | |
| }; | |
| // Creating new folder or fallback - use first connection | |
| if (connections.length === 0) { | |
| throw new Error("No drive connections available"); | |
| } | |
| const connection = connections[0]; | |
| return { | |
| driveConnection: connection, | |
| folderId: "root", | |
| folderPath: analysis.folderPath || "Inbox Zero Filed", | |
| needsToCreateFolder: true, | |
| }; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| if (needsToCreateFolder && analysis.folderPath) { | ||
| log.info("Creating new folder", { path: analysis.folderPath }); | ||
| const newFolder = await createAndSaveFilingFolder({ | ||
| driveProvider, | ||
| folderPath: analysis.folderPath, | ||
| emailAccountId: emailAccount.id, | ||
| driveConnectionId: driveConnection.id, | ||
| logger: log, | ||
| }); | ||
| targetFolderId = newFolder.id; | ||
| targetFolderPath = analysis.folderPath; | ||
| } |
There was a problem hiding this comment.
Folder creation is skipped when analysis.folderPath is empty, but the record uses the fallback ("Inbox Zero Filed"), so the stored path doesn’t match the actual upload. Consider using targetFolderPath in the creation check and calls, ensuring the folder is created and the upload path matches the record.
| if (needsToCreateFolder && analysis.folderPath) { | |
| log.info("Creating new folder", { path: analysis.folderPath }); | |
| const newFolder = await createAndSaveFilingFolder({ | |
| driveProvider, | |
| folderPath: analysis.folderPath, | |
| emailAccountId: emailAccount.id, | |
| driveConnectionId: driveConnection.id, | |
| logger: log, | |
| }); | |
| targetFolderId = newFolder.id; | |
| targetFolderPath = analysis.folderPath; | |
| } | |
| if (needsToCreateFolder && targetFolderPath) { | |
| log.info("Creating new folder", { path: targetFolderPath }); | |
| const newFolder = await createAndSaveFilingFolder({ | |
| driveProvider, | |
| folderPath: targetFolderPath, | |
| emailAccountId: emailAccount.id, | |
| driveConnectionId: driveConnection.id, | |
| logger: log, | |
| }); | |
| targetFolderId = newFolder.id; | |
| } |
🚀 Want me to fix this? Reply ex: "fix it for me".
| const driveUrl = filing.fileId | ||
| ? getDriveFileUrl(filing.fileId, connection?.provider as DriveProviderType) | ||
| : null; |
There was a problem hiding this comment.
connection may be missing, so connection?.provider as DriveProviderType can pass undefined into getDriveFileUrl(), hitting the default case and returning undefined. Consider guarding for a missing provider and avoid the cast.
| const driveUrl = filing.fileId | |
| ? getDriveFileUrl(filing.fileId, connection?.provider as DriveProviderType) | |
| : null; | |
| const driveUrl = filing.fileId && connection?.provider | |
| ? getDriveFileUrl(filing.fileId, connection.provider) | |
| : null; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| } | ||
|
|
||
| // Step 7: Determine if we should ask the user first | ||
| const shouldAsk = analysis.confidence < 0.7; |
There was a problem hiding this comment.
Missing analysis.confidence makes undefined < 0.7 false, so we auto‑file. Consider treating missing confidence as low by checking for nullish before the compare.
| const shouldAsk = analysis.confidence < 0.7; | |
| const shouldAsk = analysis.confidence == null || analysis.confidence < 0.7; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| redirectHeaders = response.headers; | ||
|
|
||
| // Step 1.5: Check for duplicate OAuth code processing | ||
| const cachedResult = await getOAuthCodeResult(code); |
There was a problem hiding this comment.
Cached-code path redirects to /drive, but first-time path uses /{emailAccountId}/drive. Consider storing emailAccountId in the cached result and using buildDriveRedirectUrl(...) when returning the cached redirect so both paths resolve to the same URL.
🚀 Want me to fix this? Reply ex: "fix it for me".
| ); | ||
| } | ||
|
|
||
| // Save new tokens |
There was a problem hiding this comment.
Suggestion: Validate tokens.access_token after auth/refresh and throw a SafeError if missing before saving/returning.
| // Save new tokens | |
| // Save new tokens | |
| if (!tokens.access_token) { | |
| throw new SafeError( | |
| "Unable to access your drive. Please reconnect your drive and try again.", | |
| ); | |
| } |
🚀 Want me to fix this? Reply ex: "fix it for me".
| return; | ||
| } | ||
|
|
||
| const replyToAddress = getFilebotEmail({ userEmail }); |
There was a problem hiding this comment.
Suggestion: Guard filing.driveConnection before use and fail fast (log+return or throw SafeError) to avoid crashes when the connection is missing.
| const replyToAddress = getFilebotEmail({ userEmail }); | |
| if (!filing.driveConnection) { | |
| log.error("Drive connection not found"); | |
| return; | |
| } | |
| const replyToAddress = getFilebotEmail({ userEmail }); |
🚀 Want me to fix this? Reply ex: "fix it for me".
| ? folders | ||
| .map( | ||
| (f) => | ||
| `<folder id="${f.id}" path="${f.path}" provider="${f.driveProvider}" />`, |
There was a problem hiding this comment.
Suggestion: Escape all interpolated values in the prompt XML (folder attributes, filename, email_subject, email_sender) so "/&/</> don’t break the prompt.
| `<folder id="${f.id}" path="${f.path}" provider="${f.driveProvider}" />`, | |
| `<folder id="${String(f.id).replace(/&/g, "&").replace(/"/g, """).replace(/</g, "<").replace(/>/g, ">")}" path="${String(f.path).replace(/&/g, "&").replace(/"/g, """).replace(/</g, "<").replace(/>/g, ">")}" provider="${String(f.driveProvider).replace(/&/g, "&").replace(/"/g, """).replace(/</g, "<").replace(/>/g, ">")}" />`, |
🚀 Want me to fix this? Reply ex: "fix it for me".
| logger, | ||
| ); | ||
|
|
||
| const messages = await convertMessages(response.value, {}); |
There was a problem hiding this comment.
Passing {} to convertMessages yields empty/incorrect labelIds. Consider fetching folder IDs via getFolderIds(client, logger) and passing them instead.
🚀 Want me to fix this? Reply ex: "fix it for me".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/components/VideoCard.tsx (1)
140-159: Add caption tracks for accessibility.The video elements (both MuxPlayer and iframe) do not include caption tracks, which prevents users who are deaf or hard of hearing from accessing the video content. This is an accessibility requirement that blocks task completion for users who need captions.
Consider adding caption/subtitle support:
- For MuxPlayer: Add caption tracks via Mux dashboard and ensure they're exposed to the player
- For iframe videos: Include closed caption parameters in the URL or ensure the source provides captions
As per coding guidelines: "Include caption tracks for audio and video elements."
🧹 Nitpick comments (47)
apps/web/utils/prisma-extensions.ts (1)
340-419: LGTM!The
driveConnectionquery handlers correctly implement token encryption for all write operations (create, update, updateMany, upsert), following the established pattern used by other connection types.Optional: Consider extracting common encryption logic.
The token encryption/decryption handlers are nearly identical across
account,calendarConnection,mcpConnection, anddriveConnection. While this follows the existing pattern, you could consider extracting helper functions in a future refactor to reduce duplication:🔎 Example helper approach (for future consideration)
// Helper to encrypt token fields in data object function encryptDataTokens<T extends Record<string, unknown>>( data: T, fields: string[] ): T { for (const field of fields) { const value = data[field]; if (value) { if (typeof value === "string") { (data as Record<string, unknown>)[field] = encryptToken(value); } else if (typeof value === "object" && value !== null && "set" in value) { (value as { set: string }).set = encryptToken((value as { set: string }).set); } } } return data; }apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (2)
45-47: Enhance error tracking by including emailAccountId.The error handling approach is correct, but you should pass
emailAccountIdtocaptureExceptionfor better error correlation and debugging.🔎 Proposed enhancement
- captureException(error, { - extra: { context: "Google Calendar OAuth initiation" }, - }); + captureException(error, { + emailAccountId, + extra: { context: "Google Calendar OAuth initiation" }, + });
73-75: Enhance error tracking by including emailAccountId.Same as the Google Calendar handler above, pass
emailAccountIdtocaptureExceptionfor better error correlation.🔎 Proposed enhancement
- captureException(error, { - extra: { context: "Microsoft Calendar OAuth initiation" }, - }); + captureException(error, { + emailAccountId, + extra: { context: "Microsoft Calendar OAuth initiation" }, + });apps/web/components/drive/TableCellWithTooltip.tsx (1)
29-39: Simplify the ternary to reduce duplication.Both branches render nearly identical JSX with only the text wrapper differing. The
InfoIconis duplicated in both branches.🔎 Proposed refactor
- {truncate ? ( - <> - <span className="truncate">{text}</span> - <InfoIcon className="size-3.5 flex-shrink-0" /> - </> - ) : ( - <> - {text} - <InfoIcon className="size-3.5 flex-shrink-0" /> - </> - )} + {truncate && <span className="truncate">{text}</span>} + {!truncate && text} + <InfoIcon className="size-3.5 flex-shrink-0" />Or even more concise:
- {truncate ? ( - <> - <span className="truncate">{text}</span> - <InfoIcon className="size-3.5 flex-shrink-0" /> - </> - ) : ( - <> - {text} - <InfoIcon className="size-3.5 flex-shrink-0" /> - </> - )} + <span className={truncate ? "truncate" : undefined}>{text}</span> + <InfoIcon className="size-3.5 flex-shrink-0" />apps/web/components/drive/FilingStatusCell.tsx (2)
24-55: Addaria-hidden="true"to decorative icons for accessibility.The LoaderIcon and InfoIcon elements are decorative since the essential information is conveyed through the adjacent text. Screen readers should skip these icons to avoid redundant announcements.
🔎 Proposed accessibility improvements
if (status === "filing" || status === "pending") { return ( <span className="flex items-center gap-2 text-muted-foreground"> - <LoaderIcon className="size-4 animate-spin" /> + <LoaderIcon className="size-4 animate-spin" aria-hidden="true" /> <span>Analyzing...</span> </span> ); } if (status === "skipped") { const tooltipContent = `Skipped — ${skipReason || "Doesn't match preferences"}`; return ( <Tooltip content={tooltipContent}> <span className="flex items-center gap-1.5 text-muted-foreground italic"> Skipped - <InfoIcon className="size-3.5 flex-shrink-0" /> + <InfoIcon className="size-3.5 flex-shrink-0" aria-hidden="true" /> </span> </Tooltip> ); } if (status === "error") { const errorMessage = error || "Failed to file"; return ( <Tooltip content={errorMessage}> <span className="flex items-center gap-1.5 text-destructive"> {errorMessage} - <InfoIcon className="size-3.5 flex-shrink-0" /> + <InfoIcon className="size-3.5 flex-shrink-0" aria-hidden="true" /> </span> </Tooltip> ); }
57-72: Addaria-hidden="true"to InfoIcon for accessibility.Same as the previous status branches, the InfoIcon here is decorative and should be hidden from screen readers.
🔎 Proposed fix
// status === "filed" const displayPath = folderPath || "—"; return ( <Tooltip content={displayPath}> <span className={cn( "flex items-center gap-1.5 text-muted-foreground truncate", className, )} > <span className="truncate">{displayPath}</span> - <InfoIcon className="size-3.5 flex-shrink-0" /> + <InfoIcon className="size-3.5 flex-shrink-0" aria-hidden="true" /> </span> </Tooltip> );apps/web/components/StatsCards.tsx (1)
32-32: Remove commented-out code.The commented-out code on line 32 should be removed to keep the codebase clean. If this code might be needed in the future, rely on version control history instead.
🔎 Proposed cleanup
- {/* <p className="text-muted-foreground text-xs">{stat.subvalue}</p> */}apps/web/utils/drive/handle-filing-reply.test.ts (1)
3-27: Consider exporting private functions or testing through the public API.Reimplementing
parseFolderPathandisSkipCommandinline duplicates logic that exists in the source file. If the implementation inhandle-filing-reply.tschanges, these tests will still pass but won't catch regressions.Options to consider:
- Export these functions (perhaps with
@internalJSDoc) for direct testing- Test them indirectly through the public API of the module
- If keeping inline, add a comment linking to the source implementation to remind maintainers to keep them in sync
apps/web/components/ui/card.tsx (1)
51-61: Consider adding size variant to CardDescription for consistency.
CardDescriptiondoesn't have agroup-data-[size=sm]/card:*variant whileCardTitle,CardHeader,CardContent, andCardFooterall do. If the small size variant should also affect description text, you may want to add a corresponding class.apps/web/app/api/user/drive/folders/[folderId]/route.ts (1)
61-68: Consider selecting only required fields from driveConnection.The
findFirstquery fetches the entiredriveConnectionrecord, but onlyidandproviderare used in the response. Using Prisma'sselectoption would limit data exposure and improve efficiency:🔎 Proposed optimization
const driveConnection = await prisma.driveConnection.findFirst({ where: { id: driveConnectionId, emailAccountId, isConnected: true, }, + select: { + id: true, + provider: true, + accessToken: true, + refreshToken: true, + expiresAt: true, + }, });Note:
accessToken,refreshToken, andexpiresAtare needed forcreateDriveProviderWithRefresh.apps/web/utils/drive/handle-filing-reply.ts (2)
62-78: Consider combining ownership validation into the initial query.The current pattern fetches the filing by
notificationMessageIdfirst (line 62-68), then validates ownership separately (line 75-78). Per coding guidelines, database queries should include ownership filtering in WHERE clauses to prevent IDOR. While the subsequent check does prevent unauthorized access, combining them is more defensive.🔎 Suggested improvement
- const filing = await prisma.documentFiling.findUnique({ - where: { notificationMessageId: inReplyTo }, + const filing = await prisma.documentFiling.findFirst({ + where: { + notificationMessageId: inReplyTo, + emailAccountId, + }, include: { driveConnection: true, emailAccount: true, }, }); if (!filing) { - logger.error("Filing not found for In-Reply-To message", { inReplyTo }); + logger.error("Filing not found or unauthorized", { inReplyTo }); return; } - - if (filing.emailAccountId !== emailAccountId) { - logger.error("Filing does not belong to this email account"); - return; - }
121-145: Track TODO items for follow-up.There are two TODO comments indicating incomplete functionality:
- Line 123: File moving for corrections not implemented
- Line 143: Pending file upload when re-fetching attachments
These may affect user experience if corrections don't actually move files or if pending filings aren't properly uploaded.
Would you like me to open issues to track these incomplete implementations?
apps/web/utils/ai/document-filing/analyze-document.ts (2)
54-59: Consider importing DriveFolder type instead of redefining.The local
DriveFoldertype differs from the one in@/utils/drive/types.ts(which has optionalpathandwebUrl, but nodriveProvider). If this is intentional due to different requirements, consider using a distinct name to avoid confusion. Otherwise, extend or import the existing type.#!/bin/bash # Verify the DriveFolder type in the codebase rg -n "export interface DriveFolder" --type ts
80-85: Remove unnecessary...modelOptionsspread; model is already configured viacreateGenerateObject.The
...modelOptionsspread on line 81 is redundant. ThecreateGenerateObjectwrapper already receivesmodelOptionsand explicitly setsmodel: modelOptions.modelinside its implementation (see/utils/llms/index.tsline 184). SpreadingmodelOptionsagain adds unnecessary fields (provider,modelName,backupModel) that the AI SDK ignores. Remove the spread and pass only the required parameters:const result = await generateObject({ system: buildSystem(emailAccount.filingPrompt), prompt: buildPrompt({ email, attachment, folders }), schema: documentAnalysisSchema, });This pattern appears across all LLM feature functions; consider applying the same cleanup consistently.
apps/web/app/api/user/drive/preview/route.ts (1)
184-186: Consider documenting single-attachment-per-message behavior.The code only processes the first attachment from each message (line 185). While this makes sense for a preview (limiting scope), a brief comment would clarify this is intentional behavior, not an oversight.
🔎 Suggested comment
for (const { message, attachments } of messagesWithAttachments) { + // Process only the first attachment per message for preview const attachment = attachments[0];apps/web/hooks/useFilingActivity.ts (1)
7-10: Consider using URLSearchParams for safer query parameter construction.The manual string interpolation works but could be error-prone if parameters contain special characters or are undefined. Using URLSearchParams ensures proper encoding and makes the code more maintainable.
🔎 Proposed refactor using URLSearchParams
export function useFilingActivity({ limit, offset }: GetFilingsQuery) { - const url = `/api/user/drive/filings?limit=${limit}&offset=${offset}`; + const params = new URLSearchParams({ + limit: String(limit), + offset: String(offset), + }); + const url = `/api/user/drive/filings?${params.toString()}`; return useSWR<GetFilingsResponse>(url, { revalidateOnFocus: false }); }apps/web/utils/webhook/validate-webhook-account.ts (1)
181-185: Consider clarifying the log message.The validation logic correctly requires both
filingEnabledandfilingPromptto be truthy. However, the log message could be more descriptive to help with debugging.🔎 Proposed improvement to log message
const hasFilingEnabled = emailAccount.filingEnabled && !!emailAccount.filingPrompt; if (!hasAutomationRules && !hasFilingEnabled) { - logger.info("Has no rules enabled and filing not configured"); + logger.info("Has no rules enabled and filing not configured", { + hasRules: hasAutomationRules, + filingEnabled: emailAccount.filingEnabled, + hasFilingPrompt: !!emailAccount.filingPrompt, + }); return { success: false, response: NextResponse.json({ ok: true }) }; }apps/web/app/(app)/[emailAccountId]/drive/DriveOnboarding.tsx (2)
11-27: Consider extracting steps constant outside the component.The
stepsarray contains static configuration data that doesn't depend on props or state. Moving it outside the component improves performance by preventing recreation on each render.🔎 Proposed refactor
+"use client"; + +import { Card } from "@/components/ui/card"; +import { + PageSubHeading, + TypographyH3, + TypographyH4, +} from "@/components/Typography"; +import { ConnectDrive } from "./ConnectDrive"; + -const steps = [ +const ONBOARDING_STEPS = [ { number: 1, title: "Tell us how you organize", description: '"Receipts go to Expenses by month. Contracts go to Legal."', }, { number: 2, title: "Attachments get filed", description: "AI reads each document and files it to the right folder", }, { number: 3, title: "You stay in control", description: "Get an email when files are sorted—reply to correct", }, ]; export function DriveOnboarding() { return ( <div className="mx-auto max-w-xl py-8"> <TypographyH3 className="text-center"> Attachments filed automatically while you work </TypographyH3> <div className="mt-10 space-y-6"> - {steps.map((step) => ( + {ONBOARDING_STEPS.map((step) => (
37-49: Consider adding aria-label for numbered step indicators.The numbered pills (step indicators) rely solely on visual presentation. Adding
aria-labelattributes would improve screen reader accessibility by providing context for each step number.🔎 Proposed accessibility enhancement
{steps.map((step) => ( <div key={step.number} className="flex gap-4"> <div + aria-label={`Step ${step.number}`} className="flex size-8 shrink-0 items-center justify-center rounded-full bg-primary text-sm font-medium text-primary-foreground" > {step.number} </div>apps/web/app/(app)/[emailAccountId]/drive/FilingActivity.tsx (1)
82-84: Verify type safety of provider casting.The type assertion
connection?.provider as DriveProviderTypemay be unsafe ifconnectionis undefined orprovideris not a validDriveProviderType. WhilefileIdis checked, the provider type is not validated before casting.🔎 Consider adding a type guard
const connection = connections.find((c) => c.id === filing.driveConnectionId); - const driveUrl = filing.fileId - ? getDriveFileUrl(filing.fileId, connection?.provider as DriveProviderType) - : null; + const driveUrl = + filing.fileId && connection?.provider + ? getDriveFileUrl(filing.fileId, connection.provider as DriveProviderType) + : null;apps/web/prisma/migrations/20251221132935_drive/migration.sql (1)
77-89: Redundant index onnotificationToken.Line 77 creates a unique constraint/index on
notificationToken, and Line 89 creates an additional non-unique index on the same column. The unique constraint already provides index functionality, making the second index redundant and consuming extra storage.🔎 Proposed fix
--- Remove redundant index --- CreateIndex -CREATE INDEX "DocumentFiling_notificationToken_idx" ON "DocumentFiling"("notificationToken");apps/web/utils/oauth/verify.ts (1)
31-38: Consider removing userId from log context.Per coding guidelines, error logs should avoid revealing internal IDs. Logging
userIdalongsideemailAccountIdcould constitute information disclosure if logs are accessed by unauthorized parties. TheemailAccountIdalone should be sufficient for debugging purposes.🔎 Proposed fix
if (!emailAccount) { logger.warn("Unauthorized OAuth callback - invalid email account", { emailAccountId, - userId: session.user.id, }); redirectUrl.searchParams.set("error", "forbidden"); throw new RedirectError(redirectUrl, responseHeaders); }apps/web/utils/drive/providers/token-helpers.ts (1)
44-49: Consider adding error handling for missing connection.
markDriveConnectionAsDisconnectedwill throw if the connection ID doesn't exist. While callers may handle this, adding a try/catch or usingupdateMany(which won't throw on no match) would make the function more resilient.🔎 Proposed fix (option 1 - silent no-op)
export async function markDriveConnectionAsDisconnected(connectionId: string) { - await prisma.driveConnection.update({ - where: { id: connectionId }, + await prisma.driveConnection.updateMany({ + where: { id: connectionId, isConnected: true }, data: { isConnected: false }, }); }apps/web/app/(app)/[emailAccountId]/drive/ConnectDrive.tsx (3)
20-24: Remove unnecessary Content-Type header for GET requests.Setting
Content-Type: application/jsonon a GET request is unnecessary since GET requests don't have a body. The header should be removed.🔎 Proposed fix
const response = await fetchWithAccount({ url: "/api/google/drive/auth-url", emailAccountId, - init: { headers: { "Content-Type": "application/json" } }, });
50-54: Remove unnecessary Content-Type header for GET requests.Same as the Google Drive handler - the
Content-Typeheader is unnecessary for GET requests.🔎 Proposed fix
const response = await fetchWithAccount({ url: "/api/outlook/drive/auth-url", emailAccountId, - init: { headers: { "Content-Type": "application/json" } }, });
17-75: Consider extracting duplicate OAuth connection logic.The
handleConnectGoogleandhandleConnectMicrosoftfunctions contain nearly identical logic. Consider extracting this into a single reusable function to reduce duplication.🔎 Proposed refactor
+ const handleConnect = async ( + provider: "google" | "microsoft", + url: string, + displayName: string + ) => { + const setLoading = provider === "google" ? setIsConnectingGoogle : setIsConnectingMicrosoft; + setLoading(true); + try { + const response = await fetchWithAccount({ + url, + emailAccountId, + }); + + if (!response.ok) { + throw new Error(`Failed to initiate ${displayName} connection`); + } + + const data: GetDriveAuthUrlResponse = await response.json(); + + if (!data?.url) throw new Error("Invalid auth URL"); + + window.location.href = data.url; + } catch (error) { + captureException(error, { + extra: { context: `${displayName} OAuth initiation` }, + }); + toastError({ + title: `Error initiating ${displayName} connection`, + description: "Please try again or contact support", + }); + setLoading(false); + } + }; + const handleConnectGoogle = async () => { - setIsConnectingGoogle(true); - try { - const response = await fetchWithAccount({ - url: "/api/google/drive/auth-url", - emailAccountId, - }); - // ... rest of the logic - } catch (error) { - // ... error handling - } + await handleConnect("google", "/api/google/drive/auth-url", "Google Drive"); }; const handleConnectMicrosoft = async () => { - setIsConnectingMicrosoft(true); - try { - const response = await fetchWithAccount({ - url: "/api/outlook/drive/auth-url", - emailAccountId, - }); - // ... rest of the logic - } catch (error) { - // ... error handling - } + await handleConnect("microsoft", "/api/outlook/drive/auth-url", "OneDrive"); };apps/web/app/(app)/[emailAccountId]/drive/DriveConnectionCard.tsx (1)
49-54: Consider using a proper modal instead ofconfirm().The native browser
confirm()dialog works but provides a suboptimal user experience. Consider using a shadcn/ui AlertDialog component for better consistency with the rest of the UI.🔎 Example with AlertDialog
// Add import import { AlertDialog, AlertDialogAction, AlertDialogCancel, AlertDialogContent, AlertDialogDescription, AlertDialogFooter, AlertDialogHeader, AlertDialogTitle, AlertDialogTrigger, } from "@/components/ui/alert-dialog"; // Replace DropdownMenuItem with AlertDialog trigger <AlertDialog> <AlertDialogTrigger asChild> <DropdownMenuItem onSelect={(e) => e.preventDefault()} className="text-red-600 focus:text-red-600" > <Trash2 className="mr-2 h-4 w-4" /> Disconnect </DropdownMenuItem> </AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle>Disconnect Drive?</AlertDialogTitle> <AlertDialogDescription> Are you sure you want to disconnect this drive? This action cannot be undone. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction onClick={async () => { await executeDisconnect({ connectionId: connection.id }); mutate(); }} disabled={isDisconnecting}> Disconnect </AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog>apps/web/utils/filebot/is-filebot-email.ts (2)
22-23: Email parsing may fail for malformed inputs with multiple@symbols.
userEmail.split("@")can yield more than two parts if the email contains multiple@(e.g.,"foo@bar@baz.com"). The current destructuring silently takes the first two parts, potentially creating incorrect patterns.Consider using a more robust approach:
Proposed fix
- const [localPart, domain] = userEmail.split("@"); - if (!localPart || !domain) return false; + const atIndex = userEmail.lastIndexOf("@"); + if (atIndex === -1) return false; + const localPart = userEmail.slice(0, atIndex); + const domain = userEmail.slice(atIndex + 1); + if (!localPart || !domain) return false;
44-50: Same email parsing concern applies here.For consistency, apply the same robust parsing approach in
getFilebotEmail.Proposed fix
export function getFilebotEmail({ userEmail }: { userEmail: string }): string { - const [localPart, domain] = userEmail.split("@"); - if (!localPart || !domain) { + const atIndex = userEmail.lastIndexOf("@"); + if (atIndex === -1) { throw new Error("Invalid email format"); } + const localPart = userEmail.slice(0, atIndex); + const domain = userEmail.slice(atIndex + 1); + if (!localPart || !domain) { + throw new Error("Invalid email format"); + } return `${localPart}+${FILEBOT_SUFFIX}@${domain}`; }apps/web/utils/drive/folder-utils.ts (1)
18-32: Case-insensitive folder matching may cause unexpected behavior.The comparison
f.name.toLowerCase() === part.toLowerCase()assumes folder names are case-insensitive. If the drive provider is case-sensitive (some configurations), this could match the wrong folder or fail to find an existing one.Consider documenting this assumption or making it configurable per provider.
apps/web/utils/webhook/process-history-item.ts (1)
234-263: Misleading comment: attachments are processed sequentially, not in parallel.The comment on line 242 says "don't await all - let them run in background" but the code
awaits eachprocessAttachmentcall sequentially within the loop. This is slower than parallel processing.If sequential processing is intentional (e.g., to avoid rate limits), update the comment. If parallel is preferred:
Option A: Update comment for sequential (if intentional)
- // Process each attachment (don't await all - let them run in background) + // Process attachments sequentially to avoid overwhelming external APIs for (const attachment of extractableAttachments) {Option B: Process in parallel (if faster is preferred)
- // Process each attachment (don't await all - let them run in background) - for (const attachment of extractableAttachments) { - await processAttachment({ + // Process all attachments in parallel + await Promise.allSettled( + extractableAttachments.map((attachment) => + processAttachment({ emailAccount: { ...emailAccount, filingEnabled: emailAccount.filingEnabled, filingPrompt: emailAccount.filingPrompt, email: emailAccount.email, }, message: parsedMessage, attachment, emailProvider: provider, logger, - }).catch((error) => { - logger.error("Failed to process attachment", { - filename: attachment.filename, - error, - }); - }); - } + }).catch((error) => { + logger.error("Failed to process attachment", { + filename: attachment.filename, + error, + }); + }), + ), + );apps/web/app/(app)/[emailAccountId]/drive/DriveSetup.tsx (1)
571-582: Manual state sync pattern works but is unconventional.Using a
useRefto track previous server state and conditionally callingsetStateduring render works, but is harder to reason about. AuseEffectwithserverFolderIdsdependency would be more idiomatic React.Alternative with useEffect
- // Sync optimistic state when server data changes - const serverFolderIds = savedFolders.map((f) => f.folderId).join(","); - const prevServerFolderIds = useRef(serverFolderIds); - if (serverFolderIds !== prevServerFolderIds.current) { - prevServerFolderIds.current = serverFolderIds; - setOptimisticFolderIds(new Set(savedFolders.map((f) => f.folderId))); - } + // Sync optimistic state when server data changes + useEffect(() => { + setOptimisticFolderIds(new Set(savedFolders.map((f) => f.folderId))); + }, [savedFolders]);Note: This would require adding
useEffectto the imports.apps/web/utils/drive/types.ts (1)
94-98: Consider makinggetAccessTokenasync if refresh is needed.The docstring states "may trigger refresh if expired," but the return type is synchronous (
string). If token refresh requires async operations, this signature won't support it. The implementations inGoogleDriveProviderandOneDriveProviderreturn the stored token synchronously, which works, but the comment is misleading.Either update the comment to clarify it returns the current token (refresh handled elsewhere), or change the signature to
Promise<string>if refresh logic should be internal.apps/web/utils/drive/client.ts (2)
99-115: Add timeout to Microsoft token exchange fetch.The fetch call to Microsoft's token endpoint has no timeout, which could cause the request to hang indefinitely if Microsoft's servers are slow to respond.
🔎 Proposed fix using AbortSignal
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + const response = await fetch( `https://login.microsoftonline.com/${env.MICROSOFT_TENANT_ID}/oauth2/v2.0/token`, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded", }, body: new URLSearchParams({ client_id: env.MICROSOFT_CLIENT_ID, client_secret: env.MICROSOFT_CLIENT_SECRET, code, redirect_uri: `${env.NEXT_PUBLIC_BASE_URL}/api/outlook/drive/callback`, grant_type: "authorization_code", scope: MICROSOFT_DRIVE_SCOPES.join(" "), }), + signal: controller.signal, }, ); + + clearTimeout(timeoutId);
129-133: Add timeout to Microsoft Graph profile fetch.Same timeout concern applies to the profile fetch call.
apps/web/app/(app)/[emailAccountId]/drive/AllowedFolders.tsx (2)
97-140: Consider per-folder loading state for better UX.Using a single
isFolderBusystate disables all folder checkboxes when any folder is being toggled. For a better user experience, consider tracking busy state per folder ID.🔎 Proposed approach
- const [isFolderBusy, setIsFolderBusy] = useState(false); + const [busyFolderIds, setBusyFolderIds] = useState<Set<string>>(new Set()); const handleFolderToggle = useCallback( async (folder: FolderItem, isChecked: boolean) => { const folderPath = folder.path || folder.name; - setIsFolderBusy(true); + setBusyFolderIds((prev) => new Set(prev).add(folder.id)); try { // ... existing logic } finally { - setIsFolderBusy(false); + setBusyFolderIds((prev) => { + const next = new Set(prev); + next.delete(folder.id); + return next; + }); } }, [emailAccountId, mutateFolders], );Then pass
isDisabled={busyFolderIds.has(folder.id)}to eachFolderNode.
330-333: DefaultdriveConnectionIdis overwritten in submit handler.The form has
defaultValues: { driveConnectionId: "" }but the actual value is set fromdriveConnectionIdprop in the submit handler (line 347). This works but is slightly confusing. Consider either removing it from defaultValues or setting the correct initial value.apps/web/utils/drive/handle-drive-callback.ts (3)
54-63: Validate cached params before building redirect URL.When building the redirect URL from cached results, the code iterates over
cachedResult.paramsand sets them as search params. While you control what's cached (line 128), consider validating themessageparam to ensure it's an expected value.🔎 Proposed fix
if (cachedResult) { logger.info("OAuth code already processed, returning cached result"); const cachedRedirectUrl = new URL("/drive", env.NEXT_PUBLIC_BASE_URL); - for (const [key, value] of Object.entries(cachedResult.params)) { - cachedRedirectUrl.searchParams.set(key, value); - } + // Only set expected params to prevent cache poisoning + const message = cachedResult.params.message; + if (message === "drive_connected" || message === "processing") { + cachedRedirectUrl.searchParams.set("message", message); + }
79-82: Redundant state check after validation.The
receivedStatewas already validated invalidateOAuthCallback(line 201 checks!receivedState). This check is defensive but the error message "Missing validated state" is confusing since validation already passed.Consider removing this check or clarifying the error message.
195-199: Arbitrary code length validation.The check
code.length < 10is arbitrary. OAuth codes vary in length by provider. Consider either removing the length check (null/undefined check is sufficient) or documenting why 10 was chosen.apps/web/utils/drive/filing-engine.ts (1)
199-199: Extract magic number to a named constant.The confidence threshold
0.7is a magic number. Extract it to a named constant for clarity and easier configuration.🔎 Proposed fix
+const FILING_CONFIDENCE_THRESHOLD = 0.7; + // ... in processAttachment function - const shouldAsk = analysis.confidence < 0.7; + const shouldAsk = analysis.confidence < FILING_CONFIDENCE_THRESHOLD;apps/web/utils/drive/providers/microsoft.ts (2)
130-144: Track large file upload TODO.The 4MB upload limit is documented with a TODO for resumable uploads. Consider creating an issue to track this limitation, especially if users may upload larger attachments.
Would you like me to help create an issue to track implementing resumable uploads for files larger than 4MB?
207-214: Remove redundant null coalescing.Line 213 uses
item.id ?? ""but line 208-209 already throws ifitem.idis missing. The fallback is unreachable.🔎 Proposed fix
private convertToFolder(item: DriveItem): DriveFolder { if (!item.id) { throw new Error("Drive item is missing `id`"); } const name = item.name || "Untitled"; return { - id: item.id ?? "", + id: item.id, name, parentId: item.parentReference?.id ?? undefined,apps/web/prisma/schema.prisma (1)
970-992: Consider documenting token encryption strategy.The
accessTokenandrefreshTokenfields store sensitive OAuth credentials. While this follows the existing pattern fromCalendarConnection, consider adding a comment noting whether these are encrypted at rest or if encryption is handled at the application layer.apps/web/utils/actions/drive.validation.ts (1)
18-23: Consider adding non-empty validation to string ID fields.The
filingFolderSchemaand other schemas accept empty strings for ID fields. Consider adding.min(1)to ID fields likefolderId,driveConnectionId, etc., to prevent accidental empty string submissions that could cause database lookup failures.🔎 Example for filingFolderSchema
const filingFolderSchema = z.object({ - folderId: z.string(), - folderName: z.string(), - folderPath: z.string(), - driveConnectionId: z.string(), + folderId: z.string().min(1), + folderName: z.string().min(1), + folderPath: z.string().min(1), + driveConnectionId: z.string().min(1), });apps/web/components/kibo-ui/tree/index.tsx (1)
169-173: Consider addingrole="tree"to TreeView for complete ARIA tree structure.For a fully accessible tree widget, the container should have
role="tree"to complement therole="treeitem"on individual nodes.🔎 Proposed fix
export const TreeView = ({ className, children, ...props }: TreeViewProps) => ( - <div className={cn("p-2", className)} {...props}> + <div className={cn("p-2", className)} role="tree" {...props}> {children} </div> );apps/web/utils/drive/document-extraction.ts (1)
132-135: Type assertion on textContent.items may be fragile.The cast
as Array<{ str?: string }>assumes a specific structure from unpdf's API. If the API changes or returns unexpected shapes, this could silently produce empty strings.Consider adding a type guard or more defensive access:
- const pageText = (textContent.items as Array<{ str?: string }>) - .map((item) => item.str ?? "") - .join(" "); + const pageText = textContent.items + .map((item) => { + if (typeof item === "object" && item !== null && "str" in item) { + return String(item.str ?? ""); + } + return ""; + }) + .join(" ");
| onSuccess: (data) => { | ||
| // Initialize states and trigger filing for each attachment | ||
| const initial: Record<string, FilingState> = {}; | ||
| for (const att of data.attachments) { | ||
| const key = `${att.messageId}-${att.filename}`; | ||
| initial[key] = { status: "filing" }; | ||
|
|
||
| fileAttachmentAction(emailAccountId, { | ||
| messageId: att.messageId, | ||
| filename: att.filename, | ||
| }).then((result) => { | ||
| const resultData = result?.data; | ||
| if (result?.serverError) { | ||
| setFilingStates((prev) => ({ | ||
| ...prev, | ||
| [key]: { status: "error", error: result.serverError }, | ||
| })); | ||
| } else if (resultData?.skipped) { | ||
| setFilingStates((prev) => ({ | ||
| ...prev, | ||
| [key]: { | ||
| status: "skipped", | ||
| skipReason: resultData.skipReason, | ||
| }, | ||
| })); | ||
| } else if (resultData) { | ||
| setFilingStates((prev) => ({ | ||
| ...prev, | ||
| [key]: { status: "filed", result: resultData }, | ||
| })); | ||
| } else { | ||
| setFilingStates((prev) => ({ | ||
| ...prev, | ||
| [key]: { status: "error", error: "Unknown error" }, | ||
| })); | ||
| } | ||
| }); | ||
| } | ||
| setFilingStates(initial); | ||
| }, |
There was a problem hiding this comment.
Race condition: initial state set after async calls are dispatched.
The onSuccess callback sets initial states for each attachment (lines 103-105), then dispatches async fileAttachmentAction calls (lines 107-136), but doesn't set filingStates until line 138. By then, some .then() callbacks may have already fired and attempted to update state that doesn't exist yet.
Move the state initialization before dispatching actions:
Proposed fix
onSuccess: (data) => {
// Initialize states and trigger filing for each attachment
const initial: Record<string, FilingState> = {};
for (const att of data.attachments) {
const key = `${att.messageId}-${att.filename}`;
initial[key] = { status: "filing" };
-
+ }
+ setFilingStates(initial);
+
+ // Now trigger filing for each attachment
+ for (const att of data.attachments) {
+ const key = `${att.messageId}-${att.filename}`;
fileAttachmentAction(emailAccountId, {
messageId: att.messageId,
filename: att.filename,
}).then((result) => {
// ... existing result handling
});
}
- setFilingStates(initial);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onSuccess: (data) => { | |
| // Initialize states and trigger filing for each attachment | |
| const initial: Record<string, FilingState> = {}; | |
| for (const att of data.attachments) { | |
| const key = `${att.messageId}-${att.filename}`; | |
| initial[key] = { status: "filing" }; | |
| fileAttachmentAction(emailAccountId, { | |
| messageId: att.messageId, | |
| filename: att.filename, | |
| }).then((result) => { | |
| const resultData = result?.data; | |
| if (result?.serverError) { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { status: "error", error: result.serverError }, | |
| })); | |
| } else if (resultData?.skipped) { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { | |
| status: "skipped", | |
| skipReason: resultData.skipReason, | |
| }, | |
| })); | |
| } else if (resultData) { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { status: "filed", result: resultData }, | |
| })); | |
| } else { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { status: "error", error: "Unknown error" }, | |
| })); | |
| } | |
| }); | |
| } | |
| setFilingStates(initial); | |
| }, | |
| onSuccess: (data) => { | |
| // Initialize states and trigger filing for each attachment | |
| const initial: Record<string, FilingState> = {}; | |
| for (const att of data.attachments) { | |
| const key = `${att.messageId}-${att.filename}`; | |
| initial[key] = { status: "filing" }; | |
| } | |
| setFilingStates(initial); | |
| // Now trigger filing for each attachment | |
| for (const att of data.attachments) { | |
| const key = `${att.messageId}-${att.filename}`; | |
| fileAttachmentAction(emailAccountId, { | |
| messageId: att.messageId, | |
| filename: att.filename, | |
| }).then((result) => { | |
| const resultData = result?.data; | |
| if (result?.serverError) { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { status: "error", error: result.serverError }, | |
| })); | |
| } else if (resultData?.skipped) { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { | |
| status: "skipped", | |
| skipReason: resultData.skipReason, | |
| }, | |
| })); | |
| } else if (resultData) { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { status: "filed", result: resultData }, | |
| })); | |
| } else { | |
| setFilingStates((prev) => ({ | |
| ...prev, | |
| [key]: { status: "error", error: "Unknown error" }, | |
| })); | |
| } | |
| }); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In apps/web/app/(app)/[emailAccountId]/drive/DriveSetup.tsx around lines 100 to
139, the code builds an initial filing state object but only calls
setFilingStates(initial) after dispatching async fileAttachmentAction calls,
causing a race where .then() handlers can run before the state keys exist; move
the call to setFilingStates(initial) so it runs immediately before starting any
fileAttachmentAction calls (i.e., construct initial, call
setFilingStates(initial), then loop/dispatch each fileAttachmentAction), keep
the per-attachment key as a block-scoped const inside the loop so closures
remain correct, and retain the existing .then() handlers using the functional
updater to merge updates.
| user: { | ||
| select: { | ||
| aiProvider: true, | ||
| aiModel: true, | ||
| aiApiKey: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "aiApiKey" --type ts -g '!*.test.ts' | head -50Repository: elie222/inbox-zero
Length of output: 3982
🏁 Script executed:
# Check the file structure and understand the context better
fd "preview/route.ts" --type fRepository: elie222/inbox-zero
Length of output: 44
🏁 Script executed:
# Get the complete content of the route file to see how credentials are handled
cat -n apps/web/app/api/user/drive/preview/route.ts | head -100Repository: elie222/inbox-zero
Length of output: 3205
🏁 Script executed:
# Check the filing-engine where processAttachment is defined
cat -n apps/web/utils/drive/filing-engine.ts | head -150Repository: elie222/inbox-zero
Length of output: 5603
🏁 Script executed:
# Search for any logging that might expose aiApiKey or credentials
rg "logger\.(info|warn|error|debug)" apps/web/app/api/user/drive/preview/route.ts -A 2Repository: elie222/inbox-zero
Length of output: 1157
🏁 Script executed:
# Check if credentials are passed to API calls or logged anywhere in processAttachment
rg -B 5 -A 5 "processAttachment" apps/web/utils/drive/filing-engine.ts | head -80Repository: elie222/inbox-zero
Length of output: 519
🏁 Script executed:
# Check analyzeDocument to see if credentials are logged there
cat -n apps/web/utils/ai/document-filing/analyze-document.ts | head -100Repository: elie222/inbox-zero
Length of output: 3537
🏁 Script executed:
# Check if credentials are logged anywhere in the error chain
rg "aiApiKey|aiProvider|aiModel" apps/web/utils/ai/document-filing/analyze-document.ts -B 2 -A 2Repository: elie222/inbox-zero
Length of output: 44
🏁 Script executed:
# Check how SafeError is used and if it properly sanitizes error messages
cat -n apps/web/utils/error.ts | head -80Repository: elie222/inbox-zero
Length of output: 2716
🏁 Script executed:
# Check if credentials are ever included in error logs in the preview route error handler
rg "attachmentError|error" apps/web/app/api/user/drive/preview/route.ts -B 2 -A 2Repository: elie222/inbox-zero
Length of output: 538
🏁 Script executed:
# Verify captureException is used safely and doesn't log credentials
rg "captureException" apps/web/app/api/user/drive/preview/route.ts -B 2 -A 2Repository: elie222/inbox-zero
Length of output: 44
🏁 Script executed:
# Check if getModel sanitizes or logs credentials
cat -n apps/web/utils/llms/model.ts | sed -n '49,100p'Repository: elie222/inbox-zero
Length of output: 1937
🏁 Script executed:
# Check if error objects from AI calls could contain credentials
rg "throw|catch" apps/web/utils/ai/document-filing/analyze-document.ts -B 2 -A 2Repository: elie222/inbox-zero
Length of output: 44
🏁 Script executed:
# Verify the logger implementation and what gets logged with error objects
cat -n apps/web/utils/logger.ts | head -80Repository: elie222/inbox-zero
Length of output: 3418
🏁 Script executed:
# Check the hashSensitiveFields function to see what fields it protects
cat -n apps/web/utils/logger.ts | sed -n '80,150p'Repository: elie222/inbox-zero
Length of output: 2514
🏁 Script executed:
# Get the rest of the logger file to see hashSensitiveFields implementation
cat -n apps/web/utils/logger.ts | sed -n '150,220p'Repository: elie222/inbox-zero
Length of output: 2552
🏁 Script executed:
# Get the hashSensitiveFields function implementation
cat -n apps/web/utils/logger.ts | sed -n '220,280p'Repository: elie222/inbox-zero
Length of output: 2434
🏁 Script executed:
# Check if emailAccount (with aiApiKey) is being passed to functions that might log it
rg -n "processAttachment\|analyzeDocument" apps/web/app/api/user/drive/preview/route.ts -B 2 -A 8Repository: elie222/inbox-zero
Length of output: 44
🏁 Script executed:
# Search for processAttachment calls in preview route with context
grep -n "processAttachment" apps/web/app/api/user/drive/preview/route.ts -B 2 -A 8Repository: elie222/inbox-zero
Length of output: 1118
🏁 Script executed:
# Check what's in REDACTED_FIELD_NAMES and if aiApiKey should be there
grep -n "REDACTED_FIELD_NAMES\|aiApiKey" apps/web/utils/logger.tsRepository: elie222/inbox-zero
Length of output: 219
🏁 Script executed:
# Verify if emailAccount with credentials is ever logged in the error path
rg "logger\.(error|warn)" apps/web/utils/drive/filing-engine.ts -B 2 -A 2 | head -40Repository: elie222/inbox-zero
Length of output: 197
Add aiApiKey to REDACTED_FIELD_NAMES in the logger for defensive protection.
The aiApiKey is selected and passed through processAttachment, which is correct for AI operations. However, it's not included in the logger's REDACTED_FIELD_NAMES set alongside other sensitive credentials like accessToken and refreshToken. While current error handling is safe (errors are sanitized to extract only the message), adding aiApiKey to REDACTED_FIELD_NAMES provides defense-in-depth against accidental exposure if logging patterns change or the emailAccount object is logged.
🤖 Prompt for AI Agents
In apps/web/app/api/user/drive/preview/route.ts around lines 63-69, the selected
user fields include aiApiKey but the logger's REDACTED_FIELD_NAMES does not; add
the string 'aiApiKey' to the REDACTED_FIELD_NAMES collection (array/set) where
it is declared in this file (or the shared logger module if imported) so that
aiApiKey is treated the same as accessToken/refreshToken and redacted from logs.
| <span className="ml-2 text-sm text-muted-foreground"> | ||
| {stat.subvalue} | ||
| </span> | ||
| <MutedText className="ml-2">{stat.subvalue}</MutedText> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MutedText component definition
rg "export.*MutedText" --type ts --type tsx -A 5Repository: elie222/inbox-zero
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check the StatsCards.tsx file around line 30
fd "StatsCards.tsx" --type f -x cat -n {} | head -50Repository: elie222/inbox-zero
Length of output: 1537
🏁 Script executed:
#!/bin/bash
# Search for MutedText in Typography component
rg "MutedText" apps/web/components/Typography -A 10 -B 2Repository: elie222/inbox-zero
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Also check if Typography file exists and list its contents
fd "Typography" --type f --path "*/components/*"Repository: elie222/inbox-zero
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Search for Typography component files
find apps/web/components -name "*Typography*" -o -name "*typography*"Repository: elie222/inbox-zero
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for MutedText export across the codebase
rg "MutedText" apps/web/components --type ts -A 5 -B 2Repository: elie222/inbox-zero
Length of output: 15070
🏁 Script executed:
#!/bin/bash
# Get the full MutedText component definition
cat -n apps/web/components/Typography.tsx | grep -A 15 "const MutedText"Repository: elie222/inbox-zero
Length of output: 616
🏁 Script executed:
#!/bin/bash
# Search for StatsCards usage to see if subvalue is always provided
rg "StatsCards" apps/web --type ts -A 20 | grep -A 20 "stats:"Repository: elie222/inbox-zero
Length of output: 1344
🏁 Script executed:
#!/bin/bash
# Also check if there are any instances where subvalue is undefined
rg "subvalue" apps/web --type ts -B 3 -A 3Repository: elie222/inbox-zero
Length of output: 2748
Remove commented code on line 32.
The MutedText component already includes text-sm styling by default (from apps/web/components/Typography.tsx), so no visual regression from the original text-sm class. However, remove the commented-out code as it's unnecessary.
Consider adding a conditional check for the optional subvalue to avoid rendering an empty component:
Suggested improvement
- <MutedText className="ml-2">{stat.subvalue}</MutedText>
+ {stat.subvalue && <MutedText className="ml-2">{stat.subvalue}</MutedText>}
- {/* <p className="text-muted-foreground text-xs">{stat.subvalue}</p> */}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/components/StatsCards.tsx around line 30, remove the commented-out
class text-sm on line 32 (it's redundant) and add a conditional to only render
the <MutedText> for stat.subvalue when stat.subvalue is present/defined (e.g.,
wrap the MutedText in a simple truthy check) so we don't output an empty
component.
| className?: string; | ||
| }) { | ||
| return ( | ||
| <TooltipProvider delayDuration={200}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider moving TooltipProvider to app-level for better performance.
Creating a TooltipProvider for each TruncatedText instance can impact performance when multiple instances are rendered. Per Shadcn best practices, wrap your app or layout with a single TooltipProvider and remove it from individual components.
🔎 Recommended approach
In your app layout or root component:
import { TooltipProvider } from "@/components/ui/tooltip";
export default function RootLayout({ children }) {
return (
<TooltipProvider delayDuration={200}>
{children}
</TooltipProvider>
);
}Then simplify this component:
export function TruncatedText({
text,
className,
}: {
text: string;
className?: string;
}) {
return (
- <TooltipProvider delayDuration={200}>
- <ShadcnTooltip>
- <TooltipTrigger asChild>
- <span className={cn("block truncate", className)}>{text}</span>
- </TooltipTrigger>
- <TooltipContent>
- <p className="max-w-xs whitespace-pre-wrap break-words">{text}</p>
- </TooltipContent>
- </ShadcnTooltip>
- </TooltipProvider>
+ <ShadcnTooltip>
+ <TooltipTrigger asChild>
+ <span className={cn("block truncate", className)}>{text}</span>
+ </TooltipTrigger>
+ <TooltipContent>
+ <p className="max-w-xs whitespace-pre-wrap break-words">{text}</p>
+ </TooltipContent>
+ </ShadcnTooltip>
);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/components/TruncatedText.tsx around line 19, the component
instantiates a TooltipProvider per instance which hurts performance; remove the
TooltipProvider wrapper from this file and keep only
Tooltip/TooltipTrigger/TooltipContent usage so the component assumes a provider
is available, then add a single TooltipProvider import and wrapper to your
app-level RootLayout or top-level layout (wrap children with <TooltipProvider
delayDuration={200}>...</TooltipProvider>) so all TruncatedText instances share
that provider.
| <ShadcnTooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className={cn("block truncate", className)}>{text}</span> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p className="max-w-xs whitespace-pre-wrap break-words">{text}</p> | ||
| </TooltipContent> | ||
| </ShadcnTooltip> |
There was a problem hiding this comment.
Consider mobile/touch device accessibility.
Tooltips rely on hover interactions and don't work reliably on touch devices where there's no hover state. Users on mobile devices may be unable to view the full text when it's truncated.
Consider these mobile-friendly alternatives:
- Use a modal or dialog on mobile instead of a tooltip
- Implement an onClick handler that shows full text in an expandable section
- Use different styling for mobile that doesn't truncate text
- Check viewport size and conditionally render different UI
Example with click-to-expand:
"use client";
import { useState } from "react";
import { useMediaQuery } from "@/hooks/use-media-query";
import {
Tooltip as ShadcnTooltip,
TooltipContent,
TooltipTrigger,
} from "@/components/ui/tooltip";
import { cn } from "@/utils";
export function TruncatedText({
text,
className,
}: {
text: string;
className?: string;
}) {
const [isExpanded, setIsExpanded] = useState(false);
const isMobile = useMediaQuery("(max-width: 768px)");
if (isMobile) {
return (
<span
className={cn("block", !isExpanded && "truncate", className)}
onClick={() => setIsExpanded(!isExpanded)}
>
{text}
</span>
);
}
return (
<ShadcnTooltip>
<TooltipTrigger asChild>
<span className={cn("block truncate", className)}>{text}</span>
</TooltipTrigger>
<TooltipContent>
<p className="max-w-xs whitespace-pre-wrap break-words">{text}</p>
</TooltipContent>
</ShadcnTooltip>
);
}🤖 Prompt for AI Agents
In apps/web/components/TruncatedText.tsx around lines 20 to 27, the current
tooltip-only approach fails on touch/mobile where hover doesn't exist; update
the component to detect mobile/touch (e.g., use a media query hook or
touch-capable detection), add local state to toggle expanded/full-text, and
conditionally render a click-to-expand or modal/dialog UI on mobile while
keeping the tooltip for non-touch devices; ensure the truncated span becomes a
button-like element (or has role/button and keyboard handlers) when clickable,
toggle the truncate class when expanded, and keep Tooltip for desktop to
preserve current behavior and accessibility.
| it("should return empty string for unknown provider", () => { | ||
| expect(getDriveFileUrl("file789", "unknown")).toBe(""); | ||
| }); |
There was a problem hiding this comment.
Fix TypeScript error: invalid provider type in test.
This test passes "unknown" as the provider, which is not a valid DriveProviderType. This will cause a TypeScript compilation error. Additionally, the implementation's default case uses an exhaustive check that returns never, not an empty string.
🔎 Proposed fix
Remove this test case since it tests invalid input that shouldn't be possible with proper typing:
- it("should return empty string for unknown provider", () => {
- expect(getDriveFileUrl("file789", "unknown")).toBe("");
- });
-If you want to test runtime behavior with invalid input, you'll need to cast the type and update the implementation to handle it:
it("should handle invalid provider gracefully", () => {
expect(getDriveFileUrl("file789", "invalid" as any)).toBe("");
});But note: the current implementation's default case is designed to be unreachable (exhaustive check with never type), so you'd need to modify the implementation in url.ts to return an empty string instead.
🤖 Prompt for AI Agents
In apps/web/utils/drive/url.test.ts around lines 17-19 the test passes "unknown"
which is not a valid DriveProviderType and causes a TypeScript compilation
error; remove this invalid test case entirely (delete the it(...) block) so
tests only use valid provider values, or if you intentionally want to test
runtime handling of invalid input, change the test to cast the provider to any
(e.g., "invalid" as any) and then update apps/web/utils/drive/url.ts to handle
that branch (replace the exhaustive never default with a runtime fallback that
returns an empty string).
| export function getDriveFileUrl( | ||
| fileId: string, | ||
| provider: DriveProviderType, | ||
| ): string { | ||
| switch (provider) { | ||
| case "google": | ||
| return `https://drive.google.com/file/d/${fileId}/view`; | ||
| case "microsoft": | ||
| return `https://onedrive.live.com/?id=${fileId}`; | ||
| default: { | ||
| captureException(new Error("Invalid provider"), { extra: { provider } }); | ||
| const exhaustiveCheck: never = provider; | ||
| return exhaustiveCheck; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google Drive file URL format public access link structure
💡 Result:
Common Google Drive public link patterns:
-
Viewer / share page:
- https://drive.google.com/file/d/FILE_ID/view?usp=sharing [1].
- https://drive.google.com/open?id=FILE_ID (older/alternate form). [5]
-
Direct-download / raw serve:
- https://drive.google.com/uc?export=download&id=FILE_ID — forces file download (requires file shared “Anyone with the link”). [1][2]
-
Embed / image view:
- https://drive.google.com/uc?export=view&id=FILE_ID — serves image for embedding (public file). [8]
-
Native Google Docs/Sheets/Slides export (to force specific format):
- Docs: https://docs.google.com/document/d/FILE_ID/export?format=pdf (or format=docx, txt, etc.). [3]
- Sheets: https://docs.google.com/spreadsheets/d/FILE_ID/export?format=xlsx (or pdf). [7]
- Slides: https://docs.google.com/presentation/d/FILE_ID/export/pptx (or pdf). [7]
Notes: the FILE_ID is the long alphanumeric string between /d/ and /view (or in the open?id=... query). File permission must allow “Anyone with the link” for public access. [1][2]
Sources: [1] How-To Geek (drive.uc direct link), [2] Google Sites / Drive direct-link tools, [3] Google Docs export guidance, [7] Audiosyntax (export examples), [8] FunButLearn (uc?export=view).
🌐 Web query:
Microsoft OneDrive file URL format public access link structure
💡 Result:
Summary of common OneDrive public/share URL patterns and how to get direct-download links:
-
Short shared link (redirector): https://1drv.ms/… — a short redirect that expands to a longer share URL. [1]
-
OneDrive Live (personal) share-preview: https://onedrive.live.com/?cid=ACCOUNT_CID&id=FILE_ID[&authkey=AUTH_KEY] — typical preview/landing URL for personal OneDrive shares. You can turn this into a direct-download form: https://onedrive.live.com/download?cid=ACCOUNT_CID&resid=FILE_ID&authkey=AUTH_KEY. [2]
-
SharePoint / OneDrive for Business “new short” format: https://{tenant}.sharepoint.com/:x:/s/{site}/{shareId}?e={token} or https://{tenant}.sharepoint.com/:w:/p/{user}/{shareId}?e={token} — team/tenant share links; appending &download=1 (or using the download endpoint returned by the service) often forces a download. [1][3]
-
Alternate direct-download / API pattern (observed for some flows): a download.aspx endpoint (or temporary tempauth token) appears during the actual download request, e.g. https://my.microsoftpersonalcontent.com/personal/{resid}/_layouts/15/download.aspx?UniqueId=…&tempauth=… — these are often transient URLs generated by OneDrive/SharePoint. [3]
Notes / practical tips:
- Short 1drv.ms links redirect to one of the longer forms above; inspect the expanded URL to construct a download link. [1]
- For personal onedrive.live.com links use the /download?cid=…&resid=…&authkey=… pattern to get a direct file download. [2]
- For SharePoint/OneDrive for Business links, appending &download=1 to the share URL often forces download; if not, use the service’s returned download endpoint or Microsoft Graph to get the file’s direct download URL (microsoft.graph.downloadUrl). [1][3]
Sources: Microsoft documentation on shorter share-link formats; OneDrive direct-download guidance and examples. [1][2][3]
Fix Microsoft OneDrive URL format—missing required cid parameter.
The Google Drive URL format is correct. However, the OneDrive URL is incomplete. According to Microsoft's documentation, the proper format for personal OneDrive share links requires a cid (Customer ID) parameter:
https://onedrive.live.com/?cid=ACCOUNT_CID&id=FILE_ID
The current code omits the cid parameter, which will result in invalid links that won't resolve properly. The fileId alone is insufficient—the Customer ID is required to construct a valid public-facing OneDrive file URL.
🤖 Prompt for AI Agents
In apps/web/utils/drive/url.ts around lines 4–19, the OneDrive URL is missing
the required cid (Customer ID) parameter so produced links are invalid; change
the function to accept a cid (e.g., add a third parameter cid: string |
undefined), and for provider === "microsoft" return the correct format
https://onedrive.live.com/?cid=ACCOUNT_CID&id=FILE_ID using that cid and the
existing fileId; update all call sites to supply the cid (or pass undefined and
guard/error if missing), adjust types/signatures accordingly, and add a unit
test covering the Microsoft case.
|
|
||
| const errorInfo = extractErrorInfo(error); | ||
|
|
||
| logger.error("Failed to create Gmail filter", { | ||
| from, | ||
| addLabelIds, | ||
| removeLabelIds, | ||
| error, | ||
| }); | ||
|
|
||
| // Check if it might be a filter limit issue | ||
| if (errorInfo.status === 500) { | ||
| try { | ||
| const filters = await getFiltersList({ gmail }); | ||
| const filterCount = filters.data?.filter?.length ?? 0; | ||
| if (filterCount >= 990) { | ||
| throw new SafeError( | ||
| `Gmail filter limit reached (${filterCount}/1000 filters). Please delete some existing filters in Gmail settings.`, | ||
| ); | ||
| } | ||
| } catch (limitCheckError) { | ||
| if (limitCheckError instanceof SafeError) throw limitCheckError; | ||
| // If limit check fails, just log and continue with original error | ||
| logger.warn("Failed to check filter count", { error: limitCheckError }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Gmail API filter limit how many filters allowed
💡 Result:
The Gmail API limits a user to 1,000 filters (see the filters.create docs). [1]
Note: forwarding-related limits differ from the general filter limit — for Google Workspace accounts Google documents a limit of 40 account filters that automatically forward messages; other forwarding-related limits (e.g., number of distinct forwarding addresses usable by filters) are documented/observed separately and can be lower. [2]
Sources:
[1] Gmail API — users.settings.filters.create (note: "you can only create a maximum of 1,000 filters").
[2] Google Workspace Admin Help — Gmail sending/forwarding limits (auto‑forward mail filters: 40).
🌐 Web query:
Gmail API filter limit error response status code
💡 Result:
When you exceed the Gmail filters limit (max ~1,000 filters), the Gmail API returns an HTTP 400 Bad Request with an error such as "Too many filters". [1][2]
Sources:
- Gmail API filters.create docs (limit: 1,000 filters). [1]
- Reports/observations of a 400 "Too many filters" error when creating beyond the limit. [2]
🏁 Script executed:
# Check the context around the filter limit check
cd apps/web/utils/gmail
cat -n filter.ts | head -70Repository: elie222/inbox-zero
Length of output: 2484
🏁 Script executed:
# Search for getFiltersList implementation to understand the API call
rg -A 20 "getFiltersList" apps/web/utils/gmail/Repository: elie222/inbox-zero
Length of output: 2073
🏁 Script executed:
# Check for any other references to filter limits in the codebase
rg "990|1000|filter.*limit" apps/web/utils/gmail/Repository: elie222/inbox-zero
Length of output: 1272
Correct the HTTP status code for filter limit error detection.
The Gmail API confirms a 1,000 filter limit and uses a 990 threshold for appropriate buffering. However, the code checks for the wrong HTTP status code: Gmail API returns HTTP 400 Bad Request (not 500) when the filter limit is exceeded. This means the filter limit detection logic on lines 42-56 will never execute.
Change line 42 from:
if (errorInfo.status === 500) {
to:
if (errorInfo.status === 400) {
The rest of the error handling logic is sound: SafeError usage is appropriate, the 990 threshold is reasonable, and the defensive error handling for failed limit checks is well-designed.
🤖 Prompt for AI Agents
In apps/web/utils/gmail/filter.ts around lines 31 to 56 the filter-limit
detection checks the wrong HTTP status code (500) so the logic never runs;
change the conditional to check for status 400 instead (i.e., replace the if
(errorInfo.status === 500) check with if (errorInfo.status === 400)) so the
Gmail filter-limit branch executes when the API returns Bad Request for too many
filters; leave the rest of the limit-check and SafeError handling intact.
| describe("isNotFoundError", () => { | ||
| it("should return true for statusCode 404", () => { | ||
| const error = { statusCode: 404, message: "Not found" }; | ||
| expect(isNotFoundError(error)).toBe(true); | ||
| }); | ||
|
|
||
| it("should return false for other status codes", () => { | ||
| expect(isNotFoundError({ statusCode: 400 })).toBe(false); | ||
| expect(isNotFoundError({ statusCode: 401 })).toBe(false); | ||
| expect(isNotFoundError({ statusCode: 403 })).toBe(false); | ||
| expect(isNotFoundError({ statusCode: 500 })).toBe(false); | ||
| }); | ||
|
|
||
| it("should return false for null", () => { | ||
| expect(isNotFoundError(null)).toBe(false); | ||
| }); | ||
|
|
||
| it("should return false for undefined", () => { | ||
| expect(isNotFoundError(undefined)).toBe(false); | ||
| }); | ||
|
|
||
| it("should return false for non-object", () => { | ||
| expect(isNotFoundError("error")).toBe(false); | ||
| expect(isNotFoundError(404)).toBe(false); | ||
| }); | ||
|
|
||
| it("should return false for object without statusCode", () => { | ||
| expect(isNotFoundError({ message: "Not found" })).toBe(false); | ||
| expect(isNotFoundError({ code: "itemNotFound" })).toBe(false); | ||
| }); | ||
|
|
||
| it("should return false for empty object", () => { | ||
| expect(isNotFoundError({})).toBe(false); | ||
| }); | ||
|
|
||
| it("should handle Error with statusCode property", () => { | ||
| const error = Object.assign(new Error("Not found"), { statusCode: 404 }); | ||
| expect(isNotFoundError(error)).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test case for nested error.code path.
The test suite is comprehensive but missing coverage for the nested error structure that isNotFoundError checks. Looking at errors.ts lines 29-35, the implementation handles Microsoft Graph API responses with nested error objects like { error: { code: "itemNotFound" } }.
🔎 Proposed test case to add
it("should handle Error with statusCode property", () => {
const error = Object.assign(new Error("Not found"), { statusCode: 404 });
expect(isNotFoundError(error)).toBe(true);
});
+
+ it("should return true for nested error.code === 'itemNotFound'", () => {
+ const error = { error: { code: "itemNotFound", message: "Item not found" } };
+ expect(isNotFoundError(error)).toBe(true);
+ });
+
+ it("should return false for nested error.code with different value", () => {
+ const error = { error: { code: "otherError", message: "Other error" } };
+ expect(isNotFoundError(error)).toBe(false);
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("isNotFoundError", () => { | |
| it("should return true for statusCode 404", () => { | |
| const error = { statusCode: 404, message: "Not found" }; | |
| expect(isNotFoundError(error)).toBe(true); | |
| }); | |
| it("should return false for other status codes", () => { | |
| expect(isNotFoundError({ statusCode: 400 })).toBe(false); | |
| expect(isNotFoundError({ statusCode: 401 })).toBe(false); | |
| expect(isNotFoundError({ statusCode: 403 })).toBe(false); | |
| expect(isNotFoundError({ statusCode: 500 })).toBe(false); | |
| }); | |
| it("should return false for null", () => { | |
| expect(isNotFoundError(null)).toBe(false); | |
| }); | |
| it("should return false for undefined", () => { | |
| expect(isNotFoundError(undefined)).toBe(false); | |
| }); | |
| it("should return false for non-object", () => { | |
| expect(isNotFoundError("error")).toBe(false); | |
| expect(isNotFoundError(404)).toBe(false); | |
| }); | |
| it("should return false for object without statusCode", () => { | |
| expect(isNotFoundError({ message: "Not found" })).toBe(false); | |
| expect(isNotFoundError({ code: "itemNotFound" })).toBe(false); | |
| }); | |
| it("should return false for empty object", () => { | |
| expect(isNotFoundError({})).toBe(false); | |
| }); | |
| it("should handle Error with statusCode property", () => { | |
| const error = Object.assign(new Error("Not found"), { statusCode: 404 }); | |
| expect(isNotFoundError(error)).toBe(true); | |
| }); | |
| }); | |
| describe("isNotFoundError", () => { | |
| it("should return true for statusCode 404", () => { | |
| const error = { statusCode: 404, message: "Not found" }; | |
| expect(isNotFoundError(error)).toBe(true); | |
| }); | |
| it("should return false for other status codes", () => { | |
| expect(isNotFoundError({ statusCode: 400 })).toBe(false); | |
| expect(isNotFoundError({ statusCode: 401 })).toBe(false); | |
| expect(isNotFoundError({ statusCode: 403 })).toBe(false); | |
| expect(isNotFoundError({ statusCode: 500 })).toBe(false); | |
| }); | |
| it("should return false for null", () => { | |
| expect(isNotFoundError(null)).toBe(false); | |
| }); | |
| it("should return false for undefined", () => { | |
| expect(isNotFoundError(undefined)).toBe(false); | |
| }); | |
| it("should return false for non-object", () => { | |
| expect(isNotFoundError("error")).toBe(false); | |
| expect(isNotFoundError(404)).toBe(false); | |
| }); | |
| it("should return false for object without statusCode", () => { | |
| expect(isNotFoundError({ message: "Not found" })).toBe(false); | |
| expect(isNotFoundError({ code: "itemNotFound" })).toBe(false); | |
| }); | |
| it("should return false for empty object", () => { | |
| expect(isNotFoundError({})).toBe(false); | |
| }); | |
| it("should handle Error with statusCode property", () => { | |
| const error = Object.assign(new Error("Not found"), { statusCode: 404 }); | |
| expect(isNotFoundError(error)).toBe(true); | |
| }); | |
| it("should return true for nested error.code === 'itemNotFound'", () => { | |
| const error = { error: { code: "itemNotFound", message: "Item not found" } }; | |
| expect(isNotFoundError(error)).toBe(true); | |
| }); | |
| it("should return false for nested error.code with different value", () => { | |
| const error = { error: { code: "otherError", message: "Other error" } }; | |
| expect(isNotFoundError(error)).toBe(false); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/web/utils/outlook/errors.test.ts around lines 4 to 43, add a test that
covers the nested Microsoft Graph error shape the implementation handles: create
an object like { error: { code: "itemNotFound" } } (and optionally a variant
with error.code capitalisation if needed) and assert isNotFoundError(...)
returns true; place it with the other cases (e.g., after the
object-without-statusCode tests) so the suite verifies the nested error.code
path is detected as a 404 equivalent.
| export async function queryMessagesWithAttachments( | ||
| client: OutlookClient, | ||
| options: { | ||
| maxResults?: number; | ||
| pageToken?: string; | ||
| }, | ||
| logger: Logger, | ||
| ): Promise<{ | ||
| messages: ParsedMessage[]; | ||
| nextPageToken?: string; | ||
| }> { | ||
| const MAX_RESULTS = 20; | ||
| const maxResults = Math.min(options.maxResults || MAX_RESULTS, MAX_RESULTS); | ||
|
|
||
| // If pageToken is a URL, fetch directly (per MS docs, don't extract $skiptoken) | ||
| if (options.pageToken?.startsWith("http")) { | ||
| const response: { value: Message[]; "@odata.nextLink"?: string } = | ||
| await withOutlookRetry( | ||
| () => client.getClient().api(options.pageToken!).get(), | ||
| logger, | ||
| ); | ||
|
|
||
| const messages = await convertMessages(response.value, {}); | ||
| return { messages, nextPageToken: response["@odata.nextLink"] }; | ||
| } | ||
|
|
||
| // Build request with hasAttachments filter | ||
| const request = createMessagesRequest(client) | ||
| .top(maxResults) | ||
| .filter("hasAttachments eq true") | ||
| .expand("attachments($select=id,name,contentType,size)") | ||
| .orderby("receivedDateTime DESC"); | ||
|
|
||
| const response: { value: Message[]; "@odata.nextLink"?: string } = | ||
| await withOutlookRetry(() => request.get(), logger); | ||
|
|
||
| const messages = await convertMessages(response.value, {}); | ||
|
|
||
| logger.info("Messages with attachments fetched", { | ||
| messageCount: messages.length, | ||
| hasNextPageToken: !!response["@odata.nextLink"], | ||
| }); | ||
|
|
||
| return { | ||
| messages, | ||
| nextPageToken: response["@odata.nextLink"], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing folderIds lookup causes incomplete label mapping.
Both code paths pass an empty object {} to convertMessages for folderIds (lines 440 and 454). This means getOutlookLabels cannot map parentFolderId to labels like INBOX, SENT, ARCHIVE, etc.
Other functions in this file (e.g., getMessage, getMessages, queryBatchMessages) call getFolderIds(client, logger) before converting messages. This function should do the same for consistency.
🔎 Proposed fix
export async function queryMessagesWithAttachments(
client: OutlookClient,
options: {
maxResults?: number;
pageToken?: string;
},
logger: Logger,
): Promise<{
messages: ParsedMessage[];
nextPageToken?: string;
}> {
const MAX_RESULTS = 20;
const maxResults = Math.min(options.maxResults || MAX_RESULTS, MAX_RESULTS);
+ const folderIds = await getFolderIds(client, logger);
+
// If pageToken is a URL, fetch directly (per MS docs, don't extract $skiptoken)
if (options.pageToken?.startsWith("http")) {
const response: { value: Message[]; "@odata.nextLink"?: string } =
await withOutlookRetry(
() => client.getClient().api(options.pageToken!).get(),
logger,
);
- const messages = await convertMessages(response.value, {});
+ const messages = await convertMessages(response.value, folderIds);
return { messages, nextPageToken: response["@odata.nextLink"] };
}
// Build request with hasAttachments filter
const request = createMessagesRequest(client)
.top(maxResults)
.filter("hasAttachments eq true")
.expand("attachments($select=id,name,contentType,size)")
.orderby("receivedDateTime DESC");
const response: { value: Message[]; "@odata.nextLink"?: string } =
await withOutlookRetry(() => request.get(), logger);
- const messages = await convertMessages(response.value, {});
+ const messages = await convertMessages(response.value, folderIds);🤖 Prompt for AI Agents
In apps/web/utils/outlook/message.ts around lines 418 to 465, both branches call
convertMessages with an empty folderIds object which prevents getOutlookLabels
from mapping parentFolderId to labels; call getFolderIds(client, logger) before
converting messages in both the pageToken URL branch and the normal request
branch and pass the returned folderIds into convertMessages so label mapping
works consistently with the other functions in this file.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 124 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/app/(app)/[emailAccountId]/drive/FilingActivity.tsx">
<violation number="1" location="apps/web/app/(app)/[emailAccountId]/drive/FilingActivity.tsx:83">
P2: Unsafe type assertion: `connection?.provider` can be `undefined` when no matching connection is found, but `as DriveProviderType` forces TypeScript to accept it. This will cause a runtime exception to be captured when the provider is undefined. Add a guard to ensure provider exists before calling `getDriveFileUrl`.</violation>
</file>
<file name="apps/web/app/api/user/drive/filings/route.ts">
<violation number="1" location="apps/web/app/api/user/drive/filings/route.ts:19">
P1: Zod `.default()` only applies to `undefined`, not `null`. Since `searchParams.get()` returns `null` for missing params, the defaults won't be used - `null` gets coerced to `0`, which fails `.min(1)` validation for `limit`. Convert `null` to `undefined` so defaults apply.</violation>
</file>
<file name="apps/web/app/(app)/[emailAccountId]/drive/DriveConnectionCard.tsx">
<violation number="1" location="apps/web/app/(app)/[emailAccountId]/drive/DriveConnectionCard.tsx:51">
P2: Missing error handling for the disconnect action. The result from `executeDisconnect` should be checked and an error displayed to the user if the action fails.</violation>
</file>
<file name="apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx">
<violation number="1" location="apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx:214">
P1: Invalid HTML nesting: `MutedText` renders as a `<p>` element, but it wraps `<div>` children. HTML does not allow `<div>` inside `<p>` - browsers will auto-close the `<p>` before the first `<div>`, breaking the layout.
Consider keeping the original `<div>` or changing the inner elements to `<span>`.</violation>
</file>
<file name="apps/web/components/StatsCards.tsx">
<violation number="1" location="apps/web/components/StatsCards.tsx:30">
P1: `MutedText` renders a `<p>` (block-level element), which will cause the subvalue to appear on a new line instead of inline next to the value. The original `<span>` was inline. Consider using a span with the muted styles, or adding `inline` to the MutedText's className.</violation>
</file>
<file name="apps/web/utils/outlook/message.ts">
<violation number="1" location="apps/web/utils/outlook/message.ts:440">
P2: Passing `{}` for folderIds to `convertMessages` results in empty/incorrect `labelIds` in the parsed messages. Consider fetching folder IDs via `getFolderIds(client, logger)` before calling convertMessages.</violation>
</file>
<file name="apps/web/utils/drive/handle-drive-callback.ts">
<violation number="1" location="apps/web/utils/drive/handle-drive-callback.ts:156">
P2: `RedirectError` already carries a `redirectUrl` with a specific error (e.g., `missing_code`, `invalid_state`). Overwriting with `connection_failed` masks the original reason. Redirect using the provided `redirectUrl` to preserve the specific error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const connection = connections.find((c) => c.id === filing.driveConnectionId); | ||
|
|
||
| const driveUrl = filing.fileId | ||
| ? getDriveFileUrl(filing.fileId, connection?.provider as DriveProviderType) |
There was a problem hiding this comment.
P2: Unsafe type assertion: connection?.provider can be undefined when no matching connection is found, but as DriveProviderType forces TypeScript to accept it. This will cause a runtime exception to be captured when the provider is undefined. Add a guard to ensure provider exists before calling getDriveFileUrl.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/(app)/[emailAccountId]/drive/FilingActivity.tsx, line 83:
<comment>Unsafe type assertion: `connection?.provider` can be `undefined` when no matching connection is found, but `as DriveProviderType` forces TypeScript to accept it. This will cause a runtime exception to be captured when the provider is undefined. Add a guard to ensure provider exists before calling `getDriveFileUrl`.</comment>
<file context>
@@ -0,0 +1,125 @@
+ const connection = connections.find((c) => c.id === filing.driveConnectionId);
+
+ const driveUrl = filing.fileId
+ ? getDriveFileUrl(filing.fileId, connection?.provider as DriveProviderType)
+ : null;
+
</file context>
|
|
||
| const { searchParams } = new URL(request.url); | ||
| const query = querySchema.parse({ | ||
| limit: searchParams.get("limit"), |
There was a problem hiding this comment.
P1: Zod .default() only applies to undefined, not null. Since searchParams.get() returns null for missing params, the defaults won't be used - null gets coerced to 0, which fails .min(1) validation for limit. Convert null to undefined so defaults apply.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/api/user/drive/filings/route.ts, line 19:
<comment>Zod `.default()` only applies to `undefined`, not `null`. Since `searchParams.get()` returns `null` for missing params, the defaults won't be used - `null` gets coerced to `0`, which fails `.min(1)` validation for `limit`. Convert `null` to `undefined` so defaults apply.</comment>
<file context>
@@ -0,0 +1,66 @@
+
+ const { searchParams } = new URL(request.url);
+ const query = querySchema.parse({
+ limit: searchParams.get("limit"),
+ offset: searchParams.get("offset"),
+ });
</file context>
|
|
||
| const handleDisconnect = async () => { | ||
| if (confirm("Are you sure you want to disconnect this drive?")) { | ||
| await executeDisconnect({ connectionId: connection.id }); |
There was a problem hiding this comment.
P2: Missing error handling for the disconnect action. The result from executeDisconnect should be checked and an error displayed to the user if the action fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/(app)/[emailAccountId]/drive/DriveConnectionCard.tsx, line 51:
<comment>Missing error handling for the disconnect action. The result from `executeDisconnect` should be checked and an error displayed to the user if the action fails.</comment>
<file context>
@@ -0,0 +1,93 @@
+
+ const handleDisconnect = async () => {
+ if (confirm("Are you sure you want to disconnect this drive?")) {
+ await executeDisconnect({ connectionId: connection.id });
+ mutate();
+ }
</file context>
| </Badge> | ||
| {fields.length > 0 && ( | ||
| <div className="ml-1 text-sm text-muted-foreground space-y-0.5"> | ||
| <MutedText className="ml-1 space-y-0.5"> |
There was a problem hiding this comment.
P1: Invalid HTML nesting: MutedText renders as a <p> element, but it wraps <div> children. HTML does not allow <div> inside <p> - browsers will auto-close the <p> before the first <div>, breaking the layout.
Consider keeping the original <div> or changing the inner elements to <span>.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx, line 214:
<comment>Invalid HTML nesting: `MutedText` renders as a `<p>` element, but it wraps `<div>` children. HTML does not allow `<div>` inside `<p>` - browsers will auto-close the `<p>` before the first `<div>`, breaking the layout.
Consider keeping the original `<div>` or changing the inner elements to `<span>`.</comment>
<file context>
@@ -211,7 +211,7 @@ function Actions({
</Badge>
{fields.length > 0 && (
- <div className="ml-1 text-sm text-muted-foreground space-y-0.5">
+ <MutedText className="ml-1 space-y-0.5">
{fields.map((field) => (
<div
</file context>
| <span className="ml-2 text-sm text-muted-foreground"> | ||
| {stat.subvalue} | ||
| </span> | ||
| <MutedText className="ml-2">{stat.subvalue}</MutedText> |
There was a problem hiding this comment.
P1: MutedText renders a <p> (block-level element), which will cause the subvalue to appear on a new line instead of inline next to the value. The original <span> was inline. Consider using a span with the muted styles, or adding inline to the MutedText's className.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/StatsCards.tsx, line 30:
<comment>`MutedText` renders a `<p>` (block-level element), which will cause the subvalue to appear on a new line instead of inline next to the value. The original `<span>` was inline. Consider using a span with the muted styles, or adding `inline` to the MutedText's className.</comment>
<file context>
@@ -26,9 +27,7 @@ export function StatsCards(props: {
- <span className="ml-2 text-sm text-muted-foreground">
- {stat.subvalue}
- </span>
+ <MutedText className="ml-2">{stat.subvalue}</MutedText>
</div>
{/* <p className="text-muted-foreground text-xs">{stat.subvalue}</p> */}
</file context>
| logger, | ||
| ); | ||
|
|
||
| const messages = await convertMessages(response.value, {}); |
There was a problem hiding this comment.
P2: Passing {} for folderIds to convertMessages results in empty/incorrect labelIds in the parsed messages. Consider fetching folder IDs via getFolderIds(client, logger) before calling convertMessages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/outlook/message.ts, line 440:
<comment>Passing `{}` for folderIds to `convertMessages` results in empty/incorrect `labelIds` in the parsed messages. Consider fetching folder IDs via `getFolderIds(client, logger)` before calling convertMessages.</comment>
<file context>
@@ -415,6 +415,55 @@ async function convertMessages(
+ logger,
+ );
+
+ const messages = await convertMessages(response.value, {});
+ return { messages, nextPageToken: response["@odata.nextLink"] };
+ }
</file context>
| if (error instanceof RedirectError) { | ||
| return redirectWithError( | ||
| error.redirectUrl, | ||
| "connection_failed", |
There was a problem hiding this comment.
P2: RedirectError already carries a redirectUrl with a specific error (e.g., missing_code, invalid_state). Overwriting with connection_failed masks the original reason. Redirect using the provided redirectUrl to preserve the specific error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/drive/handle-drive-callback.ts, line 156:
<comment>`RedirectError` already carries a `redirectUrl` with a specific error (e.g., `missing_code`, `invalid_state`). Overwriting with `connection_failed` masks the original reason. Redirect using the provided `redirectUrl` to preserve the specific error.</comment>
<file context>
@@ -0,0 +1,286 @@
+ if (error instanceof RedirectError) {
+ return redirectWithError(
+ error.redirectUrl,
+ "connection_failed",
+ error.responseHeaders,
+ );
</file context>
1 participant
User description
Description
This PR improves the error logging for Gmail filter creation failures and adds a diagnostic check for the 1,000 filter limit.
Changes
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR GOOGLE_OAUTH_TOKEN_ENDPOINT_("GOOGLE_OAUTH_TOKEN_ENDPOINT"):::added PRISMA_DB_("PRISMA_DB"):::modified MICROSOFT_OAUTH_TOKEN_ENDPOINT_("MICROSOFT_OAUTH_TOKEN_ENDPOINT"):::added GOOGLE_DRIVE_API_("GOOGLE_DRIVE_API"):::added MICROSOFT_GRAPH_API_("MICROSOFT_GRAPH_API"):::modified REDIS_("REDIS"):::modified GOOGLE_OAUTH_TOKEN_ENDPOINT_ -- "Save Google access and refresh tokens with expiry to DB" --> PRISMA_DB_ MICROSOFT_OAUTH_TOKEN_ENDPOINT_ -- "Persist Microsoft access/refresh tokens and expiry to DriveConnection" --> PRISMA_DB_ PRISMA_DB_ -- "DriveProvider reads stored tokens to authenticate Google Drive uploads" --> GOOGLE_DRIVE_API_ PRISMA_DB_ -- "OneDrive provider uses DB tokens to call Microsoft Graph APIs" --> MICROSOFT_GRAPH_API_ GOOGLE_OAUTH_TOKEN_ENDPOINT_ -- "Cache OAuth code result in Redis to prevent duplicate processing" --> REDIS_ MICROSOFT_OAUTH_TOKEN_ENDPOINT_ -- "Store processed OAuth code in Redis to avoid duplicate exchanges" --> REDIS_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxIntroduces a comprehensive Smart Filing feature, enabling users to automatically organize email attachments into Google Drive or Microsoft OneDrive by connecting drive accounts, defining AI-powered filing rules, and managing filed documents through a new user interface. Enhances Gmail filter management by improving error logging and adding a diagnostic check for the 1,000 filter limit.
@kibo-uito the component registry incomponents.json, enabling the use of new UI components from the Kibo UI library.Modified files (1)
Latest Contributors(2)
Modified files (3)
Latest Contributors(2)
Modified files (3)
Latest Contributors(2)
Modified files (86)
Checks (1)
Latest Contributors(2)
MutedTextcomponent for consistent styling of secondary text and introduces asizeprop forCardcomponents to allow for more compact layouts.Modified files (29)
Latest Contributors(2)
isNotFoundErrorandisAlreadyExistsErrorfunctions, and updatesdraft.tsto use these new utilities for better error detection.Modified files (1)
Latest Contributors(2)
SafeErrorif the limit is reached during a 500 error.Modified files (1)
Latest Contributors(1)