Add emoji reactions and polish picker and composer behavior - #27
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: 799511f7fe
ℹ️ 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".
| insertMention(recipient: Readonly<{ pubkey: string; name: string }>): boolean; | ||
| focus(): void; | ||
| }>; | ||
| export type ReactionToolProps = Readonly<{ |
There was a problem hiding this comment.
This commit's message has no Signed-off-by: trailer, so it violates the repository's DCO requirement and will fail the hosted DCO Check. Recreate the proposed commit with a sign-off from its actual author before integration.
AGENTS.md reference: AGENTS.md:L49-L59
Useful? React with 👍 / 👎.
| const references = event.tags.filter((tag) => tag[0] === "e"); | ||
| if (event.kind === 7) | ||
| return ( | ||
| [...event.content.trim()].length <= 64 && |
There was a problem hiding this comment.
When a same-origin caller submits a padded reaction such as 1,000 spaces followed by +, this check counts only the trimmed value, but the broker signs and publishes the original string. Such reactions can therefore be nearly 32 KB despite the intended 64-code-point bound, creating oversized reaction rows and bypassing the protocol limit; either reject surrounding whitespace or count the untrimmed content.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
| !session.channels | ||
| .list() | ||
| .channels.find((channel) => channel.id === row.channelId)?.archived |
There was a problem hiding this comment.
If the currently open channel becomes archived after its rows render, this value remains stale: MessageRow is memoized, does not subscribe to channels.subscribeList, and receives unchanged row/session/scope props when only channel metadata changes. The existing reaction button therefore remains enabled and can still call messages.react, which performs no archive check, contrary to the documented archived-channel behavior; derive this from a subscribed snapshot or pass the current archive state as a prop.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: two reaction-authoring defects
Reviewed head 799511f7fe29112ebe223c763571829ce135588a against base 09c6c04a49712732b97de2caa346a79c6eb2a750. Preserve the intended scope: reactions only beside existing reactions, optional Emoji tooling over the shared session/outbox, and unchanged ordinary message controls.
P2: Resolve reaction targets from retained thread evidence
At src/features/relay/messages.ts:79–82, the new command uses find, but its session callback only searches local operations and the bounded recent-event cache (session.ts:574–581). Open threads retain their verified root/replies independently and can continue rendering them after that cache evicts them. The existing visibility path already uses retainedThreadEvent; the command does not.
Reproduction by source trace: open a thread containing another author's message with an existing reaction, admit enough unrelated verified traffic to evict that message from the 4096-event/8 MiB recent cache, then select another reaction on its still-visible row. The command throws “Load the message before reacting to it” before creating any outbox operation. The retained-thread/cache-eviction state is already represented by threads.test.ts:342–361, but the new reaction path is not covered there. Resolve through session-owned, access-checked retained target evidence and add a reaction regression for that lifecycle; do not make the display cache lifetime an implicit write prerequisite.
P2: Allow valid custom shortcodes through both reaction validators
messages.ts:85–86 rejects content above 64 code points, while the existing catalog accepts 64-character shortcodes (emoji.ts:5–10). Emoji Mart wraps each accepted shortcode in two colons (emoji-mart.ts:95–100). A catalog entry with a 63- or 64-character name therefore appears as a selectable reaction but always fails with “Reaction is too long” because its literal is 65 or 66 characters. The broker independently imposes the same 64-code-point cap (dev/relay-broker.mjs:208–213), so changing only the frontend still fails signing/publication. Align both validators with the existing custom-emoji token contract and cover 62/63/64-character names through the picker-to-broker boundary while retaining a bound on arbitrary reaction text.
Scope and evidence
Source-only review on BLKD2G9MWNRWY.local, using explicit objects in /Users/wesb/.buzz/REPOS/buzz-app; checkout HEAD was unrelated (dbad6526346e7350c528280ccc5cf6c68e740607) and clean. No checkout, build, test, import, or execution of PR code. These are deterministic source traces, not claimed runtime reproductions. The PR author's reported validation is not my test evidence; hosted CI analysis was unavailable through the installed tooling.
Emoji selection/copy, GIF discovery recovery, and filesystem URL conversion received complementary review. Mixed custom/Unicode composer sizing is pre-existing, and whitespace selection is non-blocking polish; neither expands this fix. Earlier failed reactions retain global outbox retry, so replacing the row's latest delivery ID is not loss of durable recovery. Native packaged sign-in, live relay delivery, and cross-version external SDK guarantees are not validated by this source-only pass. Exit criteria are the two defects above plus focused regression coverage, not unrelated hardening.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: both prior reaction-authoring blockers remain
Reviewed head f997ddffbdc421a575998c3702ecf3232b8f5a1c against base 17f90c18fff6b86bc029e710401fb2b60bc385ea. This head merges current main into the previously reviewed 799511f7fe29112ebe223c763571829ce135588a; it does not repair the two previous findings.
P2: Resolve reaction targets from retained thread evidence
messages.ts:79–82 still resolves the target through a callback that searches only local outbox events and the bounded recent cache (session.ts:574–581). Open threads retain verified root/reply evidence independently; their visibility path already consults access-checked retainedThreadEvent, but the reaction command does not.
Source-trace reproduction: keep another author’s thread message with an existing reaction visible, then admit enough unrelated verified events to evict its target from the 4096-event/8 MiB recent cache. Selecting another reaction on the still-rendered row throws “Load the message before reacting to it” before any outbox operation exists. The retained-thread/cache-eviction lifecycle is already exercised for incoming overlays in threads.test.ts:342–361, not reaction authoring.
Exit criterion: resolve reactions from session-owned, access-checked retained target evidence and add a reaction regression for cache eviction while the thread remains visible. Do not couple write availability to an incidental display-cache lifetime or bypass recursive access checks.
P2: Accept valid custom shortcodes through both reaction validators
messages.ts:85–86 still caps the wrapped reaction at 64 code points. The catalog accepts names up to 64 characters, and the picker adds two surrounding colons.
Source-trace reproduction: load an otherwise valid catalog entry with a 63- or 64-character ASCII shortcode and a supported image URL, then choose it in the reaction picker. Its 65/66-character literal always fails with “Reaction is too long.” Fixing only the frontend is insufficient: validMessageTemplate independently caps kind-7 content at 64 on both signing and publication.
Exit criterion: align both validators with the existing custom-emoji token contract, retain a bound on arbitrary reaction text, and cover 62/63/64-character names through picker-to-session-to-broker handling.
Scope and validation
The product contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and host-owned session/authorization/outbox delivery. The command, catalog validation/directory, thread owner and broker files are byte-identical to the prior reviewed parent; the session’s only intervening change is an unrelated top-level-head classification check. Existing reaction tests are unchanged and do not close the two gaps above. The review stays on those established exit criteria and merge integration, not unrelated hardening.
Princess Donut’s independent merge-seam review found no concrete new regression. I verified the conflict resolutions and both message-row caller paths: Markdown/profile plumbing remains alongside reactions; the CSS retains both parents’ applicable rules; the thread test preserves count-independent large emoji through MessageMarkdown. This does not clear the unchanged reaction-authoring defects above.
Repository reads and source processing ran only over strict-host-verified SSH on BLKD2G9MWNRWY.local, addressing pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. Head tree da6b03697bb2593e3c62cd5ee664cd3ad321d289; base tree 605ae5fdfd605c1246b09a98c865a6b7732faa4d. The unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact head/base git diff --check passed. No checkout, import/execution of PR code, tests, builds, dependency installation, runtime probes or live-account writes were performed. These are source traces, not runtime reproductions. CI analysis is unavailable through the installed sq agent-tools interface, so this review makes no current-head CI-success claim. The PR body’s older validation is not current-head reviewer evidence. Live relay delivery, native/packaged acceptance and required human/code-owner approval remain separate gates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 673c574fca
ℹ️ 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".
| insertMention(recipient: Readonly<{ pubkey: string; name: string }>): boolean; | ||
| focus(): void; | ||
| }>; | ||
| export type ReactionToolProps = Readonly<{ |
There was a problem hiding this comment.
This file is explicitly marked FOUNDATION, but the commit expands its shared plugin contract with ReactionToolProps and reactionComponent without identifying the required human authorization. Do not integrate this contract change until explicit owner guidance is obtained and recorded, or restructure the feature to avoid modifying this protected boundary.
AGENTS.md reference: AGENTS.md:L12-L13
Useful? React with 👍 / 👎.
673c574 to
8914506
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89145062d5
ℹ️ 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".
| local().find((item) => item.event.id === id)?.event ?? | ||
| recent.peek(id)?.event, | ||
| recent.peek(id)?.event ?? | ||
| retainedThreadEvent(id), |
There was a problem hiding this comment.
When a visible channel-window row outlives the shared recent cache, this lookup returns no target because it searches only local operations, recent, and retained threads. Channel windows independently retain up to 2,400 rows / 8 MiB, while recent is a global 8 MiB LRU, so traffic in other channels can evict an event that is still rendered; its Add reaction button then opens normally, but selection fails with “Load the message before reacting to it.” Include active channel-window evidence in target resolution or pass the verified row/channel to the authoring path.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: resolve the new WebKit clipboard regression
Reviewed head 89145062d5f8681db79b3924943b9a779525f906 against base 834433c580d8e36a8ae9cd9da5695d1166604b94.
P2: The added emoji copy/paste case fails the required WebKit gate
Anchor: tests/browser/emoji.spec.mjs:81–97.
The newly added sequence drags across the rendered single custom emoji, presses Copy, focuses the composer, and presses Paste. On this exact head, hosted WebKit CI reaches line 97 and receives an empty draft instead of :party: for the full 10-second assertion window (72 other tests passed). This is a failure in the new regression coverage, not the previous head’s unrelated JavaScript failure. It also aborts the remaining assertions in this community-picker journey and fails CI required.
The retained trace/artifact shows the drag and keyboard operations completing, with the image visibly highlighted. It does not record the selected DOM range or the copy-event payload, so this evidence cannot yet distinguish an application copy defect from WebKit/input/clipboard test transport. Source inspection confirms the marked image, listener registration, and text/plain serialization are wired; I am not asserting a proven production-handler defect.
Exit criterion: isolate the selection → copy payload → paste boundary, repair the demonstrated app or test cause, and retain passing required Chromium/WebKit coverage of the shortcode-copy contract. A deterministic range plus an assertion on the copied payload can distinguish serialization from OS clipboard transport; keep real drag-selection coverage rather than silently replacing or excluding it. Do not hide this failure with a longer timeout or a new WebKit skip.
Established reaction blockers are resolved
The previous review’s retained-thread target lookup and 63/64-character shortcode bounds are fixed. Session/broker validation agrees, retained targets remain access-checked, and author-time tags/signed retry identity stay in the existing outbox. Timeline, thread root/replies, and the membership-row merge preserve the intended controls. Product contract remains reactions beside existing reactions, optional Emoji tooling, and host-owned delivery. No unrelated hardening is promoted into a new blocker.
This head has the same source tree (82f5946465408e91884adfe16322ba3a1828d624) as the fully source-reviewed predecessor 673c574; fresh exact-head CI evidence is why this review is not clear. Other hosted JavaScript, Rust/tool, measurement and browser shards passed. All independent review lanes returned before publication.
Source/diff and static CI-artifact inspection ran on strict-verified BLKD2G9MWNRWY.local against pinned Git objects. The unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact base/head git diff --check passed. No checkout, installation, build, test, import or execution of PR code, and no CI rerun. Hosted Linux WebKit evidence is not a native/macOS runtime reproduction. Live relay delivery and packaged UI acceptance remain outside this automated review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb5215f7ed
ℹ️ 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 tool = [...tools] | ||
| .sort((a, b) => a.key.localeCompare(b.key)) | ||
| .find((item) => item.reactionComponent); |
There was a problem hiding this comment.
When two installed conversation tools provide reactionComponent, this key-only sort selects whichever plugin key is lexicographically first, ignoring the ComposerTool.order contract that lower values appear first. As a result, a plugin cannot use its declared order to become the active reaction picker, even though the composer honors that same ordering; filter to reaction-capable tools and apply the established order/key comparator before selecting one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 024e58bc57
ℹ️ 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".
| recent.peek(id)?.event, | ||
| recent.peek(id)?.event ?? | ||
| retainedThreadEvent(id) ?? | ||
| channels.retainedEvent(id), |
There was a problem hiding this comment.
When the target survives only in a channel window after eviction from the shared recent and unread caches, this fallback lets messages.react enqueue and publish, but visibility()'s resolver at lines 141–145 still has no channels.retainedEvent fallback. Consequently local() drops the kind-7 operation before createChannelStore can fold it, and an echo or confirmation containing only the reaction is dropped for the same missing target, so the reaction never appears and the outbox never reaches seen, potentially prompting duplicate retries. Fresh evidence in this revision is that the new fallback here turns the former synchronous authoring failure into this downstream reconciliation failure; make retained-window target evidence available to visibility as well.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: retained-channel reconciliation and the unresolved WebKit clipboard regression
Reviewed head 024e58bc57fe4181a0ef5a9786c354a9916236f7 against base 834433c580d8e36a8ae9cd9da5695d1166604b94. Contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and host-owned session/access/outbox delivery.
P2: Use retained channel evidence in visibility, not only command lookup
The new fallback at session.ts:588–592 lets messages.react enqueue an event after its target leaves the shared cache, but the same target is still unavailable to the session’s visibility resolver. That resolver searches batch/local, recent, retained-thread and unread evidence, not channels.retainedEvent.
Source-trace reproduction: retain another author’s message with an existing reaction in channel C and no open thread, then admit unrelated traffic sufficient to evict it from the recent cache and exhaust unread evidence. The new regression’s nine 1 MiB unrelated messages already exercise the recent-cache case; its fixture has no roster-backed unread fallback. React to the still-visible target. Enqueue succeeds, but local() filters the new reaction out: eventVisibility requires every e target to resolve even when an h tag is present. The channel fold therefore never sees the local reaction. A verified echo or confirmation read containing only the reaction is filtered by the same resolver before writes.observe (session.ts:240–258), leaving an accepted operation unconfirmed and absent from the row until target evidence is reloaded elsewhere.
The new emoji.test.ts:179–201 checks only the raw outbox entry, so it passes without proving the visible/delivery contract.
Exit criterion: make session-owned retained channel evidence available to the recursive visibility path as well as authoring, without weakening access checks. Extend the regression through immediate row projection, signed echo/ID-read confirmation, pending-outbox removal, and access revocation. No second cache or delivery owner is needed.
P2: The deterministic selection change still fails the required WebKit copy/paste gate
At tests/browser/emoji.spec.mjs:96–116, the added drag assertion succeeds, then the test replaces the selection with Range.selectNode(image) and performs real Copy/Paste. Exact-head WebKit CI still receives an empty draft instead of :party: at line 116 for the full 10-second assertion window (72 other tests passed). This aborts the rest of the community-picker journey and fails CI required.
The drag assertion remains useful selection coverage. This failure now occurs after deterministic range installation, but it still does not prove an application copy-handler defect: the recorded evidence does not isolate copy dispatch, the text/plain payload, and clipboard/paste transport. The previous review’s exit criterion remains open.
Exit criterion: instrument the selection → copy payload → paste boundary, repair the demonstrated application or test cause, and retain passing required Chromium/WebKit shortcode-copy coverage. Keep real drag-selection coverage; do not weaken the expected shortcode, increase the timeout, or skip WebKit.
Scope and evidence
The archive subscription and deterministic provider-order changes have no additional blocker in this pass. Prior retained-thread lookup and 63/64-character shortcode fixes remain resolved. The new retained-channel lookup is access-checked and purged with its owning store; the defect above is its incomplete integration with visibility, not an access bypass.
Existing bot comments were reconciled: its retained-channel finding agrees with the independently traced defect above. The mount-local retry-ID observation does not remove durable recovery through the shared Outbox and is not a new blocker in this convergent pass. FOUNDATION authorization remains an owner/integration requirement, not evidence of a code defect; process metadata alone is non-blocking. The hosted DCO check is green.
Hosted JavaScript separately fails dev/agent-observer.test.mjs:69 (1 failed, 1,119 passed): https://github.com/block/buzz-app/actions/runs/34753201013/job/103713100811. That test, its decoder, package.json, and pnpm-lock.yaml are unchanged against this base. Its cause is not established here and is not an added emoji-repair blocker. Rust/tool integration, measurements, both Chromium shards and WebKit shard 2 passed. No CI reruns or polling.
Repository reads/diffs and static evidence processing ran only on strict-verified BLKD2G9MWNRWY.local, addressing pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. Head tree fd2e2b689bfee24d0e526693424d37850a5d46ae; the unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact base/head git diff --check passed. No checkout, installation, build, test, import or execution of PR code. The reaction finding is a source trace, not a claimed runtime reproduction. Native/macOS clipboard behavior, packaged acceptance and live relay delivery remain outside this automated review.
|
Addressed both current-head blockers in 6569d07.
Validation on 6569d07: focused emoji Vitest 12/12; Chromium + WebKit emoji journey 2/2; TypeScript, Biome, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6569d07cbd
ℹ️ 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".
| for (const child of node.childNodes) { | ||
| const block = | ||
| child instanceof Element && | ||
| /^(DIV|P|LI|OL|UL|SECTION|H[1-6])$/.test(child.tagName); |
There was a problem hiding this comment.
When a selection includes a rendered custom emoji inside a GFM table, this walker treats TABLE, TR, TH, and TD as inline elements, even though MessageMarkdown.tsx enables remarkGfm and renders tables. It therefore concatenates cells and rows (for example, :party:nextrow), and because copyEmoji prevents the default copy behavior, native clipboard serialization cannot restore the missing tabs or newlines; preserve table cell and row boundaries when constructing the replacement payload.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source review clear; required browser results remain an integration gate
Reviewed head 6569d07cbd6002e4d051d26b6027926055fdafac against base 834433c580d8e36a8ae9cd9da5695d1166604b94, focused on the three-file repair since the previous review. No actionable production defect found in that delta. This is COMMENTED, not approval or a claim that all required validation has passed.
Prior findings
- Retained-channel visibility is repaired.
session.ts:123–150now shares retained thread/channel evidence between command lookup and recursive visibility. Immediate local projection and reaction-only signed confirmation reach the same access-checked target beforewrites.observe. The channel callback checks current authorization; discovery applies access loss before projection, and retained windows/caches are purged. No second cache, authorization model or delivery owner was added. - Clipboard instrumentation now tests the actual app handler.
emoji.spec.mjs:81–163retains real drag selection, observes the native copy event after the app listener, and asserts exact single-emoji and historic-paragraphtext/plainpayloads. The observer neither writes the payload nor invokes the serializer directly. Native editable copy/paste is checked separately. No new engine exclusion or timeout inflation was introduced. Mongo’s independent source review agreed; Carl verified the handler and registration.
The clipboard change intentionally decomposes handler serialization and native editable transport. It does not establish a continuous rendered-image → platform clipboard → composer round-trip, nor independently establish the test comment’s general Linux WebKit explanation. The previous failing sequence was changed, but current-head passing browser evidence was not yet available in this review’s CI snapshot. Required Chromium/WebKit success remains an integration gate; do not interpret this source-clear verdict as proof that the prior runtime failure is resolved.
Non-blocking regression-test isolation gap
emoji.test.ts:180–215 adds useful immediate-row, signed-confirmation, empty-outbox and revocation assertions. However, granting viewer membership in C also lets unread state retain the target. The nine large messages in unrostered other do not exhaust unread evidence: unread.ts:392–423 ignores them. Thus the test does not isolate the newly fixed channel fallback. Grant membership in other before its overflow traffic, or preserve a separate rosterless case, so removing that fallback would fail the regression. This does not invalidate the source-traced production repair and is not a new blocker.
Scope and validation
The contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and host-owned session/access/outbox delivery. Earlier retained-thread and 63/64-character shortcode fixes are unchanged by this delta. Unrelated hardening and process metadata are not new exit criteria.
At the one-shot snapshot, exact-head GitHub Actions had successful JavaScript, Rust/tool integration and browser measurements; all four browser journey shards were still running. DCO, Semgrep and zizmor were also successful. No aggregate-success claim, CI polling or rerun.
Repository reads/diffs and static processing ran only over strict-verified SSH on BLKD2G9MWNRWY.local, using pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. Head tree 151d3245bb5571d339c1067cc9d18aa97ffe7da4; unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact base/head git diff --check passed. No checkout, install, build, test, import or execution of PR code. Live relay delivery, native/macOS clipboard behavior and packaged acceptance remain outside this source-only review.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: WebKit copy coverage and table-copy separators
Reviewed head acd4b5781e05a98eafd6a0b65c9c2f8286b979d5 against base 61274d40e0171cae4ef519021370345e18b884d3. This head merges main into the previously source-reviewed 6569d07cbd6002e4d051d26b6027926055fdafac. The contract remains reactions beside existing reactions, optional Emoji tooling, and host-owned session/access/outbox delivery.
P2: Make the native copy-payload regression pass on required WebKit
At tests/browser/emoji.spec.mjs:107–131, the new observer is installed, Copy is pressed, and the payload is read immediately. Exact-head WebKit CI fails at line 131: expected { prevented: true, text: ":party:" }, received undefined (74 other cases passed). This aborts the rest of the community-picker journey and fails CI required.
The retained trace confirms observer installation and keyboard completion, followed by an undefined payload read. Therefore the copy observer had not recorded an event by the assertion. This does not prove that the app serialized the wrong payload or that the platform clipboard discarded it. In particular, the test’s Linux WebKit explanation cannot substitute for observing the event in this run.
The previous review explicitly left required browser success open while the shards were running. This is new failing evidence for that established exit criterion, not a new production-handler allegation.
Exit criterion: isolate native copy dispatch/focus/timing from handler serialization and fix the demonstrated app or test cause. If dispatch is asynchronous, synchronize with the event using a bounded wait rather than an immediate read; if it never dispatches, establish why. Preserve real drag selection, exact shortcode/mixed-text payload assertions, and editable native transport coverage, with passing required Chromium/WebKit results. Do not manufacture the observer payload, loosen its expected text, or skip WebKit.
P2: Preserve cell and row boundaries when copying emoji-bearing tables
copy-emoji.ts:42–63 inserts separators for a small block-tag set, but not table rows/cells. The new document copy handler replaces custom-emoji images, serializes the entire selected fragment, then prevents native copy.
This affects supported GFM message content, not hypothetical HTML. MessageMarkdown enables GFM and inline emoji. Although mdast-to-hast initially adds formatting newlines around table elements, the lockfile-pinned JSX consumer hast-util-to-jsx-runtime@2.3.6 removes whitespace-string children of table/tbody/thead/tfoot/tr before rendering. Those newlines therefore do not rescue the copy walker.
Source-trace reproduction: send a table with a valid event-local party emoji:
| State | Count |
| --- | --- |
| :party: | 12 |
| done | 34 |Select the rendered table and copy. With ordinary single-line cells, the fragment walker emits StateCount:party:12done34: neither tabs nor row breaks separate the values. Because the handler calls preventDefault, native table serialization cannot restore the boundaries. Selecting a table without any marked custom emoji does not enter this handler. This is introduced by the PR's new copy interception and loses selected content structure, rather than merely changing its visual styling.
Exit criterion: retain cell and row delimiters when substituting shortcodes in selected tables, including selections spanning only part of a table. Add a regression through the actual Markdown rendering and copy handler; hand-built DOM retaining mdast formatting newlines would miss this consumer behavior. Keep the fix inside the existing serializer, without introducing another copy owner or broad markup redesign.
Mongo independently checked the copy path and, after tracing the final JSX consumer, confirmed the table-collapse finding. Both assigned review lanes are complete.
Merge integration and prior findings
The retained-thread/channel visibility and 63/64-character shortcode repairs remain unchanged. The merge preserves session/scope reaction gating, custom reaction rendering, and ReactionTool beside existing reactions while extracting the attachment branch to main’s AttachmentImage. Attachment code is identical to base; reaction ownership code is identical to the prior reviewed parent. Existing emoji tests and main’s attachment-access test coexist. Princess Donut’s independent merge review and my parent/blob/diff verification found no additional integration blocker.
The prior unread-backed retained-channel test isolation gap remains non-blocking. Durable global Outbox retry remains available, so the earlier mount-local retry-ID comment is not promoted to a new blocker. Process metadata is not a code defect; hosted DCO is successful.
Validation and limits
Run 34764137201 has successful JavaScript, Rust/tool integration, browser measurements, both Chromium shards, DCO, Semgrep and zizmor. A separate WebKit shard-2 failure is a navigation returned-to-top case reporting a /api/relay/primary/stream access-control console error (72 other cases passed). Its test and fixture are unchanged from base. Its cause is not established here; it remains an external integration gate, not an added emoji-repair finding.
Hosted CI checked synthetic merge a34ba9ef11792c4ec5fc796ea87e14a202af42de; I fetched it and verified its tree is identical to the claimed head (5aa8be46a1b6354fd3c89f314202836a03789166).
Source/diff and static CI-artifact inspection only on strict-verified Studio BLKD2G9MWNRWY.local, using pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. The unrelated checkout remained clean; exact base/head git diff --check and git show --remerge-diff were clean. No checkout, install, build, test, import/execution of PR code, runtime probe, or CI rerun. Author-reported local validation at 6569d07 is not reviewer-run validation of this head. Native/macOS clipboard behavior, live relay delivery and packaged acceptance remain outside this review.
|
Carl, an automated contributor, commenting via Wes’s GitHub account. Main update deliveredHead The combined integration guard rejected this branch’s additional reaction fixture, which already owned a private cache but was not one of main’s grandfathered direct-Vite consumers. The follow-up moves that fixture onto the shared cache-owning wrapper and removes duplicate allocation/cleanup. The guard and journey assertions remain unchanged. Validation on Apple Silicon macOS
Still not merge-readyThe existing review identifies table-copy separator loss and the required Linux WebKit copy-event failure. The table serializer and emoji journey are unchanged by this update; local passing copy results do not establish Linux recovery. The prior canceled-stream diagnostic is also unchanged and requires current-head hosted evidence. Existing review approval requirements remain; no PR merge or approval was performed. New commits have verified author/sign-off trailers; prior history was not rewritten. Hosted CI/DCO status is not verified in this update: the installed |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: the two clipboard findings remain unresolved
Reviewed head c1eee2cf71e6cc8693df09cdc2ab3b102a97c028 against base d8fa1a2dcaa8ea87cbbd7cdc1652b1e277dd89a4, concentrating on the increment since the previous review. The contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and session-owned authorization/signing/outbox delivery. No new exit criteria are being added.
P2: Required WebKit native-copy regression still fails
tests/browser/emoji.spec.mjs:107–131 is unchanged. After observer installation and keyboard Copy, it immediately reads the payload. Current-head WebKit CI again fails at line 131: expected { prevented: true, text: ":party:" }, received undefined. The retained error context and trace confirm that sequence. This aborts the remainder of the community-picker journey and fails CI required.
This establishes that the observer had not recorded a copy event by the assertion. It does not establish a wrong application payload or a platform clipboard discarding it. The test’s Linux WebKit explanation still does not explain the observed missing event.
Unchanged exit criterion: isolate native copy dispatch/focus/timing and repair the demonstrated app or fixture/assertion cause. If asynchronous, use bounded event synchronization rather than an immediate snapshot; if dispatch never occurs, establish why. Preserve real drag-selection coverage, exact shortcode and mixed-text payload assertions, and editable native transport coverage with passing required Chromium/WebKit results. Do not manufacture the observer payload, weaken expected text, inflate timeouts, or skip WebKit.
P2: Copying emoji-bearing GFM tables still collapses cells and rows
copy-emoji.ts:42–63 is also unchanged. Its separator set omits table rows/cells, while copyEmoji serializes the entire selected fragment and prevents native copy whenever a marked custom emoji is present.
Source-trace reproduction: send this supported GFM table with a valid event-local party emoji, select the rendered table, and copy:
| State | Count |
| --- | --- |
| :party: | 12 |
| done | 34 |The walker emits StateCount:party:12done34, losing cell and row boundaries. This is supported message content: MessageMarkdown:320–334 enables GFM and inline emoji. The unchanged lockfile pins hast-util-to-jsx-runtime@2.3.6; its final JSX consumer removes whitespace-only table children, so intermediate Markdown formatting newlines do not rescue the serializer. Native serialization cannot restore the boundaries after preventDefault().
Unchanged exit criterion: preserve cell/row delimiters, including partial table selections, within the existing serializer. Add a regression through actual Markdown rendering and the copy handler, not hand-built DOM retaining formatting whitespace. No second copy owner or broad markup redesign is needed.
Incremental integration and validation
Princess Donut’s independent fixture/merge lane returned no findings; I verified its diff and lifecycle claims. The new reaction fixture wrapper preserves every reaction/publication/archive/error assertion and owns an isolated optimizer cache with cleanup on close and creation failure. Its integration tests cover concurrent isolation and retirement. Main’s GIF/typeahead lifecycle changes remain present. Reaction-owned production paths, retained-thread/channel visibility, shortcode bounds, and the clipboard files are object-identical to the previously reviewed head. Earlier non-blocking test-isolation and process observations are not additional blockers.
Run 34771574238 passed JavaScript, Rust/tool integration, browser measurements, both Chromium shards and WebKit shard 2; WebKit shard 1 and the required aggregate failed. DCO, Semgrep and zizmor passed. CI’s clean synthetic merge 1c6bcf23dc21dddc84b4c31a3eede6f961c2e2de has the same tree as the claimed head: 51a5fdf081fd7e02653df4d6f16f6143398850af. No CI rerun or polling.
Repository reads/diffs and static artifact inspection ran only over strict-verified SSH on Studio BLKD2G9MWNRWY.local, addressing pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. The unrelated checkout stayed clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact base/head git diff --check passed. No checkout, install, build, test, import, runtime probe, or execution of PR code. Table behavior is source-traced, not a reviewer-run reproduction. Native/macOS clipboard behavior, packaged acceptance and live relay delivery remain outside this automated review.
|
Addressed both current-head clipboard blockers in
Validation on the tree committed as |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f40f5d7f5
ℹ️ 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: preserve empty table cells when copying
Reviewed head 1f40f5d7f56a2cf0bc778dec9466a71be2f524c7 against base d8fa1a2dcaa8ea87cbbd7cdc1652b1e277dd89a4, focused on the three-file repair since the previous review. Contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and session-owned access/signing/outbox delivery.
P2: Empty cells still collapse the copied table's column positions
The new separators fix the previous all-nonempty example, but copy-emoji.ts:48–60 emits a delimiter only when both accumulated text and the next cell's value are nonempty. An empty selected cell returns "", so its boundary disappears. Adjacent delimiter deduplication also cannot represent consecutive empty columns correctly.
Source-trace reproduction: render this supported GFM message with a valid event-local party emoji, then select and copy the table:
| State | Owner | Count |
| --- | --- | --- |
| :party: | | 12 |Expected plain text is State\tOwner\tCount\n:party:\t\t12; the current walker produces State\tOwner\tCount\n:party:\t12. Pasting it into a spreadsheet places 12 under Owner instead of Count. Leading/trailing empty cells are similarly lost. The handler prevents native copy for the selected fragment, so the browser cannot restore those boundaries.
This is the existing table-preservation exit criterion left incomplete, not a new rich-Markdown feature request. The added fixture and whole-table/partial-row assertions contain only nonempty cells and therefore miss it.
Exit criterion: preserve one column boundary per adjacent selected cell, including empty cells, independently of whether that cell contributed text. Keep this inside the existing serializer and retain ordinary block behavior. Extend actual MessageMarkdown → copy-handler coverage with leading, interior and trailing empty cells and a partial selection. No new copy owner or broad formatting redesign is needed.
Native-copy synchronization and remaining validation
The two immediate payload snapshots now use bounded expect.poll after keyboard Copy. Observer installation still precedes the trigger, the observer reads rather than manufactures the application's payload, and real drag selection, exact single/mixed-text payloads, editable native transport, both engines, and configured timeouts remain intact. That is the requested source-level synchronization repair. Current-head required Linux Chromium/WebKit results are still needed; this review does not claim that the previous hosted dispatch failure reproduced or recovered at this head.
At the one-shot metadata snapshot, GitHub reported merge conflicts and no Actions run for this exact head; Semgrep, zizmor and DCO passed. The author's reported focused two-engine pass and hook results are attributed validation, not reviewer-run or hosted Linux evidence. Merge conflicts and unavailable required results remain integration gates, not additional production defects.
Review scope and evidence
The increment is one commit changing only the serializer, emoji browser journey and fixture. Session/reaction commands, retained evidence and visibility, broker, message rendering/callers, plugin copy-listener registration and lockfile are object-identical to the previous reviewed head. Earlier reaction-authoring repairs and non-blocking observations are not reopened. The fresh bot suggestion about blockquote/PRE separators concerns unchanged behavior outside this table repair and is not adopted as another verified blocker. I completed a fresh self-review; the delegated lane was explicitly cancelled before publication, so no independent confirmation is claimed for this increment.
Source/metadata-only inspection ran on strict-verified Studio BLKD2G9MWNRWY.local, addressing pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. The unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact base/head git diff --check passed. No checkout, installation, build, test, import, runtime probe, native launch, or execution of PR code; no CI rerun or monitoring. The table failure is a source trace, not a reviewer-run reproduction. Live relay delivery, native/macOS clipboard behavior and packaged acceptance remain outside this automated review.
1f40f5d to
2975ad3
Compare
|
Addressed Wes’s current-head review and the newer inline copy finding in rebased head
Validation on exact clean head
No product behavior or timeout was weakened. Fresh hosted CI has started; I did not wait on or poll it. |
There was a problem hiding this comment.
💡 Codex Review
buzz-app/src/bundled/emoji/EmojiPicker.tsx
Lines 84 to 86 in 2975ad3
Fit the reaction picker within narrow viewports
When the reaction picker opens on a phone-sized viewport, this computes columns from the full document width without accounting for the popup's 32px collision padding or its 74px of picker chrome. At 320px it selects five columns and requests a 314px picker, while .reactionPopover caps it at 288px and .emojiPopover hides overflow, clipping part of the emoji grid and search UI. Calculate perLine from the popup's actual available width and PICKER_CHROME so all selected columns remain usable.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bce5a49d70
ℹ️ 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".
| order?: number; | ||
| component: ComponentType<ComposerToolProps>; | ||
| /** Optional emoji-only chooser for the message reaction row. */ | ||
| reactionComponent?: ComponentType<ReactionToolProps>; |
There was a problem hiding this comment.
When an independently built or plain-JavaScript plugin supplies a truthy non-function reactionComponent, ConversationService.validate still accepts the tool because it validates only component. selectReactionTool then chooses that malformed provider, React falls into “Reactions unavailable,” and a valid lower-priority provider is shadowed. Extend the existing registration-time validation to reject non-callable optional reaction components.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: previous clipboard blocker resolved
Reviewed head bce5a49d706d7e9a2488d70ec79a0c36d703076e against base 1ba1544f833082bccb604f8d6a9b40d6d4caacc8. No remaining actionable code blocker in this convergent re-review. This is a comment, not approval or permission to merge.
The contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and session-owned access/signing/outbox delivery. I reconciled the rebase against the previously reviewed series, then reviewed the new clipboard and persisted-retry commits and the macOS caret-shortcut conflict adjustment. Earlier unchanged, nonblocking observations were not promoted into new exit criteria.
- Clipboard exit criterion satisfied.
copy-emoji.ts:43–80now emits table-cell boundaries independently of cell text and newline deduplication. Leading, interior and trailing empty cells retain their column positions. Actual MessageRow/MessageMarkdown fixtures and keyboard-copy assertions cover the whole table, a partial row and the added quote/code boundary. Native editable-copy bypass and observer-before-trigger/bounded-poll synchronization remain intact. This resolves the previous blocker at source level. - Retry state now follows the outbox.
ReactionTool.tsx:111–149derives failed/unknown operations from the current session outbox, filters kind 7 by target, and retries the original event ID. It no longer depends on a mount-local sent ID. Existing relay/viewer storage partitioning, restoration and session identity remain the owners. The new browser journey forces a real row remount; it is not itself an application-restart/disk-recovery test. Independent clipboard and retry lanes returned, and I verified their material conclusions. - Narrow-picker bot comment not adopted as a blocker. Its 320px example treats all requested width as indispensable chrome. The actual locked Emoji Mart 5.6.0 layout has five 48px buttons plus 24px scroll padding inside the capped popup; the injected 100%-width rules allow that example to fit. Requested-width mismatch alone does not prove the claimed clipping. Native windows also have a 480px minimum. This is not a runtime certification of every smaller web breakpoint.
Validation and limits
Studio-only source/metadata inspection on BLKD2G9MWNRWY.local, pinned Git objects in /Users/wesb/.buzz/REPOS/buzz-app. The unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; exact base/head git diff --check passed. No checkout, install, build, test, import, runtime probe or PR-code execution.
The existing current-head CI run had JavaScript, Windows native notifications and security/DCO checks green at the review snapshots; Rust and browser journey shards were still running. No CI rerun or monitoring. Required hosted gates, including Chromium/WebKit confirmation of the clipboard synchronization repair, remain integration requirements. Author-reported focused passes are not reviewer-run evidence. Live relay delivery, native clipboard and packaged-app acceptance were not exercised.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
bce5a49 to
2df11fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df11fdc51
ℹ️ 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".
| {canReact && extensions && session && scope && ( | ||
| <ReactionTool |
There was a problem hiding this comment.
When a message's only confirmed reaction is deleted while a newly submitted reaction is in the failed state, folding excludes the failed auxiliary event and leaves row.reactions empty. Because ReactionTool—including its persisted ReactionDelivery retry notice—is mounted only inside the existing-reactions block, the failure and “Retry reaction” action disappear, including after a reload. Render the delivery status independently of this gate while keeping the add-reaction button hidden for reaction-free messages.
AGENTS.md reference: AGENTS.md:L35-L37
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: rebase integration
Reviewed head 2df11fdc51783e918f2bff0e7ed4caf8431a291f against base 57796f034f290fbb5d7ee4fc41807f5c784591ff. No new actionable blocker found in this convergent re-review. This is a comment, not approval or permission to merge.
The contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and session-owned access, signing and outbox delivery. This review carries forward the previous clear review, not a fresh runtime certification of the entire feature.
- Series reconciled.
git range-diffmatches the twelve commits to the previously cleared series. The two non-identical range-diff entries contain fixture context changes from the new base. The nine changed paths between old and new heads are the same nine paths changed between their bases; seven have identical new-head/new-base blobs. The two overlap files are the Emoji fixture and browser journey. Previously resolved clipboard and outbox-retry behavior is unchanged. - Fixture integration preserved. The catalog query now awaits an explicit gate; the retry journey observes loading and the Unicode-only picker before releasing in
finally, then requires the restored custom catalog. Reaction rejection, archive transitions and remount controls remain independent. The reaction browser test has the same blob as the cleared head. An independent fixture lane returned clear, and I verified its material conclusions. See fixture and retry journey. - Inherited scrolling change kept in scope. The Virtua patch is already in the base. The timeline retains its existing virtualizer, focus retention and row-measurement ownership; the PR adds session/scope inputs to reaction rows rather than another scroll mechanism. I found no source-level integration blocker. The inherited Mac WebKit momentum-braking tradeoff and native acceptance limits in
patches/README.mdare not newly certified here.
Validation and limits
Source/metadata-only review on strict-verified BLKD2G9MWNRWY.local, using pinned Git objects in /Users/wesb/.buzz/REPOS/buzz-app. Exact-base/head git diff --check passed. The unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607; no checkout, install, build, test, import or PR-code execution occurred.
Current CI analysis was unavailable because installed sq agent-tools 0.7.13 does not expose ci-results; I make no current-head CI-pass claim. Required hosted gates remain integration requirements. No CI rerun or monitoring. Browser/native clipboard, live relay delivery, native momentum with these reaction rows, and packaged-app acceptance were not exercised.
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Consolidated investigation with Princess Donut, at Wes’s request. Current failure and recommended repair boundaryAt head Failed job: 84 passed, 1 failed. Why main passes: this PR adds the clipboard handler and the failing test block; main First candidate to validate: WebKit’s editing modifierThe installed, pinned Playwright 1.60.0 implementation exposes a likely platform mismatch:
This makes an engine-specific editing modifier the strongest source-derived candidate. It is not yet a verified Linux fix: we inspected logs, trace and source, but did not execute a reproduction. The adapter map alone does not prove what the native Linux port ultimately consumes. // Add browserName to this test's fixtures.
const editingModifier = browserName === "webkit" ? "Meta" : "ControlOrMeta";
await observeCopyPayload();
await page.keyboard.press(`${editingModifier}+c`);
await expect.poll(copyPayload).toEqual({
prevented: true,
text: ":party:",
});Validate this on Linux WebKit first, with the actual selected image and installed observer: compare Control+C versus Meta+C and verify the exact native payload. Also check editable select-all/copy/paste. If confirmed, use the same modifier for all A/C/V operations in this clipboard section, not just the first failing Copy. No production serializer change is justified by this failure so far. If that comparison does not produce the event, capture Exit criteriaKeep the exact payload assertions and genuine editable clipboard checks. Do not increase timeouts, skip WebKit, manufacture a synthetic copy event, or preload the expected clipboard value to make this green. Exercise the complete emoji test file in Chromium and WebKit, including Linux WebKit, then require the normal PR CI run: bin/pnpm exec playwright test --config tests/browser/playwright.ci.config.mjs \
tests/browser/emoji.spec.mjs --project=chromium --project=webkit --no-depsA macOS WebKit pass does not establish that the Linux shortcut path works. This comment is diagnosis and a bounded repair/verification plan, not a claim that the candidate patch has passed. |
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e06de057d
ℹ️ 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 channelList = useSyncExternalStore( | ||
| session?.channels.subscribeList ?? emptySubscribe, | ||
| session?.channels.list ?? emptyChannelList, | ||
| session?.channels.list ?? emptyChannelList, | ||
| ); |
There was a problem hiding this comment.
In long threads, ThreadPanel mounts potentially thousands of MessageRow instances, so this creates one subscription to the global channel list per message. Any roster or metadata change—including one for an unrelated channel—then schedules every memoized row to render, turning a single list update into O(thread-size) React work. Subscribe once in the timeline/thread owner and pass the current channel's archived state as a prop so unchanged rows retain memoization.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: clipboard-test correction
Reviewed head 1e06de057d508d4dfa9743d1b4b898097c8181d6 against base 57796f034f290fbb5d7ee4fc41807f5c784591ff. No new actionable blocker found in this convergent re-review. This is a comment, not approval or permission to merge.
The contract remains reactions beside existing reactions, optional Emoji tooling, unchanged ordinary controls, and session-owned access, signing and outbox delivery. This carries forward the previous clear review, not a fresh runtime certification of the entire feature.
- Only the browser test changed. Two commits modify
tests/browser/emoji.spec.mjs(+24/−6); every other tracked file is object-identical to the previous clear head2df11fdc51783e918f2bff0e7ed4caf8431a291f. Production clipboard serialization, retained-target/access repairs, signed retry identity, picker/catalog behavior and the real fixture are unchanged. - The copy-event assertions remain substantive. The revised helper resets and installs its one-shot observer before triggering each copy. WebKit invokes the browser’s
execCommand("copy"), requires success, and then polls for the exact handler payload withdefaultPreventedandisTrustedboth true. Chromium retains keyboard copy. The five shortcode/history/table/quote cases preserve their exact expected text, including empty columns and line breaks. The diff does not manufacture a ClipboardEvent or pre-seed the expected payload. A missing or wrong handler still fails these assertions.
Coverage boundary and non-blocking observations
WebKit’s command-driven path does not prove its keyboard shortcut dispatch for non-editable content. isTrusted identifies a browser-generated event, not a physical keystroke. The unchanged textarea copy/paste controls exercise native editable transport separately; they do not establish one continuous rendered-image → system clipboard → composer round trip. The test comment’s broad Linux WebKit explanation is not independently established by this source review. Native/platform clipboard acceptance remains separate.
I reconciled the bot comments without adding unrelated exit criteria. Row-level delivery feedback can disappear if the last displayed reaction disappears, but the unchanged global Outbox still exposes failed/unknown operations and retries the same event (OutboxStatus.tsx:32–49); improving inline discoverability remains non-blocking. Optional reactionComponent validation and per-row channel-list subscription fan-out remain follow-up hardening/performance work, not verified new regressions in this test-only increment. FOUNDATION authorization is an owner/integration requirement, not evidence of a code defect; current DCO is green.
Validation
Source/metadata only on strict-verified BLKD2G9MWNRWY.local, pinned objects in /Users/wesb/.buzz/REPOS/buzz-app. Exact-base/head git diff --check passed; the unrelated checkout remained clean at dbad6526346e7350c528280ccc5cf6c68e740607. No checkout, imports, dependency installation, tests, builds or PR-code execution. An independent clipboard lane completed before publication and was integrated with the source trace above.
All ten current-head hosted checks report success, including both Chromium shards, both WebKit shards, browser measurements and CI required: run 34857498736. This is existing hosted evidence, not a reviewer-run suite, repeated-flake certification, live relay reaction delivery or packaged/native acceptance. No CI polling or reruns.
* origin/main: Add emoji reactions and polish picker and composer behavior (#27) Signed-off-by: Clay Delk <clay.delk@gmail.com> # Conflicts: # src/features/messages/MessageRow.tsx # src/features/messages/Messages.module.css # src/features/messages/ThreadPanel.tsx
Messages with existing reactions now offer a Lucide smile-plus button that opens an emoji-only picker and publishes through the existing outbox, including retry feedback. Messages without reactions keep their current controls.
Emoji-only messages stay large regardless of count. Custom emoji can be selected and copied as complete shortcodes; autocomplete inserts no extra space and leaves a normal-sized cursor. The composer reveals GIFs when relay support finishes loading, and search padding/radii follow whether tabs are present.
A separate tooling commit fixes URL-to-filesystem conversion in the design checks so validation works in workspace paths containing spaces.
GIF availability also recovers when a relay enables GIFs during an existing session: reopening retries unsupported results, and the broker retains only confirmed support.
Validation: full
bin/just scanpassed at48835ce: 34 Node integration tests, 930 JavaScript unit tests, plugin tests, 234 Chromium/WebKit app journeys, 14 design-viewer journeys, formatting/type/design checks, frontend builds, Rust Clippy, and native test commands. The follow-up at799511fpassed typecheck, 10 broker integration tests and six Chromium/WebKit GIF journeys, including unsupported-to-supported recovery. The running app's broker now reports live GIF support and a live search returned 24 results. Commit hooks passed. The full scan has not been repeated after this focused follow-up; live relay reaction delivery and native UI acceptance remain separate from fixture coverage.