Repository navigation
fix(web): host videos explain missing file access instead of offering a retry - #21088
BGD-1DAY00 wants to merge 3 commits into
Conversation
… a retry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped web bug fix that improves the existing missing-file-access state for host videos and removes only the ineffective retry action in that state. Normal video behavior remains unchanged, and the new path has focused browser coverage. You can add or adjust custom eligibility rules. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe0a9fb8f6
ℹ️ 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".
| <ChatMarkdownVideo | ||
| src={src} | ||
| sourceFailed={assetUrl._tag === "Failure" && fallbackSrc === undefined} | ||
| unavailableDetail={missingFileAccess ? MISSING_FILE_ACCESS_MESSAGE : undefined} |
There was a problem hiding this comment.
On the mobile client, when a host video is viewed over a connection without filesystem:read, ThreadMarkdownVideo still turns the access failure into only unavailable (apps/mobile/src/features/threads/ThreadMarkdownVideo.tsx:20-42), and the mobile player hard-codes only “Video unavailable” (apps/mobile/src/components/MediaVideoPlayer.tsx:165-166). The access explanation added here is therefore still absent on a first-class client; pass the missing-access reason/detail through the mobile video path too.
AGENTS.md reference: AGENTS.md:L67-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, it is the same defect on mobile. Fixed in 08412b8: ThreadMarkdownVideo passes MISSING_FILE_ACCESS_MESSAGE when the asset failed with missing-file-access, and the mobile tile shows "Video unavailable. ", the same wording ThreadMarkdownImage uses. The tile has no retry, so nothing else changes. Checked with tsc-rs --noEmit and lint in apps/mobile; not run on a device.
| <span className="inline-flex items-center gap-1.5"> | ||
| <TriangleAlertIcon aria-hidden className="size-3.5 shrink-0" /> | ||
| Video unavailable{label ? ` · ${label}` : ""} | ||
| {["Video unavailable", label, unavailableDetail].filter(Boolean).join(" · ")} |
There was a problem hiding this comment.
When an access-denied video appears before real message text containing a word from this new boilerplate (for example connection), thread find now assigns occurrence 0 to the generated status instead of the persisted message match. collectThreadFindRanges skips generated DOM only under [data-thread-find-ignore] (apps/web/src/components/chat/threadFindHighlights.ts:50-56), and the image fallback already sets that marker (ChatMarkdown.tsx:1352); mark this video label as ignored as well so local highlighting remains aligned with the server's persisted-text occurrence indexes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 08412b8: the video label now carries data-thread-find-ignore, matching ChatMarkdownMediaUnavailableLabel for images, so thread find skips the generated text.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
Issue #21087 targets apps/web. The pull request also changes apps/mobile/src/components/MediaVideoPlayer.tsx and apps/mobile/src/features/threads/ThreadMarkdownVideo.tsx. These changes implement… |
Remove the mobile changes from this pull request, or link a separate active issue that requires the mobile behavior. |
-
Check name Status Explanation Linked Issues check Issue #21087requires a host video withoutfilesystem:readto explain the missing access and avoid an ineffective retry. The web changes passMISSING_FILE_ACCESS_MESSAGEto the video fallback, dis…Description check The description covers the problem, change, scope and approval rationale, verification results, UI evidence, and unverified areas. It also identifies the agent and harness used. Title check The title clearly and concisely describes the primary fix: host videos now explain missing file access instead of offering an unusable retry. Full details: Linked Issues check
-
Explanation
Issue
#21087requires a host video withoutfilesystem:readto explain the missing access and avoid an ineffective retry. The web changes passMISSING_FILE_ACCESS_MESSAGEto the video fallback, display it in the unavailable label, and omit the retry handler for this state. The added browser test checks the message and the absence ofRetry video.
-
Full details: Out of Scope Changes check
-
Explanation
Issue
#21087targetsapps/web. The pull request also changesapps/mobile/src/components/MediaVideoPlayer.tsxandapps/mobile/src/features/threads/ThreadMarkdownVideo.tsx. These changes implement similar behavior, but the directly linked issue contains no mobile requirement. The mobile changes are therefore outside the linked issue scope.Resolution
Remove the mobile changes from this pull request, or link a separate active issue that requires the mobile behavior.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
…skips the label Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
size:M
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08412b8820
ℹ️ 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".
| {props.unavailableDetail | ||
| ? `Video unavailable. ${props.unavailableDetail}` | ||
| : "Video unavailable"} |
There was a problem hiding this comment.
When a host video is blocked on iOS or Android with VoiceOver/TalkBack enabled, this new text is nested inside a Pressable whose explicit accessibilityLabel remains Play ${props.name} (line 148). The pressable is the accessible element and that label replaces its descendant text, so users hear only “Play Demo, disabled” and never the pairing explanation; set a failure-specific label including unavailableDetail when unavailable is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in b2a0acb: when the tile is unavailable its accessibilityLabel is now ". Video unavailable. ", built from the same text the tile shows, so VoiceOver and TalkBack read the explanation. A playable tile still announces "Play ". Checked with tsc-rs --noEmit and lint in apps/mobile; not run on a device.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2a0acbd2c
ℹ️ 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".
| unavailableDetail={ | ||
| "resource" in source && asset._tag === "Failure" && asset.reason === "missing-file-access" | ||
| ? MISSING_FILE_ACCESS_MESSAGE | ||
| : undefined |
There was a problem hiding this comment.
The embedded-tile follow-up now passes this detail, but fresh evidence is the normal host-video link path: a link such as [Demo](/tmp/demo.mp4) is routed by ThreadFeed.tsx:1793-1803 directly to VideoPreviewModal, bypassing this component. In that path VideoPreviewModal.tsx:36-54 collapses asset.reason to a boolean and VideoPreviewModal.ios.tsx:46-52 replaces it with a generic connection error, so Android still displays only “Video unavailable” and iOS tells the user to check the connection even though re-pairing is required; propagate missing-file-access through both modal implementations as well.
AGENTS.md reference: AGENTS.md:L67-L70
Useful? React with 👍 / 👎.
|
Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes. |
This branch has not been deployed
2 participants
Problem
On a connection without the
filesystem:readgrant, a host video in chat () shows "Video unavailable · Demo" and a "Retry video" button. The client blocks the asset before it asks the server, so Retry can never succeed, and nothing says why. A host image in the same state already says "This connection cannot read host files. Pair again with a new link to view it." Fixes #21087.Change
ChatMarkdownAssetImageworks out the missing-file-access state once and passes the existingMISSING_FILE_ACCESS_MESSAGEto the video as well as the image.MediaVideoPlayertakes an optionalunavailableDetailand shows it in its unavailable label, asChatMarkdownMediaUnavailableLabelalready does for images, and the label is markeddata-thread-find-ignorelike the image one. In that state the video gets no retry handler, so the button goes away. Pairing again grants the scope and the video loads as before.Mobile had the same gap:
ThreadMarkdownVideonow passes the same message, and the mobile tile shows "Video unavailable. ", the wordingThreadMarkdownImageuses. Its accessibility label carries the same text while it is unavailable, so screen readers announce the reason instead of "Play ". The mobile tile never had a retry.Scope and approval
A small fix for an obvious bug, reported in #21087: the video branch dropped the reason its image sibling shows, and offered an action that cannot succeed. The web and mobile video paths, plus one test.
Verification
filesystem:read, and the server trace showed no asset request for the video.ChatMarkdown.assets.browser.test.tsx: a host video on a connection without file access shows the message and no "Retry video". On main it fails withVideo unavailable · DemoRetry video; with this change it passes.vp test run --project browser src/components/ChatMarkdown.assets.browser.test.tsx: 2 passed.vp lintandvp fmt --checkon the changed files, andtsc-rs --noEmitinapps/webandapps/mobile: clean (lint shows two existing warnings inLoadedMediaVideo, which this PR does not touch).vp run dev, seeded withmigrate-dev-db), with the browser paired bynode apps/server/src/bin.ts pair --scope orchestration:read --scope orchestration:operate --scope terminal:operate --scope relay:read, on a thread whose prompt and reply both embed a host video:Claude Opus 5.5 via Claude Code, running in T3 Code.
🤖 Generated with Claude Code