Fix resource refresh button not triggering re-fetch#1148
Fix resource refresh button not triggering re-fetch#1148olaservo wants to merge 13 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>
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>
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.
…r JSON to resourceContent
Don't delete resourceContentMap[uri] when a bypassCache refresh fails;
a transient network error should not wipe the last-known-good content.
To prevent serving stale content after a known error, the cache-hit
guard now also skips cache when resourceErrorMap[uri] is set, forcing
a re-fetch until a successful read clears the error.
Also stop writing JSON.stringify({ error }) into resourceContent in
the catch block. sendMCPRequest already sets errors.resources (which
the ResourcesTab Alert displays), and the Tools tab shows errors via
resourceErrorMap. Writing error JSON into resourceContent was both
redundant and could bleed through as JsonView content after callers
cleared errors.resources before the next read.
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 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Switch from truthiness checks to presence checks in the resource error/content handling so an empty-string error message is still treated as an error. readResource's cache-hit guard now uses `uri in resourceContentMap` / `!(uri in resourceErrorMap)`, ToolResults passes `resourceError?.[item.uri]` (possibly undefined) through to ResourceLinkView, and ResourceLinkView renders the error branch when `resourceError !== undefined`. 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.
The `in` operator also matches inherited properties like `toString` and `constructor`, which would cause false cache hits if a server ever returns a URI with one of those names. Switch the cache-hit guard and the error-map cleanup to Object.prototype.hasOwnProperty.call so only real cached entries match. Object.hasOwn would be cleaner but requires ES2022 lib support which the current tsconfig doesn't include. 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 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setResourceContent(resourceContentMap[uri]); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Because resourceErrorMap[uri] is only cleared on success, any retry attempt will continue to have an error entry until the request completes. Since some UI paths render resourceError in preference to cached content, this can leave a stale error visible while the retry is in-flight. Consider clearing resourceErrorMap[uri] when starting a new read (after passing the cache check / before sending the request), not only when a read succeeds.
| setResourceErrorMap((prev) => { | |
| if (!hasOwn.call(prev, uri)) return prev; | |
| const next = { ...prev }; | |
| delete next[uri]; | |
| return next; | |
| }); |
Previously, resourceErrorMap[uri] was only cleared when a read succeeded. During an in-flight retry (e.g., after clicking Refresh on a previously-failed resource), the stale error would remain in the map and render in ResourceLinkView while the retry was still pending. Clear the error entry after the cache-hit check passes, right before sending the request, so the UI shows the last-known-good content (or a loading state) during the retry instead of the prior error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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