Skip to content

fix(mail): silence partial refresh warnings with cached rows - #5626

Merged
steve8708 merged 3 commits into
mainfrom
steve8708/changes-100045
Sep 22, 2026
Merged

steve8708 merged 3 commits into
mainfrom
steve8708/changes-100045

Conversation

@steve8708

Copy link
Copy Markdown
Contributor

Summary

  • keep cached inbox rows usable when a bounded account refresh fails
  • hide the account failure notice when populated rows are already rendered
  • retain the notice for empty states so a failed initial load is not shown as Inbox Zero

Verification

  • mail tests: 115 files, 1052 tests passed
  • mail typecheck passed
  • mail build passed
  • pnpm guards: 77 checks passed

Reproduced on beta: cached Pitch rows rendered with the account failure notice after refresh. Production loaded the same mailbox without the notice during verification.

@github-actions

github-actions Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

@github-actions
github-actions Bot temporarily deployed to pr-5626-mail September 22, 2026 15:44 Destroyed

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 3 potential issues 🟡

Review Details

Code Review Summary

PR #5626 changes EmailList so the account-refresh notice is removed from the top-level populated-list render, while retaining the notice for the normal empty state. The approach addresses the reported cached-row UX problem and the added regression test covers the intended populated-list behavior. The mail app is standard risk: this is UI/state handling for partial provider coverage, not authentication or data mutation.

Key Findings

  • 🟡 MEDIUM — Partial account failures are now completely hidden when cached/current rows are displayed, so an incomplete mailbox can appear complete even though one connected account may be missing or stale.
  • 🟡 MEDIUM — Empty search results return before the remaining AccountErrorsNotice branch, presenting a partial failed search as a trustworthy “no results” state.
  • 🟡 MEDIUM — The empty-first-page pagination/sentinel branch also returns before the notice, leaving users without coverage feedback while more pages are pending.

The second and third cases are structural early-return regressions introduced by moving/removing the wrapper; they should be covered by empty-search and pagination-state tests. The local dev server reached a healthy state, but browser executors could not run visual checks because Chrome navigation/snapshot tools were unavailable.

🧪 Browser testing: Could not verify — the dev server was healthy, but all planner executors lacked Chrome browser tools.

{!!accountErrors?.length && (
<AccountErrorsNotice errors={accountErrors} />
)}
<div className="flex-1 overflow-y-auto" ref={scrollParentRef}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Partial account failures are hidden while cached rows are shown

When a refresh returns rows from some accounts plus accountErrors for another account, the list can contain incomplete or stale mailbox coverage. Removing the notice here makes those rows appear complete with no indication that a connected account failed; keep an unobtrusive partial-coverage indicator for populated cached lists while avoiding the Inbox Zero banner.

Additional Info
Reported by 1 of 2 code-review agents; consistent with templates/mail/AGENTS.md rule that inventory reads must not describe partial coverage as complete.

Fix in Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional for this fix: when rows are present, accountErrors means the background account refresh failed while cached mail is still available. The user-facing issue is this banner appearing above usable mail, so populated cached lists stay quiet. I restored the notice for empty search and pagination states below, where no visible rows exist and the result could otherwise look complete.

Comment thread templates/mail/app/components/email/EmailList.tsx
Comment thread templates/mail/app/components/email/EmailList.tsx
builder-io-integration[bot]

This comment was marked as outdated.

@github-actions
github-actions Bot temporarily deployed to pr-5626-mail September 22, 2026 16:16 Destroyed

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and has a few items to flag 🟡

Review Details

Incremental Code Review Summary

The latest commit addresses the stale-placeholder account-error concern by suppressing accountErrors while useEmails, useLabels, or inbox thread data is placeholder data. It also adds the corresponding source-level regression assertions and preserves the empty-search and empty-pagination notice coverage from the prior update.

Two independent review passes found no new actionable issues in the latest diff. The mail tests reported by the reviewers pass: 115 files and 1,056 tests. The previously reported populated cached-row coverage concern remains unresolved and was intentionally not reposted. The stale-placeholder review comment was verified fixed and resolved.

The local dev server is healthy. This remains standard risk because the change affects mail list state and partial-provider coverage, but not authentication or data mutation.

🧪 Browser testing: Will run after this review (PR touches UI code).

@github-actions
github-actions Bot temporarily deployed to pr-5626-mail September 22, 2026 16:39 Destroyed
@steve8708
steve8708 merged commit 538ccfa into main Sep 22, 2026
48 checks passed
@steve8708
steve8708 deleted the steve8708/changes-100045 branch September 22, 2026 16:50

This branch was successfully deployed

No deployments
pr-5626-mail 6a5a6e0b Deployed Sep 22, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant