Preserve rich content across client edits - #165
Conversation
Signed-off-by: Zach Marley <zmarley@squareup.com>
|
🤖 Correction: the images previously attached here were hand-built HTML/CSS mockups, not screenshots of the actual application or its component fixtures at the cited commit. The previous fixture/head evidence claim was incorrect. Those mockups do not satisfy the PR screenshot requirement and have been removed from this comment. Genuine isolated app/component-fixture screenshots are still required before readiness. No live user data was used. |
|
🤖 Genuine replacement capture at b4f444b. Actual MessageRow and AttachmentImage rendered from foldMessages(original kind 9 plus authorized kind 40003 edit). Receive/render illustration only: no edit UI or outgoing broker capability is shown or claimed. Mounted real PR components/styles in an isolated fixture with synthetic data and generic fictional profiles; no hand-built replacement UI. I visually inspected this image: no real identities, conversations, keys, credentials, private URLs or local paths are displayed. Inline synthetic artwork only; no live app/media capture. No production edits or commits. This replaces the invalid mockup previously posted. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No actionable defect found in the bounded receive/fold change at b4f444b7179ce24583aad7c9de6243693fc0ba89.
Reviewed the four-file PR delta from merge-base 60ed428b016ea67cfadbff03c56b812990e170a9, with guidance and relevant integration checked against pinned base 5677876408ad1e9920c64f3ab7f583bcf1e8e6a4. The latest authorized edit’s imeta supplies attachment metadata; an edit without it preserves original imeta. Markdown attachments remain part of the replacement body. Shared receive/render paths and the service-level edit helper were traced, with an independent compatibility pass.
Existing exact-head CI and DCO passed; Windows native validation was skipped. This review was source-only, with no builds, tests, or live workflow executed. Actual legacy-client wire compatibility and integrated producer-to-consumer, thread/40002, multi-edit and incremental-refold attachment coverage remain validation gaps; synthetic fold fixtures do not establish them. Outgoing edit UI/broker capability, spoiler reveal, and kind 40008 rendering remain explicit non-goals.
GitHub currently reports merge conflicts; resolve against the target branch and validate the resulting head. This comment is not an approval or a mergeability certification.
…-content-compat * origin/main: (38 commits) Fix diff content fallback, keyboard scrolling and edit selection (#205) Standardize form controls and field feedback across Buzz (#174) Keep image review downloads and external opens distinct (#144) Verify media review comments (#166) Follow system appearance (#210) Add rich composer formatting and spoiler rendering (#203) feat: show roster-backed channels and managed instances in profiles (#188) Add new direct message flow (#156) Remove Home, start in Messages, and keep Channels enabled (#194) fix: restore avatar presence controls and active-input sensing (#198) Add legacy diff messages with inline and expanded viewing (#202) Edit the latest own message with Up in the existing composer (#192) Add complete reaction toggles to the message menu (#185) feat: add persistent community navigation rail (#191) test: add margin to warm-switch performance gate (#195) Add composer attachments and compatible media preparation (#183) Add reply and copying to the shared message menu (#182) fix: avoid idle workspace re-renders from activity and label churn (#186) feat: add devtools trace capture to web profiling (#180) Add optional channel templates, teams and personal group defaults (#181) ... Signed-off-by: Zach Marley <zmarley@squareup.com> # Conflicts: # docs/channels.md
Signed-off-by: Zach Marley <zmarley@squareup.com>
Signed-off-by: Zach Marley <zmarley@squareup.com>
Signed-off-by: Zach Marley <zmarley@squareup.com>
…-content-compat * origin/main: Add Messages design gallery and tighten message layout (#158) Signed-off-by: Zach Marley <zmarley@squareup.com> # Conflicts: # src/features/direct-messages/NewMessage.recovery.test.tsx
|
🤖 Fresh actual-component illustration at 56dcae7: real MessageRow/AttachmentImage rendered from foldMessages(original kind 9 plus authorized kind 40003 edit). Isolated Vite configFile:false/envFile:false, fresh Chromium context, synthetic caption/artwork and generic Sender profile; external requests blocked. Visually privacy-inspected. This is receive/render illustration, not a new live edit/publication trace. Prior user manual receive-direction pass remains historical. Exact-head CI/DCO green and refreshed-diff review PASS. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 attachment-state round-trip defect, detailed inline. Fix both receive fallback and outgoing re-edit metadata, with a multi-edit regression test.
- Reviewed head
56dcae7c5358d53c77d1a97272474a27891af6a6against merge-basee02fe33220fa497a2e7ee780c8295517b4f967fa; also inspected integration with current mainf6caa83f724253ca11fb83481e7b60f091b4cf7a. The synthetic merge is clean and preserves main’sMessageComposer,RichComposerInput, anduseMessageEditunchanged. No separate regression found in rich serialization, mentions, paste/send, spoiler/diff rendering, or DM recovery within the reviewed changes. - Existing hosted CI is green for this head; Windows native validation was skipped. This review was source-only, with independent composer and fold review lanes. No local tests, live cross-client workflow, or runtime validation of the synthetic latest-main merge was performed. The added tests exercise single edits, not the failing re-edit sequence.
- Non-blocking: the PR description still describes edit UI/broker support, spoiler reveal, and diff admission as unavailable, although they exist in this head. Please refresh that narrative; it is not the reason for requesting changes.
| tags: [ | ||
| ["h", channelId], | ||
| ["e", messageId], | ||
| ...original.tags.filter((tag) => tag[0] === "imeta"), |
There was a problem hiding this comment.
[P2] Preserve the current attachment state when re-editing
After an authorized cross-client edit replaces attachment A with B, this PR correctly displays B. But changing only the caption in buzz-app publishes A’s original imeta here: session.ts:1136–1142 supplies the immutable original event to find, while useMessageEdit loads the latest folded sourceContent. The next fold therefore restores A. If the body retains , parseAttachments also extracts B, so the message now displays both the obsolete A and a metadata-stripped B. This is reachable through the existing ArrowUp edit UI, including thread/media-review composers; adding attachments in this UI is not required.
There is a second entry into the same state-loss defect at fold.ts:257–260: original A → authorized edit with B imeta → later text-only edit without imeta falls straight back to A instead of retaining B.
Use the current authorized attachment state consistently for receive fallback and outgoing text edits; copying the original tags is only correct before any attachment-bearing edit. Add a three-event regression covering both a later no-imeta edit and a real session/composer re-edit, asserting the final folded attachments as well as published tags. An incremental arrival should preserve the same result. This does not require changing the pre-existing ambiguity around explicitly removing all attachments.
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Approved at 3cf27245c1d6d763a357dd0f32b8c22e9c992f54 against base e02fe33220fa497a2e7ee780c8295517b4f967fa. The prior P2 attachment-state round-trip blocker is resolved; no remaining blocker in the reviewed changes.
- Fold now selects the newest surviving authorized attachment-bearing edit; composer saves pass current folded provenance, and outgoing edits reject unavailable or mismatched source evidence instead of restoring original attachments. I implemented this fix; Mongo independently reviewed the changed contracts without finding a blocker. The final amend only adds the required media-viewer test prop.
- Full Vitest on the patched working tree before that fixture typing correction: 268 files / 2,800 tests passed. Restoring the original production implementation made six new regression assertions fail. Final-head push hooks passed TypeScript, design checks and 89 files / 1,386 related tests. Hosted JavaScript, browser measurements, security and DCO checks have passed.
- Remaining merge gate: hosted Rust and browser journeys were still running at the latest check; Windows validation was skipped. No local browser/native or live cross-client validation. Retained attachment-source lookup after actual shared-cache eviction was source-reviewed, not integration-tested. These limits do not reopen the resolved code blocker; merge only once required checks pass.


Summary
imetawhen present, while preserving original attachments for historical/text-only edits that carry noimeta.Latest head update
56dcae7c5358d53c77d1a97272474a27891af6a6.mainfixes.onOpenLinkwiring, incidental space-query setup, and Escape handling. It does not weaken assertions.56dcae7c5358d53c77d1a97272474a27891af6a6: https://github.com/block/buzz-app/actions/runs/36026504636Validation
bin/pnpm exec tsc --noEmit --pretty falsebin/pnpm exec vitest run src/features/relay/cross-client.test.ts src/features/relay/fold.test.ts src/features/relay/outbox.test.ts src/features/relay/traffic.integration.test.ts(4 files, 121 tests)bin/pnpm exec biome check src/features/relay/cross-client.test.ts src/features/relay/fold.ts src/features/relay/messages.ts docs/channels.mdgit diff --check1b21453f: passed.56dcae7c5358d53c77d1a97272474a27891af6a6: TypeScript + related unit tests (80 files, 1256 tests) and design-system guards passed.56dcae7c5358d53c77d1a97272474a27891af6a6.56dcae7c5358d53c77d1a97272474a27891af6a6.Review notes
56dcae7c5358d53c77d1a97272474a27891af6a6is linked in Preserve rich content across client edits #165 (comment). Scope: real MessageRow/AttachmentImage render from foldMessages receive/fold behavior only; it is not edit UI, outgoing broker, or send proof. The illustration uses synthetic caption/artwork and a generic profile, with external requests blocked, and was privacy-inspected. Historical earlier-head capture remains in Preserve rich content across client edits #165 (comment).Linear: BOT-1939