Skip to content

fix(web): play pull request videos inline - #10617

Merged
maria-rcks merged 1 commit into
pingdotgg:mainfrom
maria-rcks:t3code/fix-playable-pr-videos
Sep 7, 2026
Merged

fix(web): play pull request videos inline#10617
maria-rcks merged 1 commit into
pingdotgg:mainfrom
maria-rcks:t3code/fix-playable-pr-videos

Conversation

@maria-rcks

@maria-rcks maria-rcks commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

PR video uploads currently render as external-link cards. Render them with the shared inline video player, including native controls, retry, and an original-link fallback; changing an upload URL resets its player.

Verified both videos from #9472 through completion in T3's PR tab and exercised failed playback followed by retry. The 29 focused markdown/first-frame tests, web typecheck, targeted lint, and formatting checks pass.

Before

PR 9472 video links before the fix

After

PR 9472 inline video after playback

Playback recordings were captured by the desktop preview, but its mac-local artifact paths are not downloadable through the available preview tools. Video evidence upload remains blocked; screenshots above were captured from the same PR.

Built with gpt-6-astra in the Codex harness.

Note

Play pull request videos inline in PullRequestMarkdown

  • Video uploads now play inline using the shared MediaVideoPlayer instead of linking to GitHub.
  • Non-video uploads continue to render as external links but now use PaperclipIcon directly with a fixed label.
  • Behavioral Change: Non-video attachments drop the previous attachment card in favor of a fixed label and PaperclipIcon.

Macroscope summarized 08e8bb6.

Summary by CodeRabbit

  • New Features
    • Pull request videos now play inline instead of opening on GitHub.
  • Bug Fixes
    • Video attachments now use their segment URL for playback and the original-link option.
    • Non-video attachments consistently display the paperclip icon and “Open attachment on GitHub” label.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 7, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 08e8bb6

Macroscope's review found this PR approvable — This is a focused one-file web UI fix that replaces the old external video link card with the existing inline media player while preserving fallback behavior for unsupported videos. Its runtime impact is limited to pull-request rendering, with no broader production or data-model changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: c90a00aa-bf75-4c63-8d82-62cabf68d225

📥 Commits

Reviewing files that changed from the base of the PR and between d64335b and 08e8bb6.

📒 Files selected for processing (1)
  • apps/web/src/components/pullRequest/PullRequestMarkdown.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Pull request video segments now render inline with MediaVideoPlayer. Non-video attachments retain a GitHub link fallback with a paperclip icon.

Changes

Pull request media rendering

Layer / File(s) Summary
Inline video rendering and attachment fallback
apps/web/src/components/pullRequest/PullRequestMarkdown.tsx
Video segments render with MediaVideoPlayer, using the segment URL for playback and the original link. Other attachments use PaperclipIcon and the “Open attachment on GitHub” label.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 08e8b

Pull request video uploads now play inline with retry and original-link fallback, while non-video attachments retain their GitHub links. The covered behavior and checks indicate no remaining merge-blocking risk.

Suggested reviewers: bil0000, juliusmarminge, sunkenintime

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description explains what changed, why it changed, the UI impact, verification steps, and before/after screenshots. It also documents the blocked video evidence upload.
Title check ✅ Passed The title clearly and concisely identifies the main change: rendering pull request videos inline in the web application.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@maria-rcks
maria-rcks merged commit 569a8cd into pingdotgg:main Sep 7, 2026
21 checks passed
aorwall added a commit to aorwall/t3code that referenced this pull request Sep 8, 2026
Merges `pingdotgg/t3code` `8b2838e0e..a37c664` — 43 commits.

`343` files landed against `343` changed in the upstream range; fork
delta `723` files. Exact match, so nothing upstream changed was dropped.

Details in
[`docs/fork/upstream-merge-log.md`](../blob/merge/upstream-2026-09-08/docs/fork/upstream-merge-log.md).

## Two fork deltas this merge had to re-apply

**Upstream split the server-update banner into two routes.** pingdotgg#10596
added `useAutoBalanceUpdateBanner` beside the single-machine condition
the fork already gates. The conflict was on the first line only, so
resolving it correctly still left the auto-balance route ungated — an
auto-balanced project would have been offered `npx t3` against a backend
that does not implement `server.updateServer`.
`FEATURES.serverUpdateBanner` now carries two gates in `ChatView.tsx`.

