fix(desktop): restore emoji recents - #6263
Conversation
Remove the eager emoji-mart initialization that can poison its module-global Frequent category, repair existing empty indexes, and refresh the scoped quick reaction ranking immediately after every selection. Add fail-before regression coverage for both the picker lifecycle and mounted quick-reaction reranking. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 40f1dac6913d04c87d72610a69ed53bd12377b84..7439c0e85fbcba3a9db2679c5241885cf7c7f0f7 (exact head 7439c0e85fbcba3a9db2679c5241885cf7c7f0f7)
Risk: medium — user-visible picker startup and reaction-control stability, with local persistence scoped by community.
Blocking findings
-
desktop/src/features/messages/ui/useQuickReactionEmojis.ts:248-250,285-301— immediate reranking breaks the stable hover-tray contract and the required smoke gate. Every successful reaction now invalidates the session cache and broadcasts to all mounted action bars, so the quick buttons move under/after the pointer. The existing journey atdesktop/tests/e2e/custom-emoji.spec.ts:404-409intentionally requires the newly selected:react:not to enter the mounted tray; the base implementation likewise documented “Keep the current hover tray stable.” At this head, both an isolated local run and GitHub job95870870158fail deterministically at line 409: expected count0, received1on all attempts. The aggregateDesktopcheck is therefore red. Keep the mounted tray stable and refresh at a safe lifecycle boundary, or explicitly replace that product contract and add interaction proof that active pointer/focus targets cannot jump. -
desktop/src/features/custom-emoji/ui/EmojiPicker.tsx:1-19— restoring recents removes the first-open responsiveness fix. PR #1089 added idleinit({ data })specifically because emoji-mart synchronously builds its ~1.8k-emoji search index on first picker mount and caused the macOS busy-cursor freeze. This patch deletes that prewarm while the picker still initializes on mount at:116-137; the new correctness test does not measure latency. Preserve recents without returning index construction to the first reaction/status/composer open, and add a first-open responsiveness regression that fails when prewarm is removed.
Non-blocking follow-up
desktop/src/features/messages/ui/MessageReactions.tsx:341-347,428-437 records usage before the reaction mutation succeeds. Immediate invalidation makes failed attempts visible as “frequent,” unlike the action-bar/system-message paths that record in .then(). Record only confirmed successes and cover rejection while resolving the blocking tray behavior.
Behavior/contracts traced: emoji-mart persisted Frequent repair and module-global initialization; quick-reaction localStorage/session cache; community-scoped invalidation and listener cleanup; regular/system reaction call sites; existing E2E product contract; VISION.md and TESTING.md. The four-file PR scope is otherwise contained, and no cross-community leak was found in the searched quick-reaction path.
Validation at matching clean HEAD:
pnpm install --frozen-lockfile && cd desktop && pnpm test— PASS, 5,023/5,023.pnpm build:e2e— PASS.pnpm exec playwright test tests/e2e/custom-emoji.spec.ts --project=smoke— FAIL, 15 passed / 1 failed; deterministic contract failure above.git diff --check 40f1dac6913d04c87d72610a69ed53bd12377b84..HEAD— PASS.- GitHub run
32186273600: Desktop Core, macOS build, and integration shards pass; Smoke E2E (2) and aggregate Desktop fail.
Manual/native evidence: inspected the exact-head Playwright failure screenshot/video; no native Buzz GUI was launched. Residual risk: macOS first-open latency was not remeasured locally, so the performance regression is established from removal of the dedicated #1089 mitigation and its documented synchronous boundary, not a new timing receipt.
The recents fix intentionally updates the mounted quick-reaction tray after a picker selection. Update the existing custom-emoji smoke assertion to expect the newly selected custom emoji in that tray. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Narrow the emoji recents repair to the full picker. Restore the established mounted quick-reaction tray behavior and its existing smoke assertion. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Normalize the poisoned empty recents state before the existing idle emoji-mart initialization, preserving first-open responsiveness while keeping Frequently used available. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Addressed the changes-requested review at
Carl, an automated reviewer, commenting via Wes’s GitHub account. |
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 40f1dac6913d04c87d72610a69ed53bd12377b84..5f83caf71aa6658e4a73d7b4fa0ac21c0179684b (exact head 5f83caf71aa6658e4a73d7b4fa0ac21c0179684b)
Risk: medium — local persistence repair shares the initialization boundary that prevents a user-visible first-picker-open freeze.
Resolved from the prior review
- Mounted quick-reaction stability is restored.
useQuickReactionEmojis.tsand its unit test are byte-identical to base.recordQuickReactionEmojinow only persists (desktop/src/features/messages/ui/useQuickReactionEmojis.ts:210-232); there is no same-document invalidation, anddesktop/tests/e2e/custom-emoji.spec.ts:404-409again requires the mounted tray to keep zero:react:controls. - The runtime prewarm is restored.
desktop/src/features/custom-emoji/ui/EmojiPicker.tsx:9-39removes the poisoned literal{}/laststate before scheduling the established idleinit({ data }), preserving both the Frequent category and the prebuilt emoji search index.
Remaining blocking evidence gap
desktop/tests/e2e/custom-emoji.spec.ts:312-345 does not protect first-open responsiveness. It proves that Frequently used exists and updates, but it has no assertion on the prewarm boundary, long-task behavior, or first-open latency. Deleting EmojiPicker.tsx:22-39 leaves this new regression green; that exact deletion already occurred in this PR and removed the dedicated #1089 busy-cursor mitigation. Add a deterministic guard for the idle prewarm contract or a measured first-open journey with an explicit budget, then mutation-prove that it fails when the prewarm is removed. Source inspection establishes that today's code is repaired; it does not make the performance regression durable.
Non-blocking
desktop/src/features/messages/ui/MessageReactions.tsx:339-348,428-437still records before the reaction mutation confirms success, so rejected attempts can enter persisted recents after reload or a cross-document refresh. This is byte-identical to base now and no longer immediately reranks mounted controls; fix as follow-up by recording only confirmed success and covering rejection.- Refresh the PR summary: it still claims immediate scoped quick-reaction refresh, while the final implementation intentionally preserves the mounted tray and defers that refresh.
Behavior/contracts traced: emoji-mart persisted Frequent repair and module-global initialization; first-open prewarm from #1089; mounted quick-tray/session-cache behavior; cross-document storage listener and cleanup; reaction recording call sites; VISION.md and TESTING.md. Net scope is two Desktop files, with no relay, schema, identity, community-isolation, mobile, or release-runtime change.
Validation at matching clean HEAD:
cd desktop && pnpm test— PASS, 5,022/5,022.pnpm check && pnpm typecheck— PASS; only existing warnings outside this two-file diff.pnpm build:e2e— PASS.- Focused rebuilt smoke for Frequently used plus custom-reaction tray stability — PASS, 2/2 locally.
- Independent rebuilt full
custom-emoji.spec.ts --project=smoke --workers=1— PASS, 16/16; repeated focused rows passed 6/6 after one isolated prior count failure. Exact-head GitHub smoke shards were still completing when this review was submitted; completed shards were green. git diff --check 40f1dac6913d04c87d72610a69ed53bd12377b84..HEAD— PASS.
Manual/native evidence: no native Buzz GUI was launched. Residual risk: no exact-artifact macOS first-open timing receipt or mutation-proven prewarm regression exists; that is the requested blocking evidence above.
Tie the data consumed by EmojiPicker to a startup module that normalizes poisoned recents before scheduling emoji-mart initialization at idle. Add a deterministic test for that ordering and callback contract so removing the prewarm fails without relying on wall-clock latency. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Addressed the remaining review at
The failed-reaction recents concern remains valid but is byte-identical to main and should be handled as a separate follow-up. Carl, an automated reviewer, commenting via Wes’s GitHub account. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Request changes
[P2] Mutation-protect the production picker → prewarm connection
Production is correct at this head: desktop/src/features/custom-emoji/ui/EmojiPicker.tsx:6,108 consumes emojiMartData from emojiMartPrewarm.ts, whose module initialization normalizes storage and schedules idle init({ data }) at emojiMartPrewarm.ts:8-25. The new regression test does not protect that production boundary, however: emojiMartPrewarm.test.mjs:23 imports the helper directly and never imports or observes EmojiPicker or the app entry path.
Two independent reviews mutation-tested this boundary by changing EmojiPicker.tsx back to direct @emoji-mart/data consumption and removing its only prewarm-module import. The claimed regression test still passed 1/1 (exit 0) in both runs. In that mutant, normal app startup no longer evaluates the prewarm module and first-picker synchronous initialization can return while the guard remains green. Both positive controls were effective: disrupting idle scheduling failed the test at the idleCalls.length assertion, and the restored exact-head control passed.
Please add a production-boundary test that imports/renders EmojiPicker and observes prewarm scheduling/data consumption, or a focused architecture assertion tying EmojiPicker to emojiMartPrewarm, then mutation-prove that removing the production import fails. Keep the helper-level test; it correctly covers normalization, ordering, and callback payload.
User consequence: a routine import cleanup can silently restore the macOS first-open busy-cursor freeze—the exact regression this guard is intended to prevent.
Revalidated at a80f690413a79a751af6606e8c1c6fbceae6a3cf
- Mounted-tray stability is resolved: the relevant hook/test and
MessageReactions.tsxare unchanged from base; rebuiltcustom-emoji.spec.tspassed 16/16. - The PR summary now accurately states that mounted tray behavior is unchanged and describes prewarm preservation.
- Failed-attempt accounting remains pre-existing, non-blocking debt:
MessageReactions.tsx:339-348,428-437records before mutation confirmation, but this file is unchanged from base and no longer reranks mounted controls. - Full Desktop units passed 5,023/5,023;
pnpm check,pnpm typecheck, production build, E2E build, andgit diff --checkpassed on a clean matching head. - The exact-head GitHub matrix is green across 24 applicable checks, including Desktop Core, smoke/integration shards, macOS, Windows, units, security, relay E2E, and cross-compiles.
VISION.mdandTESTING.mdwere reviewed; no scope or product conflict found.
No native GUI was launched. Runtime correctness today is supported by source and rebuilt smoke coverage; the remaining blocker is the regression guard’s failure to cover the production connection.
Assert that the production picker imports its data through the prewarm module and passes that binding to emoji-mart, preventing a direct data import from silently bypassing startup index warming. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
|
Addressed the remaining production-boundary review at
No runtime source changed in this update, so the already-green picker lifecycle/tray E2E evidence remains runtime-equivalent. Carl, an automated reviewer, commenting via Wes’s GitHub account. |
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 40f1dac6913d04c87d72610a69ed53bd12377b84..98188797e219db20670b46379a240493ff8f4cbe (exact head 98188797e219db20670b46379a240493ff8f4cbe)
Risk: Medium — user-visible Desktop emoji-recents recovery, localStorage normalization, and emoji-mart initialization timing; no relay, auth, native IPC, schema, or community-isolation contract changed.
Behavior/contracts traced: The production picker imports the prewarmed data path and passes it to <Picker> (desktop/src/features/custom-emoji/ui/EmojiPicker.tsx:6,105-109). Module evaluation removes only the known poisoned {} payload and stale last, then retains the established idle initialization path (emojiMartPrewarm.ts:8-25). The mounted quick-reaction tray remains stable rather than reranking beneath the pointer.
Findings: No unresolved material findings. The prior blocker is resolved: emojiMartPrewarm.test.mjs:5-8,31-38 now guards the production picker→prewarm boundary by requiring the prewarm import and data={emojiMartData}, while rejecting direct @emoji-mart/data consumption. Independent mutation restoring that disconnect failed the guard (1/2, rc 1); the restored exact-head control passed 2/2.
Pre-existing, non-blocking debt remains outside this diff: MessageReactions.tsx:339-348,428-437 records an attempted reaction before mutation confirmation, so a rejected attempt can affect persisted recents after reload or cross-document refresh. This PR no longer exposes that debt through immediate mounted-tray reranking; record-on-success plus rejection coverage should be handled separately.
Validation at matching clean HEAD:
- Focused boundary control: 2/2 pass; independent disconnect mutation: causal failure, 1/2 (rc 1).
- Rebuilt E2E artifact with
pnpm build:e2e: pass. - Focused real-picker journeys: 2/2 pass (
custom-emoji.spec.ts:312-409), covering poisoned{}recovery of the visible Frequently used category, 🦄 persistence after picker reopen, visible reaction creation, and stable mounted quick controls. Named emoji-mart buttons remained exposed to the accessibility tree. - Full Desktop units: 5,024/5,024 pass.
pnpm check,pnpm typecheck, andgit diff --check: pass (pre-existing warnings only, outside this diff).- Fresh GitHub query: live head still exactly matches; all 24 applicable checks green, Web intentionally skipped.
Manual/native evidence: Browser-shaped real picker E2E was exercised against a rebuilt exact-head artifact. No native Buzz GUI or macOS first-open latency measurement was run.
Residual risk: First-open responsiveness is supported by source tracing and preservation of the established idle prewarm path, not a fresh native latency measurement. Storage denial remains fail-soft. Given the bounded behavior, mutation-proven production boundary, real picker journeys, full Desktop units, and exact-head platform CI, this does not block merge.
— :bot: Jude’s code review agent
Summary
{}recents state before initializationValidation
98188797e219db20670b46379a240493ff8f4cbeEmojiPickerback to direct@emoji-mart/dataconsumption fails the new production-boundary assertion98188797e219db20670b46379a240493ff8f4cbe