Conversation
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughThis PR introduces a media widget data endpoint and client-side adapter layer for the media player. The backend route validates requests, auto-resolves access tokens by scanning feeds, retrieves file metadata, and serves media data with artwork and stream URLs. The client refactors MediaPlayerApp to extract MediaData through prioritized sources (toolOutput, direct data, or API fallback). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MediaPlayerApp
participant API as /mcp/widget/media-data
participant FeedDB as Feeds & Directory
participant FileSystem as File Metadata
Client->>Client: extractMediaData()
alt MediaData available in renderData
Client->>Client: Return existing MediaData
else
Client->>API: POST /mcp/widget/media-data<br/>(mediaRoot, relativePath, token?)
alt Token provided
API->>API: Validate token
else
API->>FeedDB: Scan feeds/directory<br/>for valid token
FeedDB-->>API: Return accessible token
API->>API: Validate resolved token
end
API->>FileSystem: Get file metadata
FileSystem-->>API: metadata (title, author, etc.)
API->>API: Construct artworkUrl<br/>& streamUrl
API-->>Client: MediaWidgetData
Client->>Client: Return parsed MediaData
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/routes/mcp/widget-media-data.ts:
- Around line 85-87: The method-check branch currently returns a plain text
response (new Response('Method not allowed', { status: 405 })) which is
inconsistent with other error responses; replace it with a JSON response (e.g.,
Response.json({ error: 'Method not allowed' }, { status: 405 })) so the
handler's method-check uses the same JSON error format as other branches and
clients can parse errors consistently; update the code around the
context.request.method check in widget-media-data.ts accordingly.
- Around line 81-190: The endpoint lacks CORS headers so cross-origin widget
requests fail; update the action handler to respond to OPTIONS preflight (when
context.request.method === 'OPTIONS') with a 204 and CORS headers, and include
the same CORS headers on every other Response/Response.json return (including
error responses and the final mediaData response). Add headers such as
Access-Control-Allow-Origin (e.g. '*', or the allowed origin),
Access-Control-Allow-Methods ('POST, OPTIONS'), and Access-Control-Allow-Headers
('Content-Type, Authorization') to all Response/Response.json calls and to the
Response returned for non-POST methods so that getFeedByToken, parse/validation
error responses, and the successful mediaData response all include consistent
CORS headers.
🧹 Nitpick comments (4)
app/client/widgets/media-player.tsx (3)
83-88: DuplicatedencodeRelativePathfunction.This function duplicates the implementation in
app/helpers/feed-access.ts. While the duplication may be intentional for client-side bundle isolation (avoiding server imports), consider extracting shared utilities to a common location that can be imported by both client and server code if the bundler supports it.
182-203: Consider adding a timeout for the fetch request.The fetch call has no timeout or abort signal. If the server is slow or unresponsive, the widget will remain in a loading state indefinitely. Per the coding guidelines, use
this.signalfor cancellable async operations to handle component unmounting.♻️ Suggested improvement
Pass an
AbortSignalto the fetch call. The signal could come from the component'sthis.signalif this function is called from within a Remix component context, or useAbortSignal.timeout():async function fetchMediaDataFromApi( mediaRoot: string, relativePath: string, token?: string, + signal?: AbortSignal, ): Promise<MediaData> { const response = await fetch('/mcp/widget/media-data', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ mediaRoot, relativePath, token }), + signal: signal ?? AbortSignal.timeout(30000), })
705-733: Consider usingthis.signalfor async operation cancellation.Per the coding guidelines, use
this.signalfor cancellable async operations in Remix components to handle component unmounting. If the component unmounts whileextractMediaDatais in progress (especially during the API fetch fallback),this.update()may be called on a destroyed handle.♻️ Suggested approach
function MediaPlayerApp(this: Handle) { let state: 'loading' | 'ready' | 'error' = 'loading' let media: MediaData | null = null let errorMessage = '' + const signal = this.signal // Request render data from parent frame void waitForRenderData(RenderDataSchema) .then(async (renderData) => { + if (signal.aborted) return console.log('[MediaPlayer] Received render data:', renderData) // Use the adapter to extract and transform media data media = await extractMediaData(renderData) + if (signal.aborted) return state = 'ready' this.update() }) .catch((err) => { + if (signal.aborted) return console.error('[MediaPlayer] Error receiving render data:', err)app/routes/mcp/widget-media-data.ts (1)
38-75: Consider performance for large feed counts.The function iterates through all directory and curated feeds sequentially. For deployments with many feeds, this could be slow. The dynamic imports also re-resolve on each call.
For now this is likely acceptable given the fallback nature of this code path, but worth noting for future optimization if needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/widgets/media-player.tsxapp/config/routes.tsapp/router.tsxapp/routes/mcp/widget-media-data.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Do not use React, Preact, or any other UI framework. Use@remix-run/componentfor UI components instead
Bun automatically loads .env files, so do not use dotenv package
UseBun.envinstead ofprocess.envto access environment variables for consistency
Usebun:sqlitefor SQLite instead of better-sqlite3
UseBun.redisfor Redis instead of ioredis
UseBun.sqlfor Postgres instead of pg or postgres.js
Use built-inWebSocketinstead of ws package
PreferBun.fileovernode:fsreadFile/writeFile for file operations
UseBun.$template literals instead of execa for shell commands
Files:
app/config/routes.tsapp/routes/mcp/widget-media-data.tsapp/client/widgets/media-player.tsxapp/router.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: For Remix stateless components, return JSX directly without using state management
For Remix stateful components, usethis: Handleand return a function that returns JSX, with state living in the closure
UserenderPropsinside the render function to get the latest prop values in Remix components with both props and state, and usesetupPropsfor initial setup
Useon={{ eventName: handler }}syntax instead ofonClickor other camelCase event handlers in Remix components
Use thecssprop for inline styles with pseudo-selector support in Remix components
Usethis.on()to subscribe to custom events or other event targets in Remix components
Usethis.signalfor cancellable async operations in Remix components to handle component unmounting
Use theconnectprop instead of React-style refs to detect when an element is added to the DOM
Usethis.context.set()in Remix components to provide context andthis.context.get(ProviderComponent)to retrieve context from ancestor components
Files:
app/config/routes.tsapp/routes/mcp/widget-media-data.tsapp/client/widgets/media-player.tsxapp/router.tsx
🧠 Learnings (1)
📚 Learning: 2026-01-07T19:23:24.667Z
Learnt from: CR
Repo: kentcdodds/mediarss PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-07T19:23:24.667Z
Learning: Applies to **/*.{ts,tsx} : For Remix stateless components, return JSX directly without using state management
Applied to files:
app/client/widgets/media-player.tsx
🧬 Code graph analysis (1)
app/client/widgets/media-player.tsx (1)
app/helpers/feed-access.ts (1)
encodeRelativePath(60-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (10)
app/config/routes.ts (1)
16-16: LGTM!The new route entry is properly placed within the MCP routes section and follows the existing naming conventions.
app/router.tsx (1)
35-35: LGTM!The import and route mapping are correctly placed. Importantly,
mcpWidgetMediaDatais registered beforemcpWidgetto ensure the static path matches before the parameterized wildcard route.Also applies to: 150-151
app/client/widgets/media-player.tsx (6)
23-35: LGTM!The
MediaDataSchemais well-documented as the widget's contract. The comment warning against changing this schema helps maintain backward compatibility.
68-73: Appropriate use ofz.unknown().nullable()for external data.Using
z.unknown().nullable()with.passthrough()is the right approach for data coming from external sources (ChatGPT's MCP-UI protocol), allowing flexible validation while preserving additional properties.
122-144: LGTM - Robust adapter with double validation.The adapter correctly parses the tool output, maps fields, and re-validates through
MediaDataSchema.parse. This double validation ensures any schema drift betweenToolOutputSchemaandMediaDataSchemasurfaces immediately at runtime.
149-158: LGTM!Type guard functions using
safeParseare an idiomatic Zod pattern for runtime type checking without throwing.
217-278: LGTM - Well-structured priority-based extraction.The multi-level priority system handles various MCP-UI implementation scenarios gracefully. The detailed error message with
availableDatacontext will help debug integration issues when neither toolInput nor toolOutput matches expected schemas.
367-396: LGTM - Stateless component per coding guidelines.
MetadataItemcorrectly returns JSX directly without state management, following the Remix stateless component pattern.app/routes/mcp/widget-media-data.ts (2)
27-31: LGTM!Request schema is appropriately minimal, allowing the server to auto-resolve tokens when not provided.
171-183: The types are properly synchronized between server and client. Verification confirmsMediaWidgetDataandMediaDataSchemahave matching fields and types with consistent nullability handling.
| export default { | ||
| middleware: [], | ||
| async action(context) { | ||
| // Only allow POST | ||
| if (context.request.method !== 'POST') { | ||
| return new Response('Method not allowed', { status: 405 }) | ||
| } | ||
|
|
||
| // Parse request body | ||
| let body: unknown | ||
| try { | ||
| body = await context.request.json() | ||
| } catch { | ||
| return Response.json({ error: 'Invalid JSON body' }, { status: 400 }) | ||
| } | ||
|
|
||
| // Validate request | ||
| const parseResult = RequestSchema.safeParse(body) | ||
| if (!parseResult.success) { | ||
| return Response.json( | ||
| { error: 'Invalid request', details: parseResult.error.issues }, | ||
| { status: 400 }, | ||
| ) | ||
| } | ||
|
|
||
| const { mediaRoot, relativePath, token: providedToken } = parseResult.data | ||
|
|
||
| // If no token provided, find one automatically | ||
| let token = providedToken | ||
| if (!token) { | ||
| token = (await findTokenForMedia(mediaRoot, relativePath)) ?? undefined | ||
| if (!token) { | ||
| return Response.json( | ||
| { | ||
| error: 'No access token available', | ||
| message: | ||
| 'No feed has access to this media file, or no active tokens exist.', | ||
| }, | ||
| { status: 403 }, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Validate token and get feed | ||
| const result = getFeedByToken(token) | ||
| if (!result) { | ||
| return Response.json( | ||
| { error: 'Invalid or expired token' }, | ||
| { status: 401 }, | ||
| ) | ||
| } | ||
|
|
||
| const { feed, type } = result | ||
|
|
||
| // Parse and validate the path | ||
| const parsed = parseMediaPathStrict(`${mediaRoot}/${relativePath}`) | ||
| if (!parsed) { | ||
| return Response.json({ error: 'Invalid path format' }, { status: 400 }) | ||
| } | ||
|
|
||
| // Get absolute path for the file | ||
| const filePath = toAbsolutePath(parsed.rootName, parsed.relativePath) | ||
| if (!filePath) { | ||
| return Response.json({ error: 'Unknown media root' }, { status: 404 }) | ||
| } | ||
|
|
||
| // Validate file is allowed for this feed | ||
| if (!isFileAllowed(feed, type, parsed.rootName, parsed.relativePath)) { | ||
| return Response.json( | ||
| { error: 'File not found or not accessible' }, | ||
| { status: 404 }, | ||
| ) | ||
| } | ||
|
|
||
| // Get file metadata | ||
| const metadata = await getFileMetadata(filePath) | ||
| if (!metadata) { | ||
| return Response.json( | ||
| { error: 'Could not read media file metadata' }, | ||
| { status: 404 }, | ||
| ) | ||
| } | ||
|
|
||
| // Build token-based URLs | ||
| const encodedPath = encodeRelativePath( | ||
| `${parsed.rootName}/${parsed.relativePath}`, | ||
| ) | ||
| const baseUrl = getOrigin(context.request, context.url) | ||
|
|
||
| // Build the response data | ||
| const mediaData: MediaWidgetData = { | ||
| title: metadata.title, | ||
| author: metadata.author, | ||
| duration: metadata.duration, | ||
| sizeBytes: metadata.sizeBytes, | ||
| mimeType: metadata.mimeType, | ||
| publicationDate: metadata.publicationDate?.toISOString() ?? null, | ||
| description: metadata.description, | ||
| narrators: metadata.narrators, | ||
| genres: metadata.genres, | ||
| artworkUrl: `${baseUrl}/art/${token}/${encodedPath}`, | ||
| streamUrl: `${baseUrl}/media/${token}/${encodedPath}`, | ||
| } | ||
|
|
||
| return Response.json(mediaData, { | ||
| headers: { | ||
| 'Cache-Control': 'private, max-age=300', // 5 minute cache | ||
| }, | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Missing CORS headers for cross-origin widget requests.
The media player widget is embedded in external apps like ChatGPT, which will make cross-origin requests to this endpoint. Without CORS headers, these requests will fail in browsers.
🔒️ Suggested fix - add CORS headers
export default {
- middleware: [],
+ middleware: [
+ // Handle CORS preflight
+ async (context, next) => {
+ if (context.request.method === 'OPTIONS') {
+ return new Response(null, {
+ status: 204,
+ headers: {
+ 'Access-Control-Allow-Origin': '*',
+ 'Access-Control-Allow-Methods': 'POST, OPTIONS',
+ 'Access-Control-Allow-Headers': 'Content-Type',
+ 'Access-Control-Max-Age': '86400',
+ },
+ })
+ }
+ return next()
+ },
+ ],
async action(context) {And add CORS headers to the response:
return Response.json(mediaData, {
headers: {
'Cache-Control': 'private, max-age=300',
+ 'Access-Control-Allow-Origin': '*',
},
})🤖 Prompt for AI Agents
In @app/routes/mcp/widget-media-data.ts around lines 81 - 190, The endpoint
lacks CORS headers so cross-origin widget requests fail; update the action
handler to respond to OPTIONS preflight (when context.request.method ===
'OPTIONS') with a 204 and CORS headers, and include the same CORS headers on
every other Response/Response.json return (including error responses and the
final mediaData response). Add headers such as Access-Control-Allow-Origin (e.g.
'*', or the allowed origin), Access-Control-Allow-Methods ('POST, OPTIONS'), and
Access-Control-Allow-Headers ('Content-Type, Authorization') to all
Response/Response.json calls and to the Response returned for non-POST methods
so that getFeedByToken, parse/validation error responses, and the successful
mediaData response all include consistent CORS headers.
| if (context.request.method !== 'POST') { | ||
| return new Response('Method not allowed', { status: 405 }) | ||
| } |
There was a problem hiding this comment.
Inconsistent error response format.
This returns plain text while other error responses use Response.json(). For consistency and easier client-side error handling, use JSON format.
🐛 Suggested fix
if (context.request.method !== 'POST') {
- return new Response('Method not allowed', { status: 405 })
+ return Response.json({ error: 'Method not allowed' }, { status: 405 })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context.request.method !== 'POST') { | |
| return new Response('Method not allowed', { status: 405 }) | |
| } | |
| if (context.request.method !== 'POST') { | |
| return Response.json({ error: 'Method not allowed' }, { status: 405 }) | |
| } |
🤖 Prompt for AI Agents
In @app/routes/mcp/widget-media-data.ts around lines 85 - 87, The method-check
branch currently returns a plain text response (new Response('Method not
allowed', { status: 405 })) which is inconsistent with other error responses;
replace it with a JSON response (e.g., Response.json({ error: 'Method not
allowed' }, { status: 405 })) so the handler's method-check uses the same JSON
error format as other branches and clients can parse errors consistently; update
the code around the context.request.method check in widget-media-data.ts
accordingly.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| { status: 403 }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unauthenticated endpoint exposes access tokens in response URLs
Medium Severity
The new /mcp/widget/media-data endpoint has no authentication middleware and automatically discovers access tokens via findTokenForMedia when none is provided. These discovered tokens are then embedded in the artworkUrl and streamUrl response fields. An unauthenticated caller who knows or guesses valid file paths can extract valid tokens from the response URLs and use them to access other files within the same feed, bypassing the intended token-based access control model.
Additional Locations (1)
2 participants
Fixes a schema mismatch between the MediaRSS tool's output and the media player widget's expected input. An adapter layer transforms the tool's
{ metadata, access }response into the widget'sMediaDataformat, derivingartworkUrlandstreamUrl. Includes a new API fallback for environments (like ChatGPT) that don't fully populatetoolOutputorinitial-render-data, ensuring the widget always receives valid data with robust runtime validation.Test Plan
toolOutput: Simulate arenderDatapayload wheretoolOutputcontains{ metadata, access }(as perToolOutputSchema). The widget should render correctly.toolInput(API fallback): Simulate arenderDatapayload wheretoolInputcontains onlymediaRootandrelativePath, andtoolOutputis null. The widget should make aPOST /mcp/widget/media-datacall and render correctly.Checklist
Screenshots
Note
Resolves schema mismatch and ensures the media player renders reliably across environments.
media-player.tsxwithToolOutputSchema, URL derivation helpers, andextractMediaDatato handle multiple input paths (toolOutput, toolInput, or API fallback)POST /mcp/widget/media-dataendpoint (app/routes/mcp/widget-media-data.ts) that validates access (token lookup, feed permissions), reads file metadata, and returnsMediaWidgetDataapp/config/routes.tsandapp/router.tsx; widget calls the endpoint when only minimal path params are providedWritten by Cursor Bugbot for commit 351a5de. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.