Fix resource refresh button not triggering re-fetch (#1148)
* Fix resource refresh button not triggering re-fetch (#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> * fix: don't cache failed resource reads and rename force to bypassCache 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> * test: remove flaky resource-refresh e2e test 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> * fix: invalidate stale cache on refresh failure and rename opts to options 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> * fix: surface read errors for resource_link in Tool results 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 #1120. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: preserve cached content on refresh failure and stop writing error 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> * fix: use presence checks instead of truthiness for resource error state 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> * fix: use hasOwnProperty instead of `in` for resource cache checks 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> * fix: clear resource error on read start, not only on success 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> * fix: use || instead of ?? for error message fallback The ?? operator only falls back on null/undefined, so an Error with an empty message (new Error("")) would produce an empty string and render a blank error block. Switch to || so empty strings also fall through to String(error), which produces a non-empty representation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: only sync resourceContent on cache hit from Resources tab The cache-hit path called setResourceContent unconditionally, which meant expanding a resource_link in the Tools tab could overwrite the Resources tab's displayed content without updating selectedResource. Guard the setResourceContent call with a currentTabRef check so only cache hits from the Resources tab update the display. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ola Hungerford
GitHub
parent
a523a0f15a
commit
638f2bfa5b
6 changed files with 92 additions and 15 deletions
|
|
@ -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(),
|
||||
);
|
||||
|
|
@ -914,13 +917,34 @@ 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;
|
||||
}
|
||||
|
||||
const hasOwn = Object.prototype.hasOwnProperty;
|
||||
if (
|
||||
!bypassCache &&
|
||||
hasOwn.call(resourceContentMap, uri) &&
|
||||
!hasOwn.call(resourceErrorMap, uri)
|
||||
) {
|
||||
if (currentTabRef.current === "resources") {
|
||||
setResourceContent(resourceContentMap[uri]);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
console.log("[App] Reading resource:", uri);
|
||||
setFetchingResources((prev) => new Set(prev).add(uri));
|
||||
setResourceErrorMap((prev) => {
|
||||
if (!hasOwn.call(prev, uri)) return prev;
|
||||
const next = { ...prev };
|
||||
delete next[uri];
|
||||
return next;
|
||||
});
|
||||
lastToolCallOriginTabRef.current = currentTabRef.current;
|
||||
|
||||
try {
|
||||
|
|
@ -945,10 +969,10 @@ const App = () => {
|
|||
}));
|
||||
} catch (error) {
|
||||
console.error(`[App] Failed to read resource ${uri}:`, error);
|
||||
const errorString = (error as Error).message ?? String(error);
|
||||
setResourceContentMap((prev) => ({
|
||||
const errorString = (error as Error).message || String(error);
|
||||
setResourceErrorMap((prev) => ({
|
||||
...prev,
|
||||
[uri]: JSON.stringify({ error: errorString }),
|
||||
[uri]: errorString,
|
||||
}));
|
||||
} finally {
|
||||
setFetchingResources((prev) => {
|
||||
|
|
@ -1523,9 +1547,9 @@ const App = () => {
|
|||
setResourceTemplates([]);
|
||||
setNextResourceTemplateCursor(undefined);
|
||||
}}
|
||||
readResource={(uri) => {
|
||||
readResource={(uri, options) => {
|
||||
clearError("resources");
|
||||
readResource(uri);
|
||||
readResource(uri, options);
|
||||
}}
|
||||
selectedResource={selectedResource}
|
||||
setSelectedResource={(resource) => {
|
||||
|
|
@ -1631,6 +1655,7 @@ const App = () => {
|
|||
nextCursor={nextToolCursor}
|
||||
error={errors.tools}
|
||||
resourceContent={resourceContentMap}
|
||||
resourceError={resourceErrorMap}
|
||||
onReadResource={(uri: string) => {
|
||||
clearError("resources");
|
||||
readResource(uri);
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ interface ResourceLinkViewProps {
|
|||
description?: string;
|
||||
mimeType?: string;
|
||||
resourceContent: string;
|
||||
resourceError?: string;
|
||||
onReadResource?: (uri: string) => void;
|
||||
}
|
||||
|
||||
|
|
@ -17,6 +18,7 @@ const ResourceLinkView = memo(
|
|||
description,
|
||||
mimeType,
|
||||
resourceContent,
|
||||
resourceError,
|
||||
onReadResource,
|
||||
}: ResourceLinkViewProps) => {
|
||||
const [{ expanded, loading }, setState] = useState({
|
||||
|
|
@ -24,18 +26,32 @@ const ResourceLinkView = memo(
|
|||
loading: false,
|
||||
});
|
||||
|
||||
const expandedContent = useMemo(
|
||||
() =>
|
||||
expanded && resourceContent ? (
|
||||
const expandedContent = useMemo(() => {
|
||||
if (!expanded) return null;
|
||||
if (resourceError !== undefined) {
|
||||
return (
|
||||
<div className="mt-2">
|
||||
<div className="flex justify-between items-center mb-1">
|
||||
<span className="font-semibold text-red-600">Error:</span>
|
||||
</div>
|
||||
<div className="text-sm text-red-600 dark:text-red-400 bg-red-50 dark:bg-red-950/30 p-2 rounded break-all">
|
||||
{resourceError}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
if (resourceContent) {
|
||||
return (
|
||||
<div className="mt-2">
|
||||
<div className="flex justify-between items-center mb-1">
|
||||
<span className="font-semibold text-green-600">Resource:</span>
|
||||
</div>
|
||||
<JsonView data={resourceContent} className="bg-background" />
|
||||
</div>
|
||||
) : null,
|
||||
[expanded, resourceContent],
|
||||
);
|
||||
);
|
||||
}
|
||||
return null;
|
||||
}, [expanded, resourceContent, resourceError]);
|
||||
|
||||
const handleClick = useCallback(() => {
|
||||
if (!onReadResource) return;
|
||||
|
|
|
|||
|
|
@ -46,7 +46,7 @@ const ResourcesTab = ({
|
|||
clearResources: () => void;
|
||||
listResourceTemplates: () => void;
|
||||
clearResourceTemplates: () => void;
|
||||
readResource: (uri: string) => void;
|
||||
readResource: (uri: string, opts?: { bypassCache?: boolean }) => void;
|
||||
selectedResource: Resource | null;
|
||||
setSelectedResource: (resource: Resource | null) => void;
|
||||
handleCompletion: (
|
||||
|
|
@ -229,7 +229,9 @@ const ResourcesTab = ({
|
|||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={() => readResource(selectedResource.uri)}
|
||||
onClick={() =>
|
||||
readResource(selectedResource.uri, { bypassCache: true })
|
||||
}
|
||||
>
|
||||
<RefreshCw className="w-4 h-4 mr-2" />
|
||||
Refresh
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ interface ToolResultsProps {
|
|||
toolResult: CompatibilityCallToolResult | null;
|
||||
selectedTool: Tool | null;
|
||||
resourceContent: Record<string, string>;
|
||||
resourceError?: Record<string, string>;
|
||||
onReadResource?: (uri: string) => void;
|
||||
isPollingTask?: boolean;
|
||||
}
|
||||
|
|
@ -63,6 +64,7 @@ const ToolResults = ({
|
|||
toolResult,
|
||||
selectedTool,
|
||||
resourceContent,
|
||||
resourceError,
|
||||
onReadResource,
|
||||
isPollingTask,
|
||||
}: ToolResultsProps) => {
|
||||
|
|
@ -228,6 +230,7 @@ const ToolResults = ({
|
|||
description={item.description}
|
||||
mimeType={item.mimeType}
|
||||
resourceContent={resourceContent[item.uri] || ""}
|
||||
resourceError={resourceError?.[item.uri]}
|
||||
onReadResource={onReadResource}
|
||||
/>
|
||||
)}
|
||||
|
|
|
|||
|
|
@ -176,6 +176,7 @@ const ToolsTab = ({
|
|||
nextCursor,
|
||||
error,
|
||||
resourceContent,
|
||||
resourceError,
|
||||
onReadResource,
|
||||
serverSupportsTaskRequests,
|
||||
}: {
|
||||
|
|
@ -195,6 +196,7 @@ const ToolsTab = ({
|
|||
nextCursor: ListToolsResult["nextCursor"];
|
||||
error: string | null;
|
||||
resourceContent: Record<string, string>;
|
||||
resourceError?: Record<string, string>;
|
||||
onReadResource?: (uri: string) => void;
|
||||
serverSupportsTaskRequests: boolean;
|
||||
}) => {
|
||||
|
|
@ -884,6 +886,7 @@ const ToolsTab = ({
|
|||
toolResult={toolResult}
|
||||
selectedTool={selectedTool}
|
||||
resourceContent={resourceContent}
|
||||
resourceError={resourceError}
|
||||
onReadResource={onReadResource}
|
||||
isPollingTask={isPollingTask}
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -305,4 +305,32 @@ describe("ResourcesTab - Template Query Parameters", () => {
|
|||
),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should call readResource with bypassCache: true when Refresh is clicked", () => {
|
||||
renderResourcesTab({
|
||||
selectedResource: mockResource,
|
||||
resourceContent: '{"users": []}',
|
||||
});
|
||||
|
||||
const refreshButton = screen.getByText("Refresh");
|
||||
fireEvent.click(refreshButton);
|
||||
|
||||
expect(mockReadResource).toHaveBeenCalledWith(mockResource.uri, {
|
||||
bypassCache: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("should call readResource without bypassCache when selecting a resource from the list", () => {
|
||||
renderResourcesTab({
|
||||
resources: [mockResource],
|
||||
});
|
||||
|
||||
fireEvent.click(screen.getByText("Users Resource"));
|
||||
|
||||
expect(mockReadResource).toHaveBeenCalledWith(mockResource.uri);
|
||||
expect(mockReadResource).not.toHaveBeenCalledWith(
|
||||
mockResource.uri,
|
||||
expect.objectContaining({ bypassCache: true }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue