镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix(emoji): bound the legacy custom-emoji union and keep the last good mobile palette - #8792

Open
blakemcanally wants to merge 1 commit into
mainfrom
bmcanally/emoji-union-guards
Open

blakemcanally wants to merge 1 commit into
mainfrom
bmcanally/emoji-union-guards

Conversation

@blakemcanally

Copy link
Copy Markdown
Contributor

Summary

This change adds guards to the legacy custom-emoji union.
It does not add the new kind 30180.
Desktop now bounds the union and reports when it drops data.
Desktop keeps the last good catalog when a refresh fails.
Mobile now keeps the last good palette when a fetch fails.
The mobile picker shows a Retry control after a failure.

Design

Desktop:

  • COMMUNITY_EMOJI_SET_LIMIT (500) names the set limit. The fetch uses it.
  • When the relay returns exactly that many sets, the code calls console.warn. It also sets setLimitReached in the catalog report.
  • COMMUNITY_EMOJI_ENTRY_CAP is 4000 entries.
  • Estimate: one entry is about 160 UTF-16 code units (shortcode, Blossom URL, JSON framing). That is about 320 bytes. 4000 entries are about 1.3 MB.
  • localStorageQuota.ts keeps pure caches under 2 MiB. 1.3 MB fits with margin.
  • Today the catalog is not stored in localStorage. The cap also limits the composer regex and the picker list.
  • Over the cap, the union keeps the first entries in shortcode order. The winner per shortcode is chosen first. The result does not depend on fetch order.
  • The code counts the dropped entries in droppedEntries and calls console.warn.
  • useCustomEmojiCatalogStatus() returns refreshFailed, setLimitReached, and droppedEntries. The settings card shows a short status note.
  • A failed fetch rejects. React Query keeps the last good data. The status hook reports refreshFailed for any failed fetch, including a failed first load, and hasCatalog tells the UI whether an older list still shows. The code never turns a failure into an empty list.
  • The catalog report query uses gcTime: Infinity, so the notice survives the 20-minute poll.
  • The notice text comes from the pure function customEmojiCatalogNotice.
  • The 20-minute poll and the live invalidation are unchanged.
  • The report lives in the query cache. No new module-level cache exists, so resetCommunityState() needs no change.
  • A set larger than the 512 KiB WebSocket frame (crates/buzz-relay/src/config.rs) can never reach the client. The PR adds no code for that case.

Mobile:

  • CustomEmojiPaletteNotifier keeps the last good palette.
  • A failed fetch keeps that palette and sets customEmojiRefreshFailedProvider.
  • A failed first fetch has no palette. The state becomes an error, and the picker shows Retry.
  • A generation counter stops an old fetch from overwriting a newer one.
  • refresh() no longer clears the palette to a loading state.
  • The picker shows _CustomEmojiRetryBanner above the grid. The Retry button takes its only label from its own text.
  • The mobile union cap is out of scope for this item. Desktop caps at 4000 entries and mobile does not, so the two clients can differ above that size.
  • The entry cap is a stopgap. The catalog is not stored in localStorage today, so the budget argument is a ceiling, not a measured limit. Over the cap, desktop drops the last shortcodes in alphabetical order.
  • The iOS native picker has no Retry control. Only the Flutter picker shows it.
  • If the session is disconnected at the first build, the palette is empty without a banner. This matches the earlier behavior.

Testing

Desktop (all pass):

  • pnpm -C desktop install --frozen-lockfile
  • pnpm -C desktop test (6918 node tests, 171 jsdom tests)
  • pnpm -C desktop typecheck
  • pnpm -C desktop check (3 existing warnings in other files)
  • just file-size-check

New desktop tests: customEmoji.test.mjs (cap, order, warn, limit flag) and catalogRefresh.test.mjs (failed refresh keeps data, report is published, failed first load is flagged, report is never garbage-collected, notice text for every branch). They call the production queryFn with a stubbed relayClient.fetchEvents.

