fix(desktop): surface channel history load failures - #7013
Conversation
Prevent failed channel history requests from masquerading as empty channels while keeping cached messages visible and offering a retry. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Move thread typing selection into the existing helper module and compact forwarding so the channel history fix satisfies the file-size ratchet. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
🔐 Codex Security Review
|
|
🤖
This requires manual authorization by a Block organization member for the current head SHA. I did not authorize it because CI approval is outside this run's scope. Options: authorize the existing security-review workflow for this head, or leave the security review pending. My lean is to authorize it if this PR is intended to become merge-ready now. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base 8dbc65d9e2c80d9d8516e17b751c46e0568100e6 → exact head 339f4ba951ac8bbd44280eb30591bf5cd02f5d43.
Request changes
-
[P2] The subscription-settlement refresh can erase a terminal cold-history failure into an authoritative empty result.
useChannelSubscriptioncallsrefreshChannelWindowMessages()after live-subscription setup (desktop/src/features/messages/hooks.ts:426-459). That helper awaitsinvalidateQueries()and then unconditionally callsprojectChannelWindowMessages()(desktop/src/features/messages/lib/projectChannelWindow.ts:26-52). TanStack invalidation does not throw for a failed refetch by default, so projection proceeds andsetQueryData(..., [])changes the failed no-data query fromstatus: errortostatus: successwith empty data. The new UI consequently renders the legitimate-empty surface even though authoritative history failed to load.Required fix: only project an authoritative window after a successful owned refetch, preserving cancellation/channel-switch ownership. Add a regression through the production refresh path proving that cold failure followed by subscription-settlement refresh remains an error, while cached rows survive refresh failure and a later successful retry clears the error.
-
[P2] Retry paints a cold, still-pending history load as genuinely empty.
The terminal error causes the channel to be recorded as settled (
desktop/src/features/channels/ui/ChannelScreen.tsx:618-641;desktop/src/features/messages/lib/timelineLoadingState.ts:49-65). Clicking Retry transitions a no-data TanStack query from error to pending/fetching and clearsisError; the settled latch then suppresses loading. With no rows,timelineSnapshot.ts:203-217selectsemptyfor the duration of the retry. Thus the new Retry path immediately makes the same false claim this PR is intended to prevent.Required fix: do not treat an errored cold load as successfully settled, or explicitly model retry-pending so it renders loading/retained error rather than empty. Add production-wiring regressions for initial error → retry pending → successful empty result, and initial error → retry pending → repeated failure.
The error card itself uses an appropriate native button and alert semantics. The defect is in the orchestration feeding the otherwise-correct body selector; selector-only tests do not exercise either transition.
Validation at exact head
pnpm --dir desktop test: 5,774/5,774 passedpnpm --dir desktop exec tsc --noEmit: passedpnpm --dir desktop build: passed- changed-file Biome check and
git diff --check: passed - worktree was clean and SHA was rechecked after local gates
- CI freshness check was on the same head; remaining in-progress jobs do not account for the state-machine defects above
Native assistive-technology and narrow/light/dark observations remain confidence gaps rather than additional author defects.
Keep failed cold-history queries in their error state and prevent retry transitions from being mistaken for settled empty timelines. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖
Fixed in 7ca3c8c. The owned invalidation now propagates refetch failures, so window projection only runs after a successful authoritative fetch. Regression coverage verifies cold errors remain errors, cached rows survive refresh failure, and a later successful refresh clears the error.
Fixed in 7ca3c8c. Terminal errors no longer settle the channel loading latch, so a cold retry renders loading until it either succeeds or returns to the error surface. QueryObserver regressions cover both successful-empty and repeated-failure retry transitions. Validation: desktop tests passed (5,778/5,778), TypeScript typecheck passed, desktop build passed, changed-file Biome checks passed, and the file-size/diff checks passed. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Re-reviewed base 8dbc65d9e2c80d9d8516e17b751c46e0568100e6 → exact head 7ca3c8cec5ca432c4d6aeb0c8ff882a8170e3945 after the prior runtime defects were addressed.
Request changes
[P2 test/coverage] The Retry regression does not exercise the production wiring that fixed the defect.
The runtime fix in desktop/src/features/channels/ui/ChannelScreen.tsx:636-640 correctly passes !messagesQuery.isError, preventing resolveTimelineLoadingLatch from settling a cold terminal error. However, desktop/src/features/messages/lib/timelineLoadingState.test.mjs:27-45 uses a test-only resolveQueryLoading() helper that independently supplies the same condition; it does not execute or bind the ChannelScreen call site.
A causal mutation removed the production !messagesQuery.isError argument at ChannelScreen.tsx:639, exactly restoring the previously reported false-empty Retry behavior. The full Desktop unit suite still passed 5,778/5,778. The newly added test therefore cannot catch deletion of the production fix. This leaves the specifically requested production-path regression gate unsatisfied at the precise boundary where the bug lived.
Required fix: extract a query-result → loading-latch coordinator that ChannelScreen itself uses and test that production function, or add a component/wiring regression that fails when the production argument is removed.
Both prior runtime P2 defects are otherwise closed:
projectChannelWindow.ts:48-52usesthrowOnError: true, preventing projection after failed invalidation. Removing that option correctly failed the two new refresh regressions.- The production Retry path remains loading after a cold error, and success-empty alone settles the latch.
Exact-head validation
- Desktop unit suite: 5,778/5,778 passed on the unmodified exact head
pnpm --dir desktop exec tsc --noEmit: passed- Biome on the five delta files: passed
git diff --check: passed- Causal control: removing
throwOnError: truefailed the expected refresh tests - Causal control: removing production Retry/error wiring left all 5,778 tests green
- Worktree was restored clean and HEAD reverified; current CI is on this exact head with no observed failures, though several Desktop jobs remain in progress
Native AT and narrow/theme observation remain confidence gaps, not additional author defects.
Exercise the same query-to-loading coordinator used by ChannelScreen so removing the cold-error settlement guard fails the regression. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖
Fixed in cbc41a9. I extracted A causal check removing the coordinator's |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Approved base 8dbc65d9e2c80d9d8516e17b751c46e0568100e6 → exact head cbc41a93944bb9a6fba393aec433bef930f7ce64.
The prior runtime and regression defects are closed:
- Failed invalidation rejects before projection, preserving cold error state and cached rows.
- Cold-error Retry pending remains loading; only a successful authoritative result settles the latch and permits genuine empty.
ChannelScreenand the QueryObserver regressions now use the same productionresolveTimelineQueryLoadingStatecoordinator (ChannelScreen.tsx:615-635;timelineLoadingState.ts:74-100;timelineLoadingState.test.mjs:186-261).- Causal mutation of the shared
!status.isErrorguard now fails both Retry regressions, proving the tests protect the production seam that previously escaped coverage. - Refresh cancellation and stale-completion ownership remain intact.
Exact-head validation:
- Desktop full suite: 5,778/5,778 passed
pnpm --dir desktop exec tsc --noEmit: passed- Biome on all three delta files: passed
- production build and
git diff --check: passed - clean worktree and HEAD reverified after gates
- CI is fresh on this head with no observed failures; remaining Desktop jobs are still in progress
No author action remains. Native assistive-technology announcement and fresh narrow/light/dark captures remain optional verification gaps; this head changes state coordination/tests, not the card layout or semantics.
…-history * origin/main: fix(desktop): surface channel history load failures (#7013) fix(composer): polish automatic mentions (#6956) fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
## Summary A failed initial channel-history request no longer appears as an authoritative empty channel. The timeline now shows an announced error with a Retry action, while cached messages remain visible when a later refresh fails; successful empty channels continue to use their normal intro state. ### Related issue None found. ### Testing - Full desktop unit suite (`pnpm test`) - Desktop TypeScript check (`pnpm exec tsc --noEmit`) - Biome checks for changed files - Repository file-size ratchet - Full pre-push desktop checks and tests - Desktop app launched successfully against local Postgres and Redis for manual testing No screenshot is included because the new UI is only shown after a terminal relay-history failure; the regression test pins the error/empty/list precedence directly. --------- Signed-off-by: Thomas Petersen <thomasp@squareup.com>
…c-agent-commit-identity * origin/main: fix(desktop): surface channel history load failures (#7013) fix(composer): polish automatic mentions (#6956) fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Summary
A failed initial channel-history request no longer appears as an authoritative empty channel. The timeline now shows an announced error with a Retry action, while cached messages remain visible when a later refresh fails; successful empty channels continue to use their normal intro state.
Related issue
None found.
Testing
pnpm test)pnpm exec tsc --noEmit)No screenshot is included because the new UI is only shown after a terminal relay-history failure; the regression test pins the error/empty/list precedence directly.