perf(desktop): share quick-reaction preparation per app session - #7199
wesbillman wants to merge 3 commits into
Conversation
🔐 Codex Security Review
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES on exact head 4826fd35f20bcd9cdffbd4c2f0df98be09227d68 against base e17a0d44c379b2ae40b91b8ba43f9a9c5bede90f.
P2 — the per-row observer regression is not causally protected
desktop/src/features/messages/ui/QuickReactionProvider.test.mjs:113-147,207-240 mounts test-only Consumer nodes beneath the provider. This verifies that the provider owns one query observer and storage listener, but it never mounts MessageActionBar, the production row seam from which the observer was removed (desktop/src/features/messages/ui/MessageActionBar.tsx:416-419 in this head's source numbering).
A compiling mutation restored a per-row useCustomEmojiQuery() call in MessageActionBar while retaining the provider. The full Desktop suite still passed 5,888/5,888. That mutation recreates the optimization's core regression—one query observer per mounted action bar—without failing any new test. The current deterministic 1→64 claim therefore applies only to synthetic context consumers. This conflicts with the repository's explicit falsifiability/production-seam requirement in TESTING.md:25-31 and AGENTS.md:188-192.
Author action: add a production-bound regression assertion that mounts multiple real MessageActionBars (or observes the E2E query cache after multiple real action bars mount) and requires the customEmojiQueryKey observer count to remain exactly one. Demonstrate that restoring the per-row observer fails the test, then restore the production code and prove the full Desktop suite green. Bind the storage-listener count to production action bars too if that sharing claim remains part of the protected contract.
Verification owner: review will rerun the mutation and full Desktop suite on the next immutable head.
Integrated review
No implementation or product/accessibility defect was found beyond this test defect:
- The provider sits below
CommunityQueryProviderand inside keyedAppReady; community, reinitialization, pubkey, and signer-epoch changes destroy its observer/listener. The separate Huddle React root receives one provider in its own webview. - Cleanup is symmetric, the snapshot changes identity only when rendered emoji/URL content changes, same-window ranking remains frozen, matching external storage events refresh ranking, unavailable custom emoji backfill defaults, and custom URL updates propagate.
- Existing quick-button markup, labels, styling, and keyboard/pointer/focus handlers are unchanged. The no-intended-visual-change claim is credible.
- A separate mutation removing the production provider wrapper was killed by
quick-reaction-sharing.spec.ts, so actual-app wiring is protected; it does not catch reintroducing work inside each row.
Exact-head evidence: clean full Desktop suite 5,888 passed, pnpm typecheck passed, git diff --check clean, and focused quick-reaction E2E passed. CI was still running Desktop Core/smoke/integration jobs at the final freshness check; completed applicable jobs were green. Missing native WKWebView timing/scroll measurements and a native Huddle companion-window run remain explicitly scoped confidence gaps, not author defects.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated review — request changes at head 4826fd35f20bcd9cdffbd4c2f0df98be09227d68 (base e17a0d44c379b2ae40b91b8ba43f9a9c5bede90f).
IMPORTANT — the per-row sharing contract is not bound to the production seam
The optimization's core guarantee — one custom-emoji query observer and one storage listener regardless of mounted action bars — is only asserted against synthetic consumers. QuickReactionProvider.test.mjs mounts a test-only Consumer under the provider for its getObserversCount() assertions (lines 233, 245, 344-355), and quick-reaction-sharing.spec.ts:113-119 checks rendered tray contents across real action bars but never asserts observer or listener counts. Neither new test file references MessageActionBar, the seam this PR removed the per-row useCustomEmojiQuery() call from (MessageActionBar.tsx:419).
Consequence: reintroducing a per-row observer in MessageActionBar compiles and leaves every new test green — the O(mounted rows) preparation this PR removes can silently return. That violates the repo's explicit rule that regression tests must bind the production seam and be falsifiable (TESTING.md:25-31, AGENTS.md Review-Proven Rules #3).
Fix: assert the invariant across multiple mounted real MessageActionBar instances — either a component test mounting several action bars under the provider, or extend the existing E2E via __BUZZ_E2E_QUERY_CLIENT__ to require exactly one customEmojiQueryKey observer (and one storage listener, if that remains part of the protected claim) after multiple trays are mounted. Demonstrate the test fails when a per-row observer is restored.
Three independent assessments converged on this as the sole blocker, including the existing exact-head review with mutation evidence (a restored per-row observer survived the full 5,888-test Desktop suite).
Otherwise clear
No source-level defect found in the implementation itself: the provider sits inside both the community-keyed query boundary and keyed AppReady (App.tsx:643-648), so community/identity/signer-epoch changes tear down the single observer and listener; community-scoped persistence keys, frozen same-window ranking, matching-key external storage refresh, unavailable-shortcode backfill, and URL-only palette propagation are all preserved; snapshot identity changes only with rendered content; the Huddle root inherits its own provider instance. Removing the old module-global session cache rather than layering another cache is the right shape.
CI at review time
Desktop Smoke E2E (2) is red at this head: the hard failure is empty-edit-delete.spec.ts:74 (delete-confirmation alertdialog never appeared; 3/3 attempts), with empty-edit-delete.spec.ts:51 and a file-attachment case flaky-passed. The failing assertion is in the composer-owned delete-confirmation flow this diff does not modify, and the flow passes through the PR-touched action-bar menu successfully before failing — evidence points away from this change, but the shard is required and Desktop Core was still pending, so a green run at this head (or a rerun proving flake) is still needed alongside the fix above.
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Addressed the shared production-seam finding from both reviews in The added case mounts real Mutation evidence on this immutable head:
Validation at clean The earlier review-reported delete-dialog smoke failure is not changed or claimed fixed by this commit. CI and reviewer verification remain external gates. @jedwards27 @wpfleger96 Ready for the bounded re-review/mutation verification requested in your reviews. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE at exact head 827813f2ace0b2b16aaf452db8a220cc4d97e0fc (base e17a0d44c379b2ae40b91b8ba43f9a9c5bede90f).
The changed-head fix closes the prior causal-coverage defect. The new regression mounts real MessageActionBar rows under the real query/provider/tooltip infrastructure and proves the shared preparation remains bounded through 1→16→16→1→16 row transitions.
Mutation evidence:
- Reintroducing a per-row
useCustomEmojiQuery()observer fails the focused test with17 !== 1. - Reintroducing a per-row
storagelistener fails it with17 !== 1. - Restoring production source returns the focused suite to green, with a clean tree at the reviewed SHA.
The product/UI lane found no behavior or accessibility regression: real quick buttons and custom emoji render through the production action bar, provider wiring/remount behavior remains correct, and the unchanged callback path was exercised through focused browser journeys. Keeping the picker closed is appropriate because it owns an independent legitimate observer outside the quick-tray sharing invariant.
Exact-head evidence: Desktop package suite 5,889/5,889 passed; typecheck, check, build, focused provider tests, and selected browser journeys passed. Live head still matches the reviewed SHA. At submission, all completed applicable CI checks were green; Desktop Smoke E2E (4) remained in progress.
Residual confidence gaps, not author defects: no native WKWebView timing/scroll measurement, no native Huddle companion-window run, and the reviewer’s capped just ci invocation did not complete although no failure was observed. The exact affected package suite and mutation evidence are complete.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE on exact head 827813f2ace0b2b16aaf452db8a220cc4d97e0fc against base e17a0d44c379b2ae40b91b8ba43f9a9c5bede90f.
The prior production-seam test defect is resolved. The changed-head delta is confined to QuickReactionProvider.test.mjs; it now mounts real MessageActionBar instances under the real query/provider/tooltip tree and asserts actual three-button trays, custom emoji image resolution, one palette observer, one storage listener, stable palette-read count across 1 → 16 → 16 → 1 → 16 rows, and zero resources after unmount.
Independent mutation verification killed both targeted regressions:
- restoring per-row
useCustomEmojiQuery()failsreal rows must not add palette observerswith17 !== 1; - restoring a per-row storage effect fails
real rows must not add storage listenerswith17 !== 1.
Both mutations were restored and the exact-head tree was clean. Query fetching is disabled only in the test client's defaults; the production hook and React Query observer are real, and the seeded cache plus palette getter assertions exercise preparation. Keeping the picker closed is correct because its independently mounted observer is outside the quick-tray sharing contract.
No product/UI/accessibility or implementation defect was found. The production handler is unchanged, and exact-head browser validation exercised quick-button callbacks/persistence, custom image propagation, channel remounts, storage refresh, inbox reactions, and picker ordering.
Validation at matching HEAD:
- full Desktop suite: 5,889/5,889 passed;
pnpm typecheck,pnpm check, andpnpm build: passed;- production-bound component suite: 8/8 passed;
- focused smoke E2E: 7/7 passed in the UI lane and 1/1 passed in the systems lane;
- completed CI checks are green. A rerun of the previously unrelated Desktop Smoke E2E shard remained in progress at final freshness check, so CI owns that confidence gap rather than the author.
No native WKWebView timing/scroll measurement or native Huddle companion-window run was obtained; those remain explicitly scoped confidence gaps, not defects.
Author action: none.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated re-review — clear at head 827813f2ace0b2b16aaf452db8a220cc4d97e0fc (base e17a0d44c379b2ae40b91b8ba43f9a9c5bede90f).
Prior blocker resolved
The previous round's IMPORTANT finding (review 5081711599 — sharing guarantee asserted only against synthetic consumers) is closed by the delta commit 827813f2 (test(desktop): bind quick-tray sharing to real action bars, QuickReactionProvider.test.mjs only, +99/−7):
- The new case mounts the real production
MessageActionBarunder the realQueryClientProvider → QuickReactionProvider → TooltipProvidertree and proves the production component rendered via itsdata-testid, three quick buttons, and the seeded custom-emoji image — the exact seam the per-row observer was removed from. - The guard is falsifiable at both seams: with 16 mounted real bars it requires exactly one
customEmojiQueryKeyobserver and exactly one storage listener on the same query client — a reintroduced per-rowuseCustomEmojiQuery()or storage effect fails at 17 vs 1. Instrumented shortcode getters additionally require zero further palette reads across row growth, rerender, and remount, so restored per-row preparation is caught causally, not incidentally. Teardown must return both counts to zero. - No weakening and no smuggled production change: the seven prior provider cases are intact (the only structural change on their path is a transparent
Fragmentwrapper), and the two-commit branch still contains no other production edit.
Two independent review lanes converged on this assessment.
CI
All checks green at this head, including Desktop Smoke E2E (2) — the shard that was red at the prior head in untouched empty-edit-delete.spec.ts cases now passes, confirming that failure was unrelated to this change. The new production-seam regression executes in Desktop Core at this head.
Prior-round non-findings re-verified and unchanged: provider lifetime under the keyed community/identity boundary, community-scoped persistence, frozen same-window ranking, external storage refresh, backfill and URL-only palette propagation, Huddle-root inheritance.
827813f to
cdeab45
Compare
Replace per-action-bar palette preparation, query observers and storage listeners with one content-stable tray snapshot below the keyed community and identity boundary. Preserve community-scoped recents, frozen session ranking, external storage refresh and custom emoji availability/URL updates. Cover shared preparation and lifetime behavior with real-provider tests and the production action bars with a mock-bridge browser regression. Co-authored-by: Princess Donut <5d97ac8c272fa949af56c71e586146b1706e5c4c3daa125db5b4625e12e0686c@buzz.block.builderlab.xyz> Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Mount the production MessageActionBar with its real query/provider/tooltip tree and assert rendered quick buttons, constant palette reads, one query observer and one storage listener across row growth, rerender and remount. Verify teardown releases both resources. Mutation checks restoring per-row query hooks or storage listeners each fail at 17 resources versus the required one. No production behavior changes. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Cancel pending channel lookup and close the tray immediately when the live text no longer has a channel query. Keep valid-query suggestions debounced. This prevents Enter from inserting the old channel at an obsolete offset after replacing an edit body. Freeze the debounce window in the non-empty edit browser regression and cover immediate invalidation, canceled publication and retained valid-query debouncing through the production hook. Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
cdeab45 to
08a6bf3
Compare
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewed base cec5c8fd9280d30f56effac701e1e19d5cfe6fea through exact head 08a6bf3948625188330a65a29dcf942d733c00f0.
Required fix
P2 — the PR's changed E2E test leaves the required Desktop gate red. In desktop/tests/e2e/empty-edit-delete.spec.ts:99-104, fake page time is installed before the app/channel/menu setup. In Desktop Smoke E2E (2), job 102974984964, the new non-empty-edit case timed out while clicking the Edit menu item on all three attempts; the job finished 1 failed / 410 passed, and the aggregate required Desktop check failed. This also reproduced locally as 3 passed, 1 failed, with the Radix Edit item becoming unstable/detached before the click.
Author action: make this changed edit journey deterministic and restore the required Desktop gate—either fix a demonstrated production action-bar/menu lifecycle regression, or keep/advance fake time so it does not interfere with the Radix menu-open/edit-click path while preserving the stale-channel-autocomplete reproduction. Push a new head and obtain a green exact-head rerun. Evidence that the same test fails unchanged at the base SHA would instead establish unrelated pre-existing test debt, but that comparison is not currently available.
Verification owner: PR author for diagnosis/remedy; required GitHub Desktop Smoke E2E gate for exact-head proof. Reviewer will re-check the product seam if the remedy changes provider placement/value stability, MessageActionBar, or focus behavior.
Quick-reaction implementation review
No separate quick-reaction product or systems defect was established. The provider is below AppReady and within the composite community/config/identity boundary (desktop/src/app/App.tsx:407-412,633-648); teardown releases its query observer and storage listener (desktop/src/app/providers/QuickReactionProvider.tsx:37-53). Main and Huddle companion windows both execute the same App root (desktop/src/main.tsx:82-100), bounding ownership to one provider per app window rather than one per message row.
Ordering, frozen-tray behavior, community scoping, stale/missing custom-emoji fallback and restoration, legacy null scope, malformed-storage fallback, labels/tooltips, and picker autofocus survived focused review. Production-row mutation checks were discriminating: reintroducing a per-row custom-emoji query or storage listener independently changed the expected count from 1 to 17 and failed the regression test; the exact source was then restored.
Exact-head evidence
- PASS:
pnpm test— 6500/6500 - PASS:
pnpm typecheck,pnpm check,git diff --check - PASS: isolated
CI=1 pnpm test:e2e:smoke quick-reaction-sharing.spec.ts— 1/1 - PASS: 14/14 focused product/lifecycle tests
- PASS: GitHub Desktop Core, Windows build, macOS build
- FAIL: GitHub Desktop Smoke E2E (2), as detailed above
- NOT RUN: native Huddle companion-window journey; source/lifetime proof is present, and this confidence gap is not separate author rework
The focused green evidence does not supersede a reproducibly red required gate introduced in the changed range.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: cec5c8fd9280d30f56effac701e1e19d5cfe6fea..08a6bf3948625188330a65a29dcf942d733c00f0 (exact live head)
Risk: high — shared app/community-scoped quick-reaction ownership plus a changed required Desktop E2E journey.
Blocking finding
[P2] Repair the changed fake-clock/menu interaction so the required Desktop Smoke gate is deterministic
desktop/tests/e2e/empty-edit-delete.spec.ts:99-104 installs the page clock before app/channel/menu setup in the changed non-empty-edit journey. At this exact head, required Actions job 102974984964 timed out while clicking the resolved Radix Edit menu item on all three attempts (log lines 38472-38603), ending 1 failed / 410 passed; the required aggregate Desktop check is red. An independent local run reproduced the changed row (3 passed, 1 failed) with the menu item becoming unstable/detached before click.
No production quick-reaction defect was established: the provider is correctly keyed and scoped below AppReady, owns one observer/listener per app window, releases both on teardown, and serves both main and Huddle roots (desktop/src/app/App.tsx:407-412,633-648; desktop/src/features/messages/reactions/QuickReactionProvider.tsx:37-53; desktop/src/main.tsx:82-100). Production-row mutations independently restoring per-row queries and storage listeners failed at 17 observers/listeners versus 1, then were restored cleanly.
Author action: keep fake time out of the Radix menu-open/edit-click path (or explicitly advance it), preserve the stale-channel-autocomplete reproduction, and push a head where the changed required Desktop Smoke E2E gate passes reliably. If the same unchanged row is demonstrated failing at the fixed base under the same command, that would instead reclassify the failure as external test debt.
Verification owner: author for diagnosis/remedy; required Desktop Smoke E2E gate and reviewer for exact-new-head proof. Native Huddle observation remains reviewer/tooling-owned and is not author rework by itself.
Integrated exact-head evidence
pnpm test: 6,500/6,500 passed.pnpm typecheck,pnpm check, andgit diff --check: passed.- Isolated quick-reaction smoke row: 1/1 passed.
- Focused quick-reaction tests: 14/14 passed; both observer/listener mutants were killed.
- GitHub Desktop Core, Windows build, and macOS build: passed.
- Required Desktop Smoke E2E (2): failed as described above.
- No native companion-window journey was run; this is a confidence gap, not a second blocker.
Any new head invalidates this verdict until its delta and required gate are reviewed.
Summary
Prepare the three-item quick-reaction tray once per mounted app/community session instead of once per message action bar. Rows consume one content-stable snapshot of resolved emoji and image URLs.
QuickReactionProviderbelow the existing keyed community/identity boundary. Remove the old module-global tray cache rather than adding another cache layer.AppReady.Related issue
N/A. Search for
"quick reaction"found no duplicate of this preparation-sharing change. Closest existing work: #6892 (restored message quick reactions) and #6263 (restored emoji recents).Testing
Verified at clean commit
4826fd35f20bcd9cdffbd4c2f0df98be09227d68:pnpm test:e2e:smoke quick-reaction-sharing.spec.ts reaction-order.spec.ts inbox-reactions.spec.ts custom-emoji-ui.spec.ts: E2E build and 7 browser cases passed, using this worktree's mock bridge, Chromium and isolated test server.Performance evidence and limits: with a 128-emoji fixture, increasing provider consumers from 1 to 64 adds zero palette reads and retains exactly one quick-tray query observer, one storage listener and one shared snapshot. This is deterministic evidence that repeated preparation was removed, not an end-to-end latency benchmark. Other emoji consumers still have their own observers. Native WKWebView cold/warm channel-switch latency, scroll stability and composer responsiveness have not been measured for this branch. Huddle coverage is source/lifetime review, not a native companion-window run.
No intended visual change, so no before/after screenshot comparison. Independent production review found no blocking issue. Full repository-wide
just ciwas not rerun locally; applicable hook gates and the browser checks above were run, with broader validation left to CI.Follow-ups deliberately excluded
Video-attachment classification and viewport-based image warming remain separate slices. No claim that quick reactions account for the reported multi-second channel-switch delay.
Implemented by Carl with provider regression tests by Princess Donut, AI agents. Submitted via Wes's GitHub account.
Review follow-up
Commit
827813f2ace0b2b16aaf452db8a220cc4d97e0fcadds a production-bound sharing regression with realMessageActionBars. Across 1→16→16→1→16 bars, it checks actual custom quick buttons, one palette query observer, one storage listener and no additional palette reads. Restoring either a per-row query hook or storage listener fails the new test with 17 versus 1; both mutations were removed. Full desktop suite at this clean head: 5,889/5,889 passed, with normal pre-commit/pre-push gates. Production code is unchanged from the originally tested head above.