Mutation checks on desktop (each break made a test fail, then I restored the code):

  • Cap disabled: 3 tests failed.
  • Limit flag disabled: 2 tests failed.
  • console.warn removed: 2 tests failed.
  • Fetch failure swallowed into an empty list: 2 tests failed.
  • Report not published: 1 test failed.
  • Failure flag ignores a failed first load: 1 test failed.
  • Report gcTime reduced: 1 test failed.
  • Notice ignores hasCatalog: 1 test failed.

Interop evidence: not applicable. This change adds no wire format. The union reads the same kind 30030 events as before.

Mobile: ran on a macOS laptop after the Blox run, because the workstation cannot reach pub.dev.

  • just mobile-install mobile-check mobile-test: all pass.
  • The first run found two defects in the new test. It did not import savedPrefsProvider. The first-load case also used pumpAndSettle, so Riverpod's automatic provider retry (relayProviderRetry) used the scripted success before the test checked the banner. Commit 55d1fc9 fixes the test. The product code did not change.
  • Mutation check: with the hasError guard in emoji_picker.dart removed, the first-load test fails.
  • The first Blox commit used --no-verify, because without resolved packages the pre-commit Dart formatter rewrote about 250 unrelated files. The fix commit ran the hooks, and mobile-fmt passes.

Manual test steps

Desktop:

  1. Run . ./bin/activate-hermit && pnpm -C desktop test. Success: 0 failures.
  2. Run just desktop-dev. Open Settings, then Custom emoji. Success: the community list shows. No status note shows.
  3. Stop the relay and wait for a refresh (or reconnect). Success: the list stays. The note "Could not refresh community emoji." shows.

Mobile (needs network):

  1. Run just mobile-install mobile-check mobile-test. Success: all pass, including the new journey test.
  2. Run just mobile-dev. Open the emoji picker. Turn on airplane mode. Trigger a refresh (switch community, or reconnect).
  3. Success: the Custom section stays. The banner "Couldn't load custom emoji." and a Retry button show. Turn the network on and tap Retry. The banner goes away.

🤖 Generated with Claude Code

@blakemcanally
blakemcanally marked this pull request as ready for review October 30, 2026 19:49
@blakemcanally
blakemcanally requested a review from a team as a code owner October 30, 2026 19:49
@blakemcanally
blakemcanally enabled auto-merge (squash) October 30, 2026 19:49
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T19:53:06.195226Z 55d1fc9 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 30, 2026
@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is ff15c347c2f6cc688c163495125e849f85bbd55c...f3698825b1562c3d0eabc93c494016e0a24b1d54.
A new review must complete for this exact range. When manual authorization
is required, a user with write access must comment exactly
@buzz-security-review f3698825b1562c3d0eabc93c494016e0a24b1d54 to authorize a new review.
Any previous review applies only to its recorded range.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55d1fc9d35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…d mobile palette

Desktop: name the 500-set fetch limit, cap the union at 4000 entries with a
deterministic shortcode-order subset, warn and report when the set limit is
reached or entries are dropped, and expose the report and refresh-failure
state through useCustomEmojiCatalogStatus. The status hook reports any
failed fetch, including a failed first load, so a cold-start timeout no
longer shows as an empty healthy catalog. The report query is never
garbage-collected, and the notice text is a tested pure function.

Mobile: a failed palette fetch keeps the last good palette, raises a refresh
failure flag, and the picker shows a Retry banner. Switching relay drops the
retained palette and the refresh-failed flag so one community's palette
never shows in another.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: bmcanally <bmcanally@squareup.com>

@joahg joahg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Read the desktop cap/status changes and the mobile last-good palette logic; the community-switch finding is fixed. GitHub reports the branch as conflicting with base, so it needs a rebase before merge.

_lastGood = null;
_lastGoodRelayUrl = relayUrl;
if (switched) {
state = const AsyncData([]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Assigning state inside build() and deferring the flag reset with Future.microtask is easy to get wrong. If the switch test is flaky or the flag lags a frame, resetting both in one place would be safer.

This branch was previously deployed

1 inactive deployment
codex-review — f3698825 Deployed Nov 5, 2026 by blakemcanally via Run Codex Security Review #12929
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.

2 participants