-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix resource refresh button not triggering re-fetch #1148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
234b209
3054236
5a2895a
6976069
54d86b4
4076036
42c7a01
82f7104
d15a525
d02e6c1
ffde8cb
c593fb8
01ea4cb
28e5d13
f879c0d
fca508c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -151,6 +151,9 @@ const App = () => { | |||||||||||||||||
| const [resourceContentMap, setResourceContentMap] = useState< | ||||||||||||||||||
| Record<string, string> | ||||||||||||||||||
| >({}); | ||||||||||||||||||
| const [resourceErrorMap, setResourceErrorMap] = useState< | ||||||||||||||||||
| Record<string, string> | ||||||||||||||||||
| >({}); | ||||||||||||||||||
| const [fetchingResources, setFetchingResources] = useState<Set<string>>( | ||||||||||||||||||
| new Set(), | ||||||||||||||||||
| ); | ||||||||||||||||||
|
|
@@ -902,8 +905,20 @@ const App = () => { | |||||||||||||||||
| setPromptContent(JSON.stringify(response, null, 2)); | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| const readResource = async (uri: string) => { | ||||||||||||||||||
| if (fetchingResources.has(uri) || resourceContentMap[uri]) { | ||||||||||||||||||
| const readResource = async ( | ||||||||||||||||||
| uri: string, | ||||||||||||||||||
| { bypassCache = false }: { bypassCache?: boolean } = {}, | ||||||||||||||||||
| ) => { | ||||||||||||||||||
| if (fetchingResources.has(uri)) { | ||||||||||||||||||
| return; | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+908
to
+914
|
||||||||||||||||||
|
|
||||||||||||||||||
| if ( | ||||||||||||||||||
| !bypassCache && | ||||||||||||||||||
| uri in resourceContentMap && | ||||||||||||||||||
| !(uri in resourceErrorMap) | ||||||||||||||||||
| ) { | ||||||||||||||||||
| setResourceContent(resourceContentMap[uri]); | ||||||||||||||||||
|
olaservo marked this conversation as resolved.
Outdated
|
||||||||||||||||||
| setResourceContent(resourceContentMap[uri]); | |
| if (currentTabRef.current === "resources") { | |
| setResourceContent(resourceContentMap[uri]); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in fca508c — guarded the cache-hit setResourceContent call with currentTabRef.current === "resources" so expanding a resource_link in the Tools tab no longer overwrites the Resources tab's displayed content.
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 01ea4cb — moved resourceErrorMap[uri] clearing from the success handler to right after the cache-hit check (before sending the request). During an in-flight retry, the UI now shows last-known-good content instead of the stale error.
Copilot
AI
Apr 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorString uses nullish coalescing (??), so an Error with an empty message (e.g. new Error("")) will produce an empty string and render a blank error. Prefer a fallback that treats empty messages as missing (e.g. use || and/or include error.name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f879c0d — switched from ?? to || so an empty .message falls through to String(error), which always produces a non-empty representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new cache-bypass and error-not-cached behavior in
readResourceis central to fixing #1120, but it’s currently only covered indirectly viaResourcesTabprop tests (it doesn’t assert that App actually re-fetches when cached, nor that errors aren’t cached). Consider adding an App-level unit/integration test that verifies: (1) cache hits syncresourceContentwithout callingmakeRequest, (2){ bypassCache: true }forces a re-fetch even when cached, and (3) failed reads don’t populateresourceContentMapso a subsequent click retries.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid suggestion. The ResourcesTab prop tests verify that the correct arguments (
bypassCache: truefor Refresh, no options for normal clicks) are passed, but App-level integration tests verifying the actual cache/retry behavior would add more confidence. This would require mockingsendMCPRequestand the full App rendering context — better suited as a follow-up rather than this bug fix PR.