fix: pass aggregatedCounts only when all component queries succeed - #96
Conversation
Signed-off-by: Marek Dano <mk.dano@gmail.com>
There was a problem hiding this comment.
Nice work! LLM-assisted feedback from testing/analysis:
Moving the aggregate onto fetchedComponents looks right, since the fallback items carry no enabled field and were never comparable to a handshake that only sees enabled components. Couple of things worth sorting before merge.
The one-query-fails test passes without the fix. With VirtualServerDetailsPanel.tsx reverted to the parent commit, suppresses the mismatch banner when one component query fails still passes. The test's handshake returns componentCounts: { tools: 1 }, and in getCountMismatchKeys the missing resources key resolves through componentCounts?.[key] ?? 0 to 0, matching the aggregate's 0. Giving the handshake a count for the failed type makes it fail before and pass after:
- componentCounts: { tools: 1 },
+ componentCounts: { tools: 1, resources: 2 },The all-queries-fail test does catch the regression as written.
The banner also still shows while a query is in flight. The aggregate is now built only from fetchedComponents, so before those resolve it is { tools: 0, resources: 0, prompts: 0 }, and that gets compared. The buildComponentItems fallback used to fill that window from the server prop. Repro: tools query delayed, resources and prompts empty, handshake returning { tools: 1 }, server with one associated tool. No banner before, banner after. Worst in the case the PR targets, since a backend that hangs rather than 500s holds isLoading for the full 30s useQuery timeout while the handshake returns much sooner. Nothing else covers the window; componentsLoading is only read by the Components tab.
A not-yet-loaded guard next to the error guard clears it, with toolsData, resourcesData, promptsData added to the dep array:
const aggregatedComponentCounts = useMemo(() => {
if (toolsError || resourcesError || promptsError) return undefined;
+ if (!toolsData || !resourcesData || !promptsData) return undefined;
const counts: Record<string, number> = { tools: 0, resources: 0, prompts: 0 };Keep both guards, since useQuery never clears data and a query failing after an earlier success still has data set. With that, VirtualServerDetailsPanel.test.tsx and HandshakeTestPanel.test.tsx both pass, including the mismatch cases.
Signed-off-by: Marek Dano <mk.dano@gmail.com>
There was a problem hiding this comment.
Approving ✅
Optional, non-blocking LLM-review suggestion:
-->
The loading-window fix has no test of its own. It's the one behaviour in this PR that was found in review, fixed in code, and left uncovered. If you want it, the repro I used is a straight copy of the existing test shape with await delay(3000) on the tools handler and empty resources/prompts.
Signed-off-by: Marek Dano <mk.dano@gmail.com>
Closes #6507
Summary
The virtual server drawer's Test Connection tab cross-checks handshake component counts against a locally-built aggregate, and shows a mismatch banner when they disagree. Two paths let that aggregate be built from data that isn't comparable to what the handshake sees, producing false-positive banners:
A single failed query silently contributed 0. The aggregate was summed over whichever of the three
/servers/{id}/{tools, resources, prompts}that isn't comparable to what the handshake sees, producing false-positive banners:A single failed query silently contributed 0. The aggregate was summed over whichever of the three
/servers/{id}/{tools, resources, prompts}queries returned data. If one query failed (permission denial, backend error) while the others succeeded, it contributed0to the aggregate instead of being excluded, firing a spurious mismatch.The fallback population couldn't be filtered. When all three queries failed or returned empty,
allComponentsfell back tobuildComponentItems(server), built from theassociatedTools/associatedResources/associatedPromptsname lists. Those items carry noenabledfield, so theenabled === falsefilter could never fire and disabled components got double-counted against a handshake that only ever sees enabled ones.Fix
errorfrom each of the threeuseQuerycalls (tools/resources/prompts).aggregatedComponentCountsnow returnsundefinedwhen any of the three queries errored, instead of silently treating a failed query as0.HandshakeTestPanelalready handles an absent aggregate:hasComparisongoesfalseand the comparison column/banner drop out.fetchedComponentsonly, never thebuildComponentItemsfallback, so theenabled === falsefilter always has a field to check.Testing