Repository navigation
feat(mcp): select library media and follow indexing progress in the MCP App - #152
Haseeb-1698 wants to merge 2 commits into
Conversation
|
Hey @Haseeb-1698, |
|
Hi @SaadBazaz, both are done: the repo is starred, and this PR comes from my fork ( For context on the human question: I came to VidXP through the FAST-NUCES Islamabad Career Services invitation, and I commented on #73 first to check the direction before writing anything. Happy to answer anything about the change , why I flagged one limit in the PR body and it still stands: I have not run the widget inside a real MCP host, so the two new views are covered by unit tests, lint and JS syntax only, not against a live |
tulayha
left a comment
There was a problem hiding this comment.
This is a useful extension of the existing MCP App and tools. Please address the four inline UI findings and add focused coverage for these cases. The requested changes stay within the current library-selection and progress scope.
| more.disabled = true; | ||
| notice.textContent = ""; | ||
| try { | ||
| render(await callTool("list_media", { cursor: data.next_cursor })); |
There was a problem hiding this comment.
Please preserve the original filename and state filters when loading another page. Media cursors are tied to those filters, but this call sends only the cursor. For a filtered list, clicking Load more therefore fails with an invalid media cursor instead of showing the next page.
| notice.textContent = latestResult.isError ? "VidXP returned an error." : ""; | ||
| if (data.view === "upload" || data.upload_session_url || data.aggregate_state) renderUpload(data); | ||
| else if (data.view === "evidence" || data.board) renderEvidence(data, latestResult); | ||
| else if (data.view === "library" || asArray(data.items).some((item) => typeof asObject(item).media_id === "string")) renderLibrary(data); |
There was a problem hiding this comment.
An empty list_media result has items=[] and no view field, so it never reaches renderLibrary and instead displays "This tool result does not include an interactive view." Please recognize empty media pages too, so a new library or a filter with no matches gets an appropriate empty state.
| }; | ||
|
|
||
| const renderProgress = (data) => { | ||
| if (progressTimer !== null) { |
There was a problem hiding this comment.
The polling timer is cleared only when rendering another progress view. If a library or evidence result replaces this view, the old timer still runs and its response can replace the current screen with the previous job. Please stop polling when leaving the view or tearing down the widget, and ignore responses from requests belonging to a view that is no longer active.
| const actions = element("div", "toolbar"); | ||
| const refresh = element("button", "button", "Refresh status"); | ||
| refresh.type = "button"; | ||
| refresh.addEventListener("click", async () => { |
There was a problem hiding this comment.
If the refresh request fails, poll() catches the error but this button stays disabled. The user cannot retry that refresh. Please restore the button after a failed request, for example through finally, and cover the failure-and-retry path.
…CP App The MCP App covered uploading and inspecting evidence. Two items from the issue's first useful version were missing: selecting a video that is already registered, and following indexing progress. Attach the existing app template to list_media and get_job_status, and add the two matching views to the widget. list_media pages through the library and records the chosen media in the model context; get_job_status renders stage, message and step counts, and re-polls itself using the job's own poll_after_seconds until the job is terminal. No new tools and no separate application backend: both views drive tools that already exist, and the reused media and job lifecycle stays the source of truth. Closes grayhatdevelopers#73.
Four corrections from review, plus behavioural coverage for each. Load more sent only the cursor, so a filtered page failed with an invalid media cursor. The widget now carries the query it issued and merges the cursor into it. MediaPage returns items, total and next_cursor only, so an agent-issued first page leaves no recoverable filter context; pagination is withheld there rather than sending a cursor that cannot honour the filters. An empty media page has items: [] and no view field, so the dispatcher fell through to the no-interactive-view message. It now recognises a MediaPage by shape. The integer total is what separates it from JobPage, which carries items and next_cursor but no total. Progress polling cleared its timer only when another progress view rendered, so a library or evidence result left it running and a late response could replace the current screen. A generation token is now taken when the view renders and rechecked before any response touches the DOM; every render and the resource-teardown handler stop polling. A failed manual refresh left its button disabled with no way to retry. The button is now restored in a finally block. Add web/mcp-app with node --test coverage driving the shipped widget in a DOM stub: six cases, one per behaviour. They fail against the pre-fix widget and pass after.
3590566 to
fcb1947
Compare
|
Thanks for the review @tulayha. All four are addressed, and the branch is rebased onto 1. Load more dropping the filters. The widget now carries the query it issued and merges the cursor into it, so a filtered page continues correctly. There is a half of this I could not fix inside the widget, and I would rather flag it than paper over it. That is a deliberate trade and it has a visible cost: on an agent-issued first page, Load more no longer appears at all. Two ways out if you would rather it did. Either 2. Empty media page. The dispatcher now recognises a 3. Polling timer and stale responses. A generation token is taken when the progress view renders and rechecked before any response is allowed to touch the DOM, so an in-flight 4. Failed refresh. The button is restored in a ValidationNew
Python suite, on a clean
The same two tests fail on both trees: Still true from the original description: I have not run this inside a real MCP host, so the |
Heads up: Greptile reviewed this. Push back if it's wrong — it has no feelings.
[Medium impact] The change reaches people using VidXP through its MCP App. Fix unreachable library paging and failed selection checks before merging.
|
| wrapper.append(listing); | ||
| } | ||
|
|
||
| if (typeof data.next_cursor === "string" && data.next_cursor && query) { |
There was a problem hiding this comment.
Later library pages are unreachable
Every host entry point calls render without mediaQuery, but this condition requires it to show “Load more.” The only caller that supplies mediaQuery is the button's own handler, so users cannot reach videos beyond the first page. Capture the original list_media arguments before rendering and preserve its filters when following next_cursor.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/vidxp/assets/mcp_app/index.html
Line: 504
Comment:
**Later library pages are unreachable**
Every host entry point calls `render` without `mediaQuery`, but this condition requires it to show “Load more.” The only caller that supplies `mediaQuery` is the button's own handler, so users cannot reach videos beyond the first page. Capture the original `list_media` arguments before rendering and preserve its filters when following `next_cursor`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| persistMediaSelection(item.media_id); | ||
| const next = await callTool("get_media", { media_id: item.media_id }); | ||
| const detail = next.structuredContent; | ||
| lede.textContent = `Selected ${detail.original_filename || item.original_filename || item.media_id}.`; |
There was a problem hiding this comment.
Unavailable videos appear selected
The widget saves selectedMediaId before get_media confirms that the video still exists, and it never checks next.isError. If a listed video becomes unavailable, a returned tool error still produces “Selected …” and leaves that unavailable ID in the model context. Check next.isError and save the selection only after get_media succeeds.
| persistMediaSelection(item.media_id); | |
| const next = await callTool("get_media", { media_id: item.media_id }); | |
| const detail = next.structuredContent; | |
| lede.textContent = `Selected ${detail.original_filename || item.original_filename || item.media_id}.`; | |
| const next = await callTool("get_media", { media_id: item.media_id }); | |
| if (next.isError) throw new Error("VidXP could not select this video."); | |
| const detail = next.structuredContent; | |
| persistMediaSelection(item.media_id); | |
| lede.textContent = `Selected ${detail.original_filename || item.original_filename || item.media_id}.`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/vidxp/assets/mcp_app/index.html
Line: 487-490
Comment:
**Unavailable videos appear selected**
The widget saves `selectedMediaId` before `get_media` confirms that the video still exists, and it never checks `next.isError`. If a listed video becomes unavailable, a returned tool error still produces “Selected …” and leaves that unavailable ID in the model context. Check `next.isError` and save the selection only after `get_media` succeeds.
```suggestion
const next = await callTool("get_media", { media_id: item.media_id });
if (next.isError) throw new Error("VidXP could not select this video.");
const detail = next.structuredContent;
persistMediaSelection(item.media_id);
lede.textContent = `Selected ${detail.original_filename || item.original_filename || item.media_id}.`;
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const next = await callTool("get_job_status", { job_id: jobId }); | ||
| if (generation !== progressGeneration) return; | ||
| render(next); | ||
| } catch (_error) { | ||
| if (generation === progressGeneration) notice.textContent = "Could not refresh job status."; |
There was a problem hiding this comment.
Progress stops after one error
The timer is cleared before poll runs, but a failed request never schedules another attempt. A returned isError also reaches render(next), which replaces the status view and removes its refresh button. Keep the last status on errors, check next.isError, and schedule a bounded retry while the same job view remains active.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/vidxp/assets/mcp_app/index.html
Line: 564-568
Comment:
**Progress stops after one error**
The timer is cleared before `poll` runs, but a failed request never schedules another attempt. A returned `isError` also reaches `render(next)`, which replaces the status view and removes its refresh button. Keep the last status on errors, check `next.isError`, and schedule a bounded retry while the same job view remains active.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Related issue
Closes #73.
Summary
The MCP App already covered two of the four items in the issue's "first useful version": uploading (
create_media_upload) and inspecting evidence boards, keyframes and clips (present_job_evidence). This adds the other two , selecting a video that is already registered, and following indexing progress.Both are driven by tools that already exist, so there is no new tool and no separate application backend:
list_medianow carries the app template. The widget lists registered videos with state, duration, size and container, pages throughnext_cursor, and on selection writesselectedMediaIdviaui/update-model-contextandsetWidgetState, then confirms withget_media.get_job_statusnow carries it too. The widget rendersstage,messageandcurrent/totalwith a progress bar, offers a manual refresh, and re-polls itself using the job's ownpoll_after_secondswhileterminalis false, clearing its timer on every re-render.I used
get_job_statusrather thanget_jobbecause its own description recommends it for active work andJobSummaryis the compact record.Affected interfaces: two read-only tools gain
_metaonly; no signatures, arguments or return types change. The widget additions are new render paths dispatched after the existinguploadandevidencebranches, so current results render exactly as before. No compatibility or migration impact.Validation
Run on linux/amd64, Python 3.12, at
24b916a, afteruv sync --all-extras:ruff check .(0.16.6, whole repo)uv run --no-sync pytest -q tests/test_mcp.pyPYTHONPATH=. uv run --no-sync pytest -q(full suite)node --checkon the widget's extracted<script>The 7 failures are
chromadbRuntimeErrors fromchromadb/config.py:375intest_benchmarks.py,test_local_snapshots.pyandtest_storage_integration.py. They reproduce identically with this branch's two files stashed (7 failed, 32 passed on the clean tree), so they are pre-existing on this machine and unrelated to the change. The full suite also needsPYTHONPATH=.here or five packaging modules fail to importutils; that is likewise unchanged by this branch.Not exercised: I have not run the widget inside a real MCP host, so the new views are verified by unit tests, lint and JavaScript syntax only, not against an actual host's
ui/*message flow, and not against a live indexing job. Please treat the rendering as unproven at that boundary.I opened this rather than waiting on my comment in #73 so there is something concrete to react to. Happy to rework the scope, split it into library-selection and progress separately, or drop it if you have a different direction in mind.