Polish inline message reactions and previews - #213
Conversation
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: 9847413d5f
ℹ️ 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".
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 9847413d5f4d005354af16ddceb76654f507991e. Resolve the five inline findings with targeted regression coverage; the findings are P2 correctness/test defects, not an urgent production incident.
- Restore the existing media-comment journey in both browser engines. Exact-head CI fails in Chromium and WebKit because
tests/browser/messages.spec.mjs:547now matches both Add reaction controls. Preserve the intended placements and scope the test to the control it exercises; this failure alone does not establish broken media behavior. - Validation was source-only on the pinned base/head, with an independent UI/input lane and existing hosted CI. JavaScript and Rust/tool jobs passed, but that does not exclude the key-dependent test defect below. No code/tests were executed or CI rerun for this review.
- Browser/Tauri share these components; live relay, rendered geometry/touch/screen-reader behavior, and native-window acceptance remain unverified. After fixes, provide the affected full test-file results and green required CI.
9847413 to
b761603
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7616039db
ℹ️ 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".
b761603 to
b1e48ba
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1e48ba86f
ℹ️ 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".
|
🤖 Wes’s five inline findings are addressed on PR head Validation on the updated branch:
I left the review threads open for your re-review. |
929f43a to
9e1fa17
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e1fa17b6f
ℹ️ 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".
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 9e1fa17b6f1f5b816a73046ff84b3984716d26b4 against e02fe33220fa497a2e7ee780c8295517b4f967fa.
- P2: Bound reaction text and shortcode fallbacks. Reproduced in Chromium and WebKit. Merge criterion: contained text/counts with full accessible labels and regression coverage for both long inputs.
- P2 validation: Control the preview-delay clock. The prior timing gap remains. Prove the 1200ms boundary, warmed-row switching and reset deterministically.
- The five findings from my earlier review are fixed. The publication gate is fixed; all current PR commits have DCO trailers and hosted DCO succeeds.
Validation: 29/29 focused unit checks; 15/16 browser checks across Chromium/WebKit (reactions, reaction polish, messages, avatar shapes). Chromium reactions.spec.mjs:310 failed the Escape → Enter reopen path; WebKit passed. Failure trace preserved, no retries. Its cause/preexistence is not established, so I am reporting it separately rather than claiming a third introduced defect. Current-head hosted CI is green. Packaged-native/live-account, touch-device and screen-reader acceptance were not exercised.
No production changes, approval or merge performed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 356f49225f
ℹ️ 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".
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
Signed-off-by: OpenAI Codex <codex@openai.com>
356f492 to
c1e343b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1e343b007
ℹ️ 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".
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear for the pinned snapshot: no remaining material blocker found at c1e343b007ec76d48471944b9857866f3f432b99 against 1e15d5d33b6b89c673ceefabeb55d1e5dbb8d579.
- Both prior P2 exit criteria are satisfied. Long reaction text/fallback shortcodes are bounded while retaining full accessible labels and separate counts. The browser regressions now exercise truncation and the deterministic 1199ms/1200ms preview boundary, immediate warm switching, and reset after leaving.
- The five earlier fixes remain intact. Source review also covered the rebased picker placement, Escape/focus-return path, and shared preview/popover integration. Existing CI run 36058221006 passed JavaScript, Rust, measurements, and Chromium/WebKit journeys, including the reaction suites. Its tested checkout was synthetic merge
c1e4cba74a6719f2abcfc2b5c62333a2a12940a0, combining this head with the pinned base. - Limits: that run does not establish integration with the newer main tip observed at
b276b867d91646522c6ed354cfbedffb1e101882. Windows native validation was skipped; packaged-native/live-account, touch-device, and screen-reader acceptance were not exercised by this review. These are disclosed validation gaps, not evidence of an introduced defect.
Focused re-review and rebase integration assessment were source-only on Blox; no PR code execution or CI reruns. This is a COMMENT, not approval or merge authorization, and it does not dismiss earlier reviews.
Preserve the compact quick-reaction wrappers while resolving the React import conflict. Keep PR-only screenshots outside the source tree. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
The existing main baseline fails its no-owner-read assertion because public metadata discovery adds an exact-target kind-10100 read. Assert all three supported requests while retaining the strict count and target guard. No profile production behavior changes. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Carl, an automated reviewer, commenting via Wes’s GitHub account. Superseded: all code findings in this review are fixed and verified at d61a411. Dismissal is not approval; the separate PluginImport test failure and required CI/reviewer gates remain disclosed.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reaction/integration review clear at d61a4110; not merge-ready. Main fff36cc7 is integrated, the four stale threads are resolved, and the obsolete blocking reviews are dismissed. No approval or merge performed.
- All 24 affected Chromium/WebKit checks and mandatory push hooks passed (TypeScript, design guards, 1,764 unit tests). The profile-test correction is test-only, preserves the no-owner-read guard, and fixes a failure reproduced on the clean main baseline. Independent review found no remaining introduced code blocker.
- External validation gate: the full unit package is 3,903 passed / 1 failed:
src/app/PluginImport.test.tsx:99expects “stays enabled” but receives “stays on”. Test and implementation are unchanged from main. Resolve this separately or integrate its upstream fix before calling the full suite green. New-head CI was in progress at handoff; hosted DCO succeeds. Required CI/code-owner approval remain outstanding. - Exact commands, test-layer accounting, removed PR-only artifacts and untested native/live-account/touch/screen-reader surfaces are recorded in the updated PR description. No claim of whole-app stability or release validation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61a4110bc
ℹ️ 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".
| ), | ||
| ]; | ||
| const revealName = () => { | ||
| void session.profiles.ensure(authors, "background").catch(() => {}); |
There was a problem hiding this comment.
When a reaction has more than 1,024 distinct authors, ProfileQueries.ensure rejects the entire request before fetching any profiles, and this catch silently discards that error. Session windows can retain up to 2,000 overlay events, so a popular reaction can reach this case; its preview then remains a very large list of public-key prefixes instead of resolved names. Bound/truncate the displayed reactor list and only ensure those IDs rather than passing every author.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
Signed-off-by: OpenAI Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 571ac18810
ℹ️ 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".
| loadedReactors.get(author)?.name ?? | ||
| profiles?.get(author)?.name ?? | ||
| author.slice(0, 10), |
There was a problem hiding this comment.
When two channel members share a profile name, or a reactor is an agent with a locally resolved name, these direct Profile.name lookups bypass the active channel identity policy and can render an indistinguishable Alex, Alex list or the wrong agent label. src/shared/design-system/DESIGN.md:84-98 requires channel-member comparison and qualified identities; pass each reactor through useChannelIdentityNames(session, row.channelId) and use the shared public-key fallback before joining the preview names.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review, published via Wes’s GitHub account.
No actionable defects found in this focused merge follow-up. Reviewed head 571ac18810030ff7a289706723e4f2a85285d56a against base 0db7ada55385fa04743f44dcc624d48fdd6fe0e8; compared the incoming changes with the previously reviewed d61a4110bc680d85a49993068104ab9b1dc1056f.
- Fifteen of the 19 PR files have identical Git blobs to that previously reviewed head. I checked the four changed files and the relevant message-row, profile-subscription, outbox/projection and shared-overlay integration rather than reopening unrelated, previously settled areas.
- The prior fixes remain present in source: catalog-specific custom-emoji selection, a dedicated reactor-profile subscription, guarded pending/failed toggles, literal shortcode fallbacks and bounded reaction text. The controlled-clock preview-boundary regression and explicit custom-reaction timestamps remain intact. Incoming message millisecond ordering does not replace the reaction grouping/order logic.
- The profile-test merge retains the explicit no-owner-read assertion and waits for the expected public-metadata reads. The previously reported
PluginImport.test.tsxcopy mismatch has an upstream correction in this snapshot; that file now matches the pinned base. I am not carrying the old-head failure forward as a current finding.
Validation limits: source only; no tests, builds, PR-code execution, app launches or CI reruns. Downloaded sources were checked against their pinned Git blob IDs; no local working-tree code was used. A single hosted CI snapshot associated with this head showed JavaScript, Rust/tool integration, browser measurements, DCO and one Chromium shard successful, with five browser shards still running and Windows native validation skipped. This is not an all-green claim. Rendered geometry, touch, screen-reader, packaged-native and live-account behavior remain unverified by this review.
This is a non-blocking COMMENT, not approval or merge authorization.
…ad-on-send * origin/main: (58 commits) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) fix(status): reopen a Today status as Today near 16:00 (#275) test: use current navigation for GIF send roundtrip (#309) Fix composer focus when selecting channels and DMs (#307) fix: retire mention searches after chips and refuted prose (#303) ... # Conflicts: # src/features/messages/MessageComposer.test.tsx # src/features/messages/MessageComposer.tsx
* origin/main: (45 commits) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentCard.tsx # src/bundled/agents/AgentsPage.tsx
* origin/main: (36 commits) Delay message timestamp tooltips by 500 ms (#321) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentEditor.tsx # src/bundled/profiles/ProfileAgentIdentity.test.tsx
Summary
Integration update (2026-09-25)
fff36cc7e119d870f689f8ed9d550c08ff27dbd9without rewriting the original six commits. Resolved the React import conflict and preserved main's compact quick-reaction wrappers and picker focus behavior.ProfileAgentIdentity.test.tsx: public metadata discovery now makes an exact-profile kind-10100 read in addition to kinds 0 and 30315. The unchanged test failed on main72ba8a21independently of this PR. The corrected assertion still rejects additional owner/policy reads; no profile production code changed.test-results/browser/; these are not product assets or visual baselines.Verification at
d61a4110reactions,reactions-polish,avatar-shapes, andmessagesbrowser files. Command:bin/pnpm test:browser tests/browser/reactions.spec.mjs tests/browser/reactions-polish.spec.mjs tests/browser/avatar-shapes.spec.mjs tests/browser/messages.spec.mjs --no-deps --workers=2.src/app/PluginImport.test.tsx:99: it expects “stays enabled and may run immediately” but the rendered copy says “stays on and may run immediately”. Both its test and implementation are unchanged from integrated main. This separate failure remains unresolved here; the full suite is not green.Test-layer and acceptance limits
The feature adds three browser cases for wrapping/picker geometry and real focus, controlled preview timing, and text containment. Existing browser files retain their full cases with selector/avatar assertions updated; no browser case was removed in this takeover. No new browser case or production behavior was added during integration. The profile-test repair has fail-on-clean-main / pass-on-integrated-head evidence. Touch-device, screen-reader, packaged-native and live-account acceptance were not exercised. Reaction ordering uses retained earliest events; reloads with a different retained history are not a cross-session ordering guarantee.