fix(mail): silence partial refresh warnings with cached rows - #5626
Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
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
AccountErrorsNoticebranch, 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}> |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).

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