Fix resource refresh button not triggering re-fetch#1148
Fix resource refresh button not triggering re-fetch#1148olaservo wants to merge 7 commits intomodelcontextprotocol:mainfrom
Conversation
…ocol#1120) The readResource function had an early return when the resource URI was already cached, preventing the Refresh button from re-fetching. Add a force parameter to bypass the cache on refresh, and update the cache-hit path to still sync the displayed content when switching between previously viewed resources. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
olaservo
force-pushed
the
feature/fix-resource-refresh-button-1120
branch
from
234b209 to
70a6b03
Compare
Failed resource reads were cached in resourceContentMap, causing stale
errors to be served on re-click instead of retrying. Errors now only
set the display content without polluting the cache.
Also renamed the `force` boolean parameter to a `{ bypassCache }` options
object for self-documenting call sites.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
olaservo
force-pushed
the
feature/fix-resource-refresh-button-1120
branch
from
c3cd4ef to
3054236
Compare
The test depends on npx downloading @modelcontextprotocol/server-everything at runtime and uses waitForTimeout, making it unreliable in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions Clear cached resource content when a bypassCache refresh fails, so subsequent clicks retry the fetch instead of serving stale data. Rename the inline wrapper parameter from opts to options for consistency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes resource re-fetch behavior in the client by adding a bypassCache option to readResource, ensuring the Refresh button triggers a new server request and cached content is correctly synced to the UI when revisiting previously viewed resources.
Changes:
- Add
{ bypassCache }option toreadResourceand use it to bypass cache on Refresh. - Sync cached resource content to the display instead of early-returning without updating UI state.
- Stop caching read errors in
resourceContentMap(intended to ensure retries on re-click).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client/src/App.tsx | Adds bypassCache support to readResource, updates caching/refresh logic, and forwards options through the ResourcesTab prop wrapper. |
| client/src/components/ResourcesTab.tsx | Updates readResource prop signature and passes { bypassCache: true } from the Refresh button. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously, failed resource_link reads left resourceContentMap[uri]
empty, so ResourceLinkView rendered an empty expansion with no error
feedback when a read failed in the Tools tab. Track errors in a
separate resourceErrorMap and thread it through ToolsTab/ToolResults
to ResourceLinkView so the user sees the error message. The existing
cache-skip behavior is preserved (errors don't pollute the content
map, so retries still work).
Also adds a ResourcesTab test asserting the Refresh button passes
{ bypassCache: true } to readResource, to prevent regression of modelcontextprotocol#1120.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (bypassCache) { | ||
| setResourceContentMap((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[uri]; | ||
| return next; | ||
| }); | ||
| } |
There was a problem hiding this comment.
When bypassCache is true and the refresh read fails, this deletes any previously cached successful content for the URI. That means a transient refresh failure can wipe the last-known-good content and force future reads to hit the network before anything can display. Consider keeping the cached content on refresh failure and only updating/removing cache entries on success (while still avoiding caching the error).
| if (bypassCache) { | |
| setResourceContentMap((prev) => { | |
| const next = { ...prev }; | |
| delete next[uri]; | |
| return next; | |
| }); | |
| } |
| console.error(`[App] Failed to read resource ${uri}:`, error); | ||
| const errorString = (error as Error).message ?? String(error); | ||
| setResourceContentMap((prev) => ({ | ||
| setResourceContent(JSON.stringify({ error: errorString })); |
There was a problem hiding this comment.
In the error path, setResourceContent(JSON.stringify({ error: errorString })) will overwrite the last successful resourceContent. Because callers clear errors.resources before triggering a new read, the Resources tab can temporarily render this JSON error as if it were valid cached content (no alert) until the next request resolves. Consider not writing error JSON into resourceContent here (leave previous content intact or clear it explicitly) and rely on errors.resources / resourceErrorMap for error presentation.
| setResourceContent(JSON.stringify({ error: errorString })); |
Summary
Fixes #1120
readResourcefunction had an early return when the resource URI was already inresourceContentMap, which prevented the Refresh button from ever re-fetching{ bypassCache }options parameter toreadResourceto skip the cache when the Refresh button is clickedChanges
client/src/App.tsx— Added{ bypassCache }options parameter toreadResource; stopped caching errors inresourceContentMap; forwarded options through the prop wrapperclient/src/components/ResourcesTab.tsx— Updated type signature; Refresh button now passes{ bypassCache: true }Test plan
npx tsc --noEmit)npm run build-client)npm run lint)🦉 Generated with Claude Code