-
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 7 commits
234b209
3054236
5a2895a
6976069
54d86b4
4076036
42c7a01
82f7104
d15a525
d02e6c1
ffde8cb
c593fb8
01ea4cb
28e5d13
f879c0d
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,16 @@ 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 && resourceContentMap[uri]) { | ||||||||||||||||||
| setResourceContent(resourceContentMap[uri]); | ||||||||||||||||||
|
olaservo marked this conversation as resolved.
|
||||||||||||||||||
| return; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
||||||||||||||||||
| 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.