Conversation
✅ Deploy Preview for agent-native-fw ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder has reviewed your changes and found 3 potential issues.
Review Details
Code Review Summary
PR #34 adds a "reopen closed tab" feature (Cmd+Shift+T) to the desktop app, intercepts Cmd+W in the main process to close tabs instead of the window, adds the necessary IPC wiring through preload, and includes minor UI polish (color opacity bumps, icon size, layout fix). Risk level: Standard 🟡.
The overall architecture is sound — using a useRef stack for closed tab history, a ref-based active tab ID to avoid stale closures in IPC callbacks, and returning the IPC cleanup function directly from useEffect. However, two real bugs were identified by 2 out of 3 independent review passes:
Key Findings
🔴 HIGH — Keyboard shortcuts fail when a webview is focused (found 2/3 agents)
Both the new Cmd+T/Cmd+Shift+T shortcuts and the existing navigation shortcuts (Cmd+1-9, Cmd+[/]) are registered via window.addEventListener("keydown") in the shell renderer. Electron <webview> elements run out-of-process; once the user clicks into any app tab, focus moves to the webview and these events never reach the host window. The shortcuts are effectively broken in normal use. The author already recognized this limitation for Cmd+W (correctly using before-input-event in the main process), but didn't apply the same approach to the other shortcuts.
🔴 HIGH — Cmd+W interception misses webview-focused input (found 2/3 agents)
before-input-event is attached only to win.webContents (the shell BrowserWindow). When keyboard focus is inside a <webview> guest, input events go to the guest's own WebContents and never fire on the parent. This means Cmd+W falls through to Electron's default close-window behavior in the most common interaction state.
🟡 MEDIUM — Side effect inside setAppTabs updater runs twice in StrictMode (found 2/3 agents)
closedTabsRef.current.push(...) is placed inside the setAppTabs((prev) => ...) functional updater. React 18 StrictMode intentionally double-invokes updaters to surface exactly this kind of bug. Each tab close pushes a duplicate entry, corrupting the reopen stack.
✅ IPC cleanup in preload is correct (same reference used for on/removeListener). ✅ React 18 automatic batching means setActiveSidebarAppId + setAppTabs in handleReopenTab updates atomically. ✅ activeTabIdRef pattern for avoiding stale IPC closures is appropriate.
Code review by Builder.io
There was a problem hiding this comment.
Builder has reviewed your changes and found 3 potential issues.
Review Details
Code Review Summary
PR #34 is a multi-area polish pass: dark mode is refactored from hardcoded class="dark" attributes to a proper ThemeProvider (calendar) and split light/dark CSS variables (mail); the desktop shell gains Cmd+W tab-close and Cmd+Shift+T reopen-closed-tab with webview keyboard forwarding via IPC; Gmail label names are now resolved from the API rather than displayed as raw IDs; and the mail app's GoogleConnectBanner gains a full-page hero onboarding variant. Risk: Standard.
Key Findings
🔴 HIGH:
- Unvalidated URL in
shell.openExternal— any XSS or malicious redirect in a webview guest can callwindow.open("ms-msdt:…")orwindow.open("file:///…"), triggering OS-level protocol handlers. Found by all 4 review passes.
🟡 MEDIUM:
- N+1 uncached Gmail label API calls —
fetchGmailLabelMap()is called per account on every inbox/email request with no caching; materially slower loads and quota burn. Found by 3/4 passes. - Multi-account label ID collision — Gmail label IDs are account-scoped; merging all accounts into one
Mapcan overwrite and mislabel messages. Found by 3/4 passes. listLabelsalways returnsunreadCount: 0— all unread badges disappear as soon as Gmail is connected.listLabelsdeduplication key mismatch — deduplicates by Gmail raw ID but exportsname.toLowerCase(), so same-named labels from two accounts produce duplicate sidebar entries.- Global shortcut
preventDefaultfires for every Cmd/Ctrl key — blocks copy/paste/select-all in the shell renderer. bg-whitebuttons invisible on new light theme — zero contrast on white background in mailGoogleConnectBannerhero.
Found by running 4 code-review agent calls with randomized file ordering.
Code review by Builder.io
| ); | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🟡 Uncached Gmail label fetch on every inbox request — N+1 latency and quota burn
fetchGmailLabelMap() is called for every connected account on every /api/emails and /api/emails/:id request with no caching. Labels change rarely; add a short-lived in-memory cache keyed by account email to avoid an extra round-trip per account on every inbox refresh.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Skipping — this is a premature optimization. We have a small number of accounts and labels change rarely, but the fetch is already parallelized. Can revisit if it becomes a bottleneck.
| } | ||
| const emails = messages.map((m) => gmailToEmailMessage(m)); | ||
| const emails = messages.map((m) => | ||
| gmailToEmailMessage(m, undefined, labelMap), |
There was a problem hiding this comment.
🟡 Multi-account label maps merged by Gmail ID — account-scoped IDs can collide and mislabel messages
Gmail user label IDs (Label_1, Label_2, etc.) are only unique per account. Merging all accounts into one Map<string,string> means the second account overwrites the first when IDs collide. Scope label maps per account and look up each message using its own account's map.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Acknowledged but low risk in practice — we're targeting 1-3 accounts max right now, and Gmail's user label IDs colliding across accounts would just show a wrong label display name. Not worth the added complexity yet.
There was a problem hiding this comment.
Builder has reviewed your changes and found 3 potential issues.
Review Details
Code Review Summary (Update 2)
This PR update adds significant new features: an HtmlEmailBody component for rendering HTML emails in an iframe, a Google profile photo fetcher in getAuthStatus, an AccountPopover UI for multi-account management, trackEvent analytics hooks, expanded keyboard shortcuts, and additional AGENTS.md de-branding. Risk: Standard.
Previously reported issues (shell.openExternal, N+1 Gmail label fetches, multi-account label collision) remain unfixed on the PR.
New Findings
🔴 HIGH:
HtmlEmailBodyiframesandbox="allow-same-origin"with unsanitized HTML — allows malicious email HTML to issue credentialed same-origin requests (found by 4/4 review passes)
🟡 MEDIUM:
getAuthStatusN+1 sequential People API calls — blocks status endpoint with one external API call per account, called on every mount/focus and every 2s during OAuth polling (found by 4/4 passes)window.opencalled asynchronously inuseEffect— async popup invocation gets blocked by modern browser popup blockers; also continues polling if user abandons OAuth flow (found by 2/4 passes)- HTML email auto-loads remote images — tracking pixels and remote resources load automatically on open, leaking user IP/agent to senders (found by 1/4 passes, clearly valid regression)
Found by running 4 code-review agent calls with randomized file ordering.
Code review by Builder.io
| style={{ | ||
| width: "100%", | ||
| height: `${height}px`, | ||
| border: "none", |
There was a problem hiding this comment.
🔴 HtmlEmailBody sandbox="allow-same-origin" with unsanitized email HTML
allow-same-origin is unnecessary here — doc.write(), doc.close(), and scrollHeight reads are all performed by parent-frame JS and work regardless of this flag. With the flag, the iframe inherits the app's real origin, so CSS in a malicious email (e.g. @import url(...), background: url(...)) can fire credentialed GET requests to same-origin API endpoints. Remove allow-same-origin; change to sandbox="". If clickable links are needed, add only allow-popups.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Disagree with removing allow-same-origin — it's required here. The code accesses iframeRef.current.contentDocument to call doc.open(), doc.write(), and read scrollHeight for auto-resizing. Without allow-same-origin, contentDocument becomes null due to cross-origin restrictions and the entire email renderer breaks. Added allow-popups for clickable links in ca75082.
| const tokens = readJsonFile<GoogleTokens>(file); | ||
| if (!tokens) continue; | ||
| const email = path.basename(file, ".json"); | ||
| let photoUrl: string | undefined; |
There was a problem hiding this comment.
🟡 getAuthStatus makes a sequential Google People API call per account on every status check
getAuthStatus now awaits people.people.get() for each account inside a for loop, so latency scales as N × RTT. This endpoint is hit on every mount/window-focus (no staleTime in useGoogleAuthStatus) and polled every 2 seconds by AccountPopover during OAuth. Fix: cache the photoUrl to disk when the account is first connected, or at minimum parallelize with Promise.all.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Skipping — the sequential call is fine for now with 1-3 accounts. We can cache the photo URL to disk on first connect if this becomes noticeable.
| const disconnectGoogle = useDisconnectGoogle(); | ||
|
|
||
| useEffect(() => { | ||
| if (authUrl.data?.url) { |
There was a problem hiding this comment.
🟡 window.open called asynchronously in useEffect — blocked by browser popup blockers
window.open(authUrl.data.url, "_blank") fires inside a useEffect callback, not in a direct click handler, so browsers treat it as an unsolicited popup and block it. Additionally, if the user abandons the flow without completing it, the setInterval continues polling /api/google/status indefinitely. Move the window.open call to an onClick handler and open the window synchronously before the fetch.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Skipping — this runs inside an Electron webview, not a regular browser tab. Popup blockers don't apply in this context.
There was a problem hiding this comment.
Builder has reviewed your changes and found 6 potential issues.
Review Details
Code Review Summary — PR #34
This PR is a significant update spanning 176 files (+1,623 / -5,975 lines). The core changes are: (1) a multi-account Gmail management system with AccountPopover, photo avatars via Google People API, and new keyboard shortcuts; (2) HTML email rendering via a sandboxed iframe (HtmlEmailBody); (3) Electron keyboard shortcut overhaul (webview IPC forwarding, Cmd+W tab close); (4) analytics template generalization replacing hardcoded org-specific values with env-var-driven config; and (5) broad branding/naming cleanup across templates.
Risk: 🔴 High — the PR touches auth token handling, Electron shell security, email HTML rendering, and admin access gating logic.
Positive changes: The shell.openExternal RCE from the previous review is properly fixed with a protocol allowlist. The webview keyboard forwarding architecture (main → preload → renderer IPC) is sound. The HtmlEmailBody iframe approach for HTML email is reasonable isolation.
Key Findings
🟡 MEDIUM — Keyboard shortcuts (3/5 agents)
Unconditional e.preventDefault() in the new shell keydown handler suppresses all Cmd/Ctrl+ combinations (Cmd+R, Cmd+F, future clipboard ops) when the shell has focus — not just the handled subset.
🟡 MEDIUM — getEmail latency regression (2/5 agents)
fetchGmailLabelMap is called before messages.get in the getEmail route, adding a sequential Gmail API round-trip to every email open. In multi-account setups, label fetches also fire for accounts that don't own the email, creating an N×accounts waste.
🟡 MEDIUM — OAuth polling bugs (2/5 agents)
AccountPopover polling only terminates when accounts.length increases, meaning it runs indefinitely on OAuth cancel or existing-account re-auth. Compounded by setInterval with an async callback that can stack overlapping requests.
🟡 MEDIUM — Reply-all sends only to sender (1/5 agents)
The new A shortcut is labeled reply-all but calls the same handleReply() as R, which only pre-fills the sender's address.
🟡 MEDIUM — Multi-account read/unread sends to wrong account (1/5 agents)
New u/Shift+I/Shift+U shortcuts call markRead.mutate({ id, isRead }) without accountEmail, so Gmail modify requests always route to the first connected account.
🟡 MEDIUM — isAdmin silently disabled when env var unset (1/5 agents)
adminDomain && email.endsWith(...) evaluates to "" (falsy string) when ADMIN_EMAIL_DOMAIN is not set, silently blocking all domain-based admin access with no error.
Found via 5 parallel code-review agent calls with randomized file ordering.
Code review by Builder.io
|
|
||
| // Keyboard shortcuts — shell has focus | ||
| useEffect(() => { | ||
| const handler = (e: KeyboardEvent) => { |
There was a problem hiding this comment.
🟡 Unconditional e.preventDefault() swallows all Cmd/Ctrl shortcuts in shell
The new keydown handler calls e.preventDefault() before checking whether the key is handled:
const handler = (e: KeyboardEvent) => {
if (!e.metaKey && !e.ctrlKey) return;
e.preventDefault(); // ← fires for EVERY Cmd+ key
handleShortcut(e.key, e.shiftKey);
};handleShortcut only handles T, 1–9, [, ] — so Cmd+R (reload in dev), Cmd+F, and any future shell text inputs will silently swallow their defaults. Fix: return a boolean from handleShortcut and only call preventDefault() when it returns true.
How did I do? React with 👍 or 👎 to help me improve.
| for (const { email, client } of clients) { | ||
| try { | ||
| const gmail = google.gmail({ version: "v1", auth: client }); | ||
| const labelMap = await fetchGmailLabelMap(client); |
There was a problem hiding this comment.
🟡 fetchGmailLabelMap called before messages.get in getEmail — sequential latency + per-account waste
fetchGmailLabelMap(client) is awaited before gmail.users.messages.get, adding a full Gmail API round-trip to every email open. For multi-account users it's worse: the label fetch fires for every account in the loop before finding the one that owns the message — so an account that 404s still paid the label-list cost. Fix: move fetchGmailLabelMap after the successful messages.get, or parallelize with Promise.all.
How did I do? React with 👍 or 👎 to help me improve.
| if (res?.ok) { | ||
| const data = await res.json(); | ||
| if (data.accounts?.length > accounts.length) { | ||
| clearInterval(interval); |
There was a problem hiding this comment.
🟡 AccountPopover polling never stops on OAuth cancel or re-auth of existing account
The interval only clears when data.accounts.length > accounts.length. If the user cancels the OAuth popup or re-authenticates an already-connected account, the count never increases and the interval runs until the popover unmounts. setInterval also fires every 2 s regardless of whether the previous fetch resolved, stacking overlapping requests when the server is slow. Fix: stop polling on a timeout, use setTimeout recursively to prevent overlap, or listen for a postMessage from the callback page.
How did I do? React with 👍 or 👎 to help me improve.
| }, [email]); | ||
|
|
||
| // Keyboard shortcuts — Gmail / Superhuman standard (active when thread is open) | ||
| useKeyboardShortcuts( |
There was a problem hiding this comment.
🟡 "A" reply-all shortcut only replies to sender — drops all other recipients
{ key: "a", handler: () => handleReply() } calls the same path as plain R. handleReply pre-fills only to: replyTo.from.email and never populates cc or the original recipients. Users who expect standard Gmail A (reply-all) semantics will silently send to the sender only. Fix: implement a dedicated reply-all path that includes the original to/cc recipients (minus the current account), or remove the A binding until reply-all is supported.
How did I do? React with 👍 or 👎 to help me improve.
| const toggleFocusedRead = useCallback(() => { | ||
| if (!focusedId) return; | ||
| const email = emails.find((e) => e.id === focusedId); | ||
| if (!email) return; |
There was a problem hiding this comment.
🟡 Read/unread shortcuts omit accountEmail — always modify first connected account
The new u, Shift+I, and Shift+U handlers call markRead.mutate({ id: focusedId, isRead }) without accountEmail. The server's getClient(undefined) falls back to the first token file, so for any message from a secondary account the modify request goes to the wrong mailbox — either 404ing or updating a different message. The email object already carries accountEmail; it needs to be forwarded through the mutation.
How did I do? React with 👍 or 👎 to help me improve.
|
|
||
| // Check if user is admin (builder.io email) or in analytics team (DATA_DICT_REVIEWERS) | ||
| const isAdmin = userInfo.email.toLowerCase().endsWith("@builder.io"); | ||
| // Check if user is admin (matches ADMIN_EMAIL_DOMAIN) or in analytics team (DATA_DICT_REVIEWERS) |
There was a problem hiding this comment.
🟡 isAdmin evaluates to empty string when ADMIN_EMAIL_DOMAIN is unset — silently blocks all admin access
adminDomain && email.endsWith(...) short-circuits to "" (a falsy string, not false) when ADMIN_EMAIL_DOMAIN is not set. All if (isAdmin) guards pass, but domain-based admin access is silently disabled with no warning. Deployers who omit the env var get 403s with no indication of why. Fix: use a ternary — adminDomain ? email.endsWith(...) : false — and apply the same fix to the identical pattern in handleSaveInstruction.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Builder has reviewed your changes and found 3 potential issues.
Review Details
PR #34 Code Review
This PR is a broad template generification effort — replacing ~6,000 lines of Builder.io-specific proprietary content (project IDs, API keys, customer event names, persona/messaging/competitor skill files) with generic template placeholders. It also introduces a desktop-app "restore closed tabs" (Cmd+Shift+T) feature with a React StrictMode workaround, adds Google profile photos to the mail template's auth status, and makes various analytics/mail template improvements.
Risk Assessment: 🟡 Standard — business logic changes (tab management, analytics queries, multi-account email routing), API route modifications, and state management refactoring.
Review ran 5 parallel agents with randomized file ordering to combat position bias.
Key Findings
🔴 HIGH — Blanket e.preventDefault() breaks all Cmd/Ctrl shortcuts (found by 3 of 5 agents): The refactored keyboard handler calls preventDefault() unconditionally for every metaKey/ctrlKey event before delegating to handleShortcut, which only handles 5 keys. Cmd+C, Cmd+V, Cmd+A, etc. are silently swallowed when the desktop shell has focus — a clear functional regression.
🔴 HIGH — handleTabClose stale closure corrupts undo stack (found by 2 of 5 agents): The StrictMode fix correctly moves the push outside the updater, but reads appTabs from a stale closure. Rapid closes can push the same tab twice, causing duplicate React keys on restore and crashing the renderer.
🔴 HIGH — your-gcp-project-id not substituted in server SQL writes: The server's resolveTablePlaceholder() only rewrites @app_events. Hardcoded your-gcp-project-id in gamification writes causes validation persistence to fail silently for any real installation.
🟡 MEDIUM — getAuthStatus makes N sequential Google People API calls (found by 3 of 5 agents): Previously a fast file-read, it now awaits a live API call per account in a for...of loop. Called every 2 seconds during account setup polling — significant latency regression.
🟡 MEDIUM — Gmail labels always return unreadCount: 0: The refactored listLabels() builds labels from fetchGmailLabelMap() but hardcodes unreadCount: 0, breaking nav unread badges for all connected Gmail users.
🟡 MEDIUM — Raw HTML email body auto-loads remote tracking pixels: HtmlEmailBody writes bodyHtml directly into an iframe without blocking external resources, silently firing tracking pixels when emails are opened.
Found by running 5 code-review agents with randomized file ordering.
Code review by Builder.io
| const appState = appTabs[activeSidebarAppId]; | ||
| const closedTab = appState?.tabs.find((t) => t.id === tabId); | ||
| if (closedTab) { | ||
| closedTabsRef.current.push({ |
There was a problem hiding this comment.
🔴 handleTabClose reads stale appTabs closure — corrupts closed-tab undo stack
The push to closedTabsRef reads appTabs from the callback closure, but handleTabClose is recreated on every tab change and the IPC listener re-registers after the next paint. During that window, the stale handler can fail to find a newly opened tab, silently omitting it from the undo stack. Use a ref (appTabsRef.current = appTabs) mirroring the existing activeTabIdRef pattern so the push always reads current state.
How did I do? React with 👍 or 👎 to help me improve.
| useEffect(() => { | ||
| const handler = (e: KeyboardEvent) => { | ||
| if (!e.metaKey && !e.ctrlKey) return; | ||
| e.preventDefault(); |
There was a problem hiding this comment.
🔴 Blanket e.preventDefault() suppresses all Cmd/Ctrl shortcuts
The handler calls e.preventDefault() unconditionally before delegating to handleShortcut, which only handles t, [, ], and 1-9. Cmd+C, Cmd+V, Cmd+A, Cmd+Z, etc. are silently swallowed when the shell has focus. Move e.preventDefault() inside each handled branch of handleShortcut (or have it return a boolean) so only keys the app actually consumes are prevented.
How did I do? React with 👍 or 👎 to help me improve.
| async function persistValidationToBigQuery(validation: any): Promise<void> { | ||
| const sql = ` | ||
| INSERT INTO \`builder-3b0a2.logs.metric_validations\` | ||
| INSERT INTO \`your-gcp-project-id.logs.metric_validations\` |
There was a problem hiding this comment.
🔴 Validation writes hardcoded to placeholder BigQuery project
The INSERT targets `your-gcp-project-id.logs.metric_validations` literally. The server's runQuery() only substitutes @app_events, so all validation writes fail silently for any installation with BIGQUERY_PROJECT_ID configured. Build the table name from process.env.BIGQUERY_PROJECT_ID instead.
How did I do? React with 👍 or 👎 to help me improve.
1 participant
No description provided.