feat(github): render PR descriptions with inline media - #335
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review through Wes’s account.
Reviewed head 0668dd5d5ccb122317999cb8bdc45a9f9b9cbce4 against base/merge-base 85d6bf82c54d1c8d930d58444597a1fe31cc8975.
One actionable P2 finding is inline: the HTML-image transform silently discards non-image description content and attachment links. Preserve that content without enabling arbitrary HTML execution.
Scope: all 11 changed files, GitHub panel fetching/navigation, Markdown/media classification, existing image/video/audio consumers and fullscreen lifecycle, CSP, and added test sources. Thirty pinned app source/document blobs and two vision-document blobs were hash-verified; no dirty checkout inputs were used. This was source-only: I did not execute PR code, tests, builds, installs, the app, or live media requests.
One exact-head CI snapshot showed JavaScript, Rust/tool integration, browser measurements, DCO and security checks passing; browser journeys were incomplete, with WebKit shard 2/3 failed. Its existing log reports notification-settings.spec.mjs:26 timing out on the Appearance button; the added GitHub-media journey passed in that job. I have not established the Notifications failure as baseline or flaky. Native/packaged behavior, real GitHub media redirects/access, and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One additional image-description finding inline. The existing HTML-content-loss finding remains applicable and is not duplicated.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review through Wes’s account.
Reviewed head 10634c0dcd28746db2b86caea0a8025b10ee0db8 against base/merge-base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. This follow-up checks the prior finding and the changes since 0668dd5d5ccb122317999cb8bdc45a9f9b9cbce4, with the complete current diff and relevant callers for context.
No actionable findings in this source-only follow-up. The prior HTML-content-loss finding is addressed in source: the replacement transform retains prose, summaries, safe attachment links and media fallbacks instead of extracting only images. Added regression sources cover HTML-only and mixed content, linked media, unsafe destinations and over-depth literal fallback. The Notifications browser setup now follows the existing profile-menu Settings path without removing its layout assertions.
Scope: all 12 changed files, GitHub panel fetch/navigation, Markdown and attachment handling, shared media consumers/lifecycle, CSP and test sources. Verified 29 pinned app source/document blobs and two pinned vision documents; no dirty checkout inputs or delegates.
Validation limits: no PR code, tests, builds, installs, app launches or live media requests were executed. One exact-head hosted-CI snapshot (run 36388104774) showed JavaScript, browser measurements, Chromium shards 2/3 and 3/3, WebKit shard 2/3, DCO and security checks passing. Rust/tool integration and the remaining browser shards were still running; Windows native validation was skipped. Native/packaged behavior, real GitHub media redirects/access and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review through Wes’s account.
Reviewed head 1b3965bb02cd1a91c0aa75a87e394ca6d6ec6853 against base/merge-base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. Follow-up scope: the two-file image-description/caption fix since reviewed head 10634c0dcd28746db2b86caea0a8025b10ee0db8, prior feedback, and the relevant renderer/media callers and lifecycle.
No actionable findings in this source-only follow-up. The image-description feedback is addressed in source: Markdown/HTML images retain their alt text, including explicit empty alt, without displaying it as a loaded caption. Images promoted from links receive the plain-text link label, including nested formatting; an empty label preserves MediaAttachment’s default description. Preview and fullscreen share that description, and error paths retain the attachment link. The added colocated test sources cover these cases. The earlier HTML-content-preservation fix remains intact.
Evidence: 29 app source/document blobs verified against the pinned head; the relevant vision documents verified at block/buzz head c4c86006f2d67b2ea9055780129ee7ce95c54e0a. No dirty checkout inputs or delegates.
Validation limits: no PR code, tests, builds, installs, app launches, or live media requests were executed; CI was not checked in this cycle. Native/packaged behavior, real GitHub media redirects/access, and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two HTML text-preservation regressions; see inline comments. The previously reported block-content loss and image-caption issues are addressed in the reviewed paths.
Render bounded GitHub-flavored Markdown and infer ordered attachments from structured API metadata. Reuse the image, video and audio players, preserve fallback links and allow verified GitHub media origins in the packaged CSP. Add renderer, loading and navigation regression coverage plus Chromium/WebKit playback, seeking, fullscreen and layout checks. Record PR #327 with live public videos as a local validation artifact; packaged playback and human acceptance remain pending. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Hide original attachment links after inline media loads and restore them on failure. Preserve descriptive labels, surrounding Markdown, unsupported media links, and ordinary file attachments. Add focused normal and fallback rendering coverage and update the Chromium/WebKit media journey. Refresh the local PR #327 recording without publishing externally. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Keep Markdown support, inline media, and fallback access guidance. Remove renderer implementation details from the channel documentation. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Extract HTML text, images, file links and media destinations into safe Markdown nodes instead of dropping non-image content. Preserve document order and fallback links while keeping arbitrary HTML inert and bounding traversal depth. Cover HTML-only and mixed blocks, unsafe links, linked media and depth limits with focused tests. Exercise mixed HTML content in the existing Chromium/WebKit panel journey. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Preserve authored image alt text without loaded captions. Describe image previews and fullscreen images using flattened link labels while retaining formatted captions and fallback links. Add mounted regression coverage, verified failing before the fix and passing afterward. All 24 renderer tests, TypeScript, and the existing Chromium/WebKit PR-media journey pass. Human visual acceptance remains pending. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
49feada to
972f959
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes recommended: two product defects and one deterministic browser-test integration failure, detailed inline. The earlier HTML-content, image-alt/caption, anchor-label and table-cell findings are fixed.
Reviewed head 972f95907596435f82d09a7078d9173bae98ffb6 against base snapshot a7b45d346d50ca7137a4f696239fe120dc1633da (merge-base f551b81ced0d940075f0ca28f83e0ae718743428). Reconciled the in-flight rebase from 49feada4, including the shared video-player and CSP changes. Read all 12 changed files and relevant callers/docs; integrated independent HTML/accessibility and security/URL review lanes.
Validation at current head: 66 focused unit tests passed. Notifications passed in Chromium/WebKit; the checked-in GitHub journey failed in both at the obsolete native-controls assertion. A diagnostic copy targeting the current custom timeline passed the rest of that journey in both engines. Separate browser probes reproduced the newline defect and confirmed that a real public WAV fails under the current media CSP but loads when the missing redirect origin is permitted. No production files were modified.
Exit criteria: resolve the two rendering/media findings, align the browser assertion with the existing custom controls without dropping seeking coverage, and rerun affected checks. Latest hosted snapshot: measurements, security and DCO passed; JavaScript, Rust/tool integration and browser shards were still running. Packaged desktop and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
* origin/main: Keep custom emoji animated in reactions (#354) Polish community dialogs, agent cards, and conversation controls (#342) fix(channels): paginate membership discovery beyond 500 channels (#326) Remove local project context from docs (#350) feat(github): render PR descriptions with inline media (#335) feat(dev): measure channel opens, warming cost and live setup (#315) fix(agents): start new agents on Create and make their status clear (#332) fix(macos): close the window without quitting Buzz (#349) fix: allow parallel desktop dev worktrees (#336) feat(relay): complete packaged community access and recovery (#338) fix(workflows): clarify controls and align compact workflow UI (#337) feat: add custom emoji from settings (#346) feat(channels): show typing status on sidebar dm rows (#305) fix(workflows): page batched definition reads (#325) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentsPage.tsx
Summary
Validation