Fix clipped emoji in reaction pills - #339
Conversation
Signed-off-by: OpenAI Codex <codex@openai.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1465ef2527
ℹ️ 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".
| await expect(pill).toHaveCSS("height", "28px"); | ||
| await expect(glyph).toHaveCSS("min-width", "18px"); | ||
| await expect(glyph).toHaveCSS("height", "18px"); | ||
| await expect(glyph).toHaveCSS("font-size", "14px"); |
There was a problem hiding this comment.
Every Chromium and WebKit functional run also executes reactions-polish.spec.mjs, whose line 30 still requires this glyph to have font-size: 12px; the changed CSS and this new assertion compute 14px, so the mandatory browser gate will fail regardless of whether the new case passes. Update or consolidate the existing assertion as part of this change.
AGENTS.md reference: AGENTS.md:L144-L149
Useful? React with 👍 / 👎.
| import react from "@vitejs/plugin-react"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| test("native reaction emoji fit inside compact pills", async ({ page }) => { |
There was a problem hiding this comment.
This case uses the standard stateless source configuration and no mutable server middleware, but declaring it with the base test starts and stops another Vite server for each browser execution instead of sharing the worker-scoped server already exposed by the imported sourceTest. Use sourceTest with the relative fixture URL so the added Chromium/WebKit coverage does not impose redundant server and optimizer startup work on every browser gate.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review via Wes’s account.
One actionable finding: the existing reaction-polish browser journey still requires the old 12px native-emoji size, while this change and its new test require 14px. The inline comment identifies the smallest repair. No additional actionable production-code defect found in this two-file change.
Reviewed head: 1465ef252721110a606c9244d352e1405165b473
Target base and merge base: 85d6bf82c54d1c8d930d58444597a1fe31cc8975
Source-only review of the CSS, reaction glyph/pill/preview consumers, typography tokens, fixtures and browser discovery (23 pinned source blobs verified; no dirty checkout inputs). No local tests, builds, PR-code execution or app launch. One read-only CI snapshot plus its completed Chromium 2/3 job log confirmed the retained assertion fails with expected 12px, received 14px: https://github.com/block/buzz-app/actions/runs/36412891794/job/108897041312 . Other browser jobs were still running in that snapshot; this is not an all-green CI or native/visual acceptance claim. Computed-style assertions do not establish unclipped glyph rendering on every platform/font or text-size setting.
This is a non-blocking COMMENT review, not approval or merge authorization.
| font-size: var(--text-caption); | ||
| line-height: 1; | ||
| height: 18px; | ||
| font-size: var(--text-body-sm); |
There was a problem hiding this comment.
[P2] Update the existing reaction-polish size assertion alongside this change
Switching native emoji to --text-body-sm changes the default computed size from 12px to 14px, but tests/browser/reactions-polish.spec.mjs:30 still asserts 12px on the same native glyph. That retained journey is selected in both browser projects, so adding the new 14px assertion in reactions.spec.mjs leaves the suite inconsistent and stops the older journey before its preview/toggle/inline-add checks. The completed Chromium 2/3 job already confirms this exact failure (expected 12px, received 14px): https://github.com/block/buzz-app/actions/runs/36412891794/job/108897041312 . Update the retained assertion to the intended 14px contract, keep its downstream behavior checks, and revalidate both reaction specs in Chromium and WebKit rather than reverting the glyph fix or weakening the assertions.
Signed-off-by: OpenAI Codex <codex@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: no remaining actionable findings. The prior assertion mismatch is repaired: the retained reaction-polish journey now expects 14px, matching the production token. This is the only change since the previously reviewed head; downstream interaction assertions remain intact.
- Validation: exact-head CI passed. Job logs confirm all six cases across
reactions.spec.mjsandreactions-polish.spec.mjspassed in both Chromium and WebKit (12 executions), including wrap/preview/toggle/add and long-content containment. I inspected both uploaded native-reaction screenshots; the pictured thumbs-up is visibly contained. - Optional: reusing
sourceTestremains a test-setup cleanup, not a correctness blocker. - Limits: source and hosted-CI evidence review, no local execution. The screenshots cover the CI Linux font/default-size rendering, not every OS, emoji or enlarged-text setting. Native/human acceptance is not established by this review. No internal-information or generated-artifact additions found in the three-file diff or PR description.
Head: a7131063d67e1a5493f91f60a773ff6afc8b9109
Base: 85d6bf82c54d1c8d930d58444597a1fe31cc8975
COMMENT only; not approval or merge authorization.
* origin/main: Refine presence labels and status colors (#323) Polish media viewers, playback controls, and galleries (#341) perf(mentions): name each choice set once and skip closed choosers (#310) Fix clipped emoji in reaction pills (#339) Delay message timestamp tooltips by 500 ms (#321) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
* origin/main: Fix clipped emoji in reaction pills (#339) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
Summary
Validation