Skip to content
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
234b209
Fix resource refresh button not triggering re-fetch (#1120)
olaservo Mar 15, 2026
3054236
fix: don't cache failed resource reads and rename force to bypassCache
olaservo Apr 12, 2026
5a2895a
Merge branch 'main' into feature/fix-resource-refresh-button-1120
olaservo Apr 13, 2026
6976069
test: remove flaky resource-refresh e2e test
olaservo Apr 13, 2026
54d86b4
fix: invalidate stale cache on refresh failure and rename opts to opt…
olaservo Apr 13, 2026
4076036
Merge branch 'main' into feature/fix-resource-refresh-button-1120
olaservo Apr 14, 2026
42c7a01
fix: surface read errors for resource_link in Tool results
olaservo Apr 14, 2026
82f7104
fix: preserve cached content on refresh failure and stop writing erro…
olaservo Apr 14, 2026
d15a525
fix: use presence checks instead of truthiness for resource error state
olaservo Apr 14, 2026
d02e6c1
fix: use hasOwnProperty instead of `in` for resource cache checks
olaservo Apr 14, 2026
ffde8cb
Merge branch 'main' into feature/fix-resource-refresh-button-1120
olaservo Apr 14, 2026
c593fb8
Merge branch 'main' into feature/fix-resource-refresh-button-1120
olaservo Apr 14, 2026
01ea4cb
fix: clear resource error on read start, not only on success
olaservo Apr 14, 2026
28e5d13
Merge branch 'main' into feature/fix-resource-refresh-button-1120
olaservo Apr 15, 2026
f879c0d
fix: use || instead of ?? for error message fallback
olaservo Apr 16, 2026
fca508c
fix: only sync resourceContent on cache hit from Resources tab
olaservo Apr 16, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions client/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
);
Expand Down Expand Up @@ -902,13 +905,32 @@ 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)) {
Comment on lines +908 to +912
Copy link

Copilot AI Apr 16, 2026

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 readResource is central to fixing #1120, but it’s currently only covered indirectly via ResourcesTab prop 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 sync resourceContent without calling makeRequest, (2) { bypassCache: true } forces a re-fetch even when cached, and (3) failed reads don’t populate resourceContentMap so a subsequent click retries.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

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: true for 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 mocking sendMCPRequest and the full App rendering context — better suited as a follow-up rather than this bug fix PR.

return;
}
Comment on lines +908 to +914
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchingResources is React state, so fetchingResources.has(uri) can be stale if readResource is triggered twice before the state update is applied (e.g., double-click Refresh / rapid re-select). That can lead to duplicate resources/read requests for the same URI. Consider tracking in-flight URIs in a useRef (or similar) so the guard is synchronous and cannot race with state updates.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid observation, but this is a pre-existing issue — fetchingResources was already React state before this PR. The race condition on rapid clicks applies to the original code as well. A useRef-based guard would be a good improvement, but it's out of scope for this bug fix and should be addressed in a separate PR.


const hasOwn = Object.prototype.hasOwnProperty;
if (
!bypassCache &&
hasOwn.call(resourceContentMap, uri) &&
!hasOwn.call(resourceErrorMap, uri)
) {
setResourceContent(resourceContentMap[uri]);
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readResource now calls setResourceContent(resourceContentMap[uri]) on a cache hit. When readResource is triggered from non-Resources contexts (e.g. Tools tab expanding a resource_link), this will update the global resourceContent string without updating selectedResource, so the Resources tab can later display content that doesn’t match the selected resource header. Consider only syncing resourceContent on cache hits when the current tab is resources (or when selectedResource?.uri === uri), and otherwise just return without touching resourceContent.

Suggested change
setResourceContent(resourceContentMap[uri]);
if (currentTabRef.current === "resources") {
setResourceContent(resourceContentMap[uri]);
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

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.

return;
}

Copy link

Copilot AI Apr 14, 2026

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.

Suggested change
setResourceErrorMap((prev) => {
if (!hasOwn.call(prev, uri)) return prev;
const next = { ...prev };
delete next[uri];
return next;
});

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

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.

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 {
Expand All @@ -934,9 +956,9 @@ const App = () => {
} catch (error) {
console.error(`[App] Failed to read resource ${uri}:`, error);
const errorString = (error as Error).message ?? String(error);
setResourceContentMap((prev) => ({
setResourceErrorMap((prev) => ({
...prev,
[uri]: JSON.stringify({ error: errorString }),
[uri]: errorString,
}));
Comment on lines 959 to 964
Copy link

Copilot AI Apr 15, 2026

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).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

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.

} finally {
setFetchingResources((prev) => {
Expand Down Expand Up @@ -1482,9 +1504,9 @@ const App = () => {
setResourceTemplates([]);
setNextResourceTemplateCursor(undefined);
}}
readResource={(uri) => {
readResource={(uri, options) => {
clearError("resources");
readResource(uri);
readResource(uri, options);
}}
selectedResource={selectedResource}
setSelectedResource={(resource) => {
Expand Down Expand Up @@ -1590,6 +1612,7 @@ const App = () => {
nextCursor={nextToolCursor}
error={errors.tools}
resourceContent={resourceContentMap}
resourceError={resourceErrorMap}
onReadResource={(uri: string) => {
clearError("resources");
readResource(uri);
Expand Down
28 changes: 22 additions & 6 deletions client/src/components/ResourceLinkView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ interface ResourceLinkViewProps {
description?: string;
mimeType?: string;
resourceContent: string;
resourceError?: string;
onReadResource?: (uri: string) => void;
}

Expand All @@ -17,25 +18,40 @@ const ResourceLinkView = memo(
description,
mimeType,
resourceContent,
resourceError,
onReadResource,
}: ResourceLinkViewProps) => {
const [{ expanded, loading }, setState] = useState({
expanded: false,
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 (
Comment thread
olaservo marked this conversation as resolved.
<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;
Expand Down
6 changes: 4 additions & 2 deletions client/src/components/ResourcesTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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: (
Expand Down Expand Up @@ -229,7 +229,9 @@ const ResourcesTab = ({
<Button
variant="outline"
size="sm"
onClick={() => readResource(selectedResource.uri)}
onClick={() =>
readResource(selectedResource.uri, { bypassCache: true })
}
Comment thread
olaservo marked this conversation as resolved.
>
<RefreshCw className="w-4 h-4 mr-2" />
Refresh
Expand Down
3 changes: 3 additions & 0 deletions client/src/components/ToolResults.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -63,6 +64,7 @@ const ToolResults = ({
toolResult,
selectedTool,
resourceContent,
resourceError,
onReadResource,
isPollingTask,
}: ToolResultsProps) => {
Expand Down Expand Up @@ -228,6 +230,7 @@ const ToolResults = ({
description={item.description}
mimeType={item.mimeType}
resourceContent={resourceContent[item.uri] || ""}
resourceError={resourceError?.[item.uri]}
onReadResource={onReadResource}
/>
)}
Expand Down
3 changes: 3 additions & 0 deletions client/src/components/ToolsTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ const ToolsTab = ({
nextCursor,
error,
resourceContent,
resourceError,
onReadResource,
serverSupportsTaskRequests,
}: {
Expand All @@ -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;
}) => {
Expand Down Expand Up @@ -884,6 +886,7 @@ const ToolsTab = ({
toolResult={toolResult}
selectedTool={selectedTool}
resourceContent={resourceContent}
resourceError={resourceError}
onReadResource={onReadResource}
isPollingTask={isPollingTask}
/>
Expand Down
28 changes: 28 additions & 0 deletions client/src/components/__tests__/ResourcesTab.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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