**A new settings page needs a gate even though it degrades politely.**
pingdotgg#8103 added `/settings/snap-shot` for desktop window capture. Every
control drives `window.desktopBridge`, and upstream renders an
"unavailable" notice rather than hiding the page, so a hosted build
listed a sidebar section and six searchable rows for a feature it can
never run. Gated with `FEATURES.snapShots`.

Two smaller fixes: `packages/moatless-api` still ran `tsgo --noEmit`
after upstream replaced `@typescript/native-preview` with TypeScript
7.0.2, and `duplicate-adds.mjs` now skips `pnpm-lock.yaml` (it read
`iconv-lite: 0.6.3` as taken twice; `d3-dsv` and `encoding` each declare
it).

## Usable as-is

- Stop-thread keybinding command (pingdotgg#4308).
- Project import tolerates servers that predate the git-identity scan
(pingdotgg#10547).
- Proactive panels open when entering a thread (pingdotgg#10610); pull-request
markdown links open in the panel (pingdotgg#10623); markdown images navigate as
galleries (pingdotgg#10625); pull-request videos play inline (pingdotgg#10617).
- Settings project scopes are searchable and scrollable (pingdotgg#10570); ref
picker stays steady when opening (pingdotgg#9472); sidebar timer uses
`tabular-nums` (pingdotgg#10592); popup triggers stay steady when pressed
(pingdotgg#9468); settled PR colors restore on hover (pingdotgg#10023).
- Composer Fast mode persists across new chats (pingdotgg#2981); inserted
citations are removed on cancel (pingdotgg#10518).
- TypeScript 7.0.2 (pingdotgg#10663) and the knip desktop-export rules (pingdotgg#10269).

## Unsupported in Moatless / needs implementation

- **Cross-platform window capture** (pingdotgg#8103) —
`apps/desktop/src/snapShot/**`,
`apps/web/src/components/settings/SnapShotSettings.tsx`,
`apps/web/src/lib/desktopSnapShot.ts`. Needs an Electron
`window.desktopBridge`; a browser tab has none. Gated behind
`FEATURES.snapShots` in this PR.
- **Auto-balance server update** (pingdotgg#10596) —
`apps/web/src/components/chat/useAutoBalanceUpdateBanner.tsx`. Needs
`server.updateServer`, which Moatless does not dispatch. Gated behind
`FEATURES.serverUpdateBanner` in this PR.
- **Preview recording transfer** (pingdotgg#10572) —
`apps/server/src/mcp/toolkits/preview/handlers.ts`,
`apps/web/src/browser/browserRecordingUpload.ts`. Moves a finished
preview recording into the agent environment over the desktop bridge.
Adds four error types to `packages/contracts/src/previewAutomation.ts`
and no new RPC method, so no union changed. Sits behind the
`previewAutomation.connect` / `focusHost` / `respond` gap already in the
register.
- **Local media linked from remote threads** (pingdotgg#10619) and **browser
editing shortcuts** (pingdotgg#10621) — Electron shell only.
- **iOS Keychain access group** (pingdotgg#3665) and the mobile provider account
badge (pingdotgg#9899) — the fork ships no mobile build against Moatless.

## Backend behavior to consider reproducing in Moatless

- **Name the usage limit and its reset instead of relaying "out of
credits"** (pingdotgg#10473, `apps/server/src/provider/**` Codex adapter).
Moatless owns its provider runtime, so the clearer limit message has to
be produced there.
- **Report usage limits on retried turns** (pingdotgg#10549, Claude adapter). A
retry currently loses the limit signal; same ownership.
- **Disable executable capabilities in Claude metadata generation**
(pingdotgg#4169, `apps/server/src/textGeneration/ClaudeTextGeneration.ts`). Title
and metadata generation should not be able to run tools. Worth mirroring
wherever Moatless generates thread titles.

## Verification

`verify.mjs`: duplicate-adds, tripwires, resolution-check,
unsupported-methods (0 ADD, 0 DROP, 2 KEEP), fmt, lint and typecheck all
pass.

Tests pass except `@t3tools/desktop`, which cannot compile
`scripts/browser-secret-native.test.mjs` because the sandbox has no
`libsecret-1` — 1283 tests pass, 0 fail, and the file is byte-identical
to upstream. New entry in `docs/fork/gaps.md`. `t3` failed
`GrokAdapter.test.ts` once under parallel load and passes 42/42 alone.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/6d8ea486-2fcb-4c25-bd34-dcd15cc4a7ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant