Preserve state between tab switches#1540
Conversation
dosubot
Bot
added
the
size:L
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughIntroduces visit tracking in App: a visitedTabs Set initialized with "servers" and updated when activeTab changes. Tab panes (tools, resources, prompts, app-builder) are wrapped to stay hidden until first visit but preserve their rendered state thereafter. Adds tests verifying that Tools, Resources, and Prompts retain local state across tab switches. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/__tests__/App.keepalive.test.tsx (1)
371-383: This doesn’t actually proveAppBuilderTabis kept alive.The test uses App Builder only as a waypoint. Since it never writes to
app-builder-input, a remount bug inAppBuilderTabwould still pass. Add a symmetric assertion for App Builder’s own local state.Suggested test extension
fireEvent.click(screen.getByRole("button", { name: "app-builder" })); - expect(screen.getByTestId("app-builder-tab")).toBeInTheDocument(); + fireEvent.change(screen.getByLabelText("app-builder-input"), { + target: { value: "draft=weather-widget" }, + }); fireEvent.click(screen.getByRole("button", { name: "tools" })); expect(screen.getByLabelText("tools-input")).toHaveValue("weather=nyc"); + + fireEvent.click(screen.getByRole("button", { name: "app-builder" })); + expect(screen.getByLabelText("app-builder-input")).toHaveValue( + "draft=weather-widget", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/__tests__/App.keepalive.test.tsx` around lines 371 - 383, The test "preserves Tools state when switching to App Builder and back" doesn't verify AppBuilderTab's local state; update the test to also write to and assert App Builder's input so the tab is actually exercised and would fail on a remount: after clicking the "app-builder" button and asserting app-builder-tab is shown, find the "app-builder-input" (or the input controlled by AppBuilderTab), fireEvent.change to set a value (e.g., "layout=grid"), then switch back to "tools" and assert tools-input retains "weather=nyc", then switch again to "app-builder" and assert app-builder-input still has "layout=grid" to confirm AppBuilderTab is kept alive (refer to App, AppBuilderTab, tools-input, app-builder-input, and the test case name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/__tests__/App.keepalive.test.tsx`:
- Around line 61-116: The test currently hardcodes a custom useAppState mock
that will drift from the real store; replace this bespoke fixture by importing
and applying the shared test presets (mcpApiPresets and storePresets) from the
repo's test mocks and use the provided helper to mount the client test
environment instead of manually mocking useAppState; locate the
vi.mock("../hooks/use-app-state", ...) block and remove it, then update the test
setup to call the shared mock preset utilities so the test uses the standardized
store and API mocks (refer to mcpApiPresets, storePresets and the shared test
mock helper functions).
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 379-386: The flicker happens because tab content is gated only by
visitedTabs which is updated in a post-render effect; update each render guard
to allow immediate render when the tab is currently active by changing checks to
"activeTab === <TAB_ID> || visitedTabs.has(<TAB_ID>)" (apply this wherever you
currently use visitedTabs.has(...) for tab content—e.g. the guards around the
components referenced at lines ~515–525, ~537–547, ~550–560, ~605–615); keep the
existing useEffect that adds to visitedTabs (useEffect, visitedTabs, activeTab)
but add the activeTab short-circuit to eliminate the one-frame empty panel.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/__tests__/App.keepalive.test.tsx`:
- Around line 371-383: The test "preserves Tools state when switching to App
Builder and back" doesn't verify AppBuilderTab's local state; update the test to
also write to and assert App Builder's input so the tab is actually exercised
and would fail on a remount: after clicking the "app-builder" button and
asserting app-builder-tab is shown, find the "app-builder-input" (or the input
controlled by AppBuilderTab), fireEvent.change to set a value (e.g.,
"layout=grid"), then switch back to "tools" and assert tools-input retains
"weather=nyc", then switch again to "app-builder" and assert app-builder-input
still has "layout=grid" to confirm AppBuilderTab is kept alive (refer to App,
AppBuilderTab, tools-input, app-builder-input, and the test case name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8766d4bb-625a-4ee2-8895-8ce2ddef4e46
📒 Files selected for processing (2)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.keepalive.test.tsx
hide tabs vs destroying them
mitchcapper
force-pushed
the
feat_preserve_state_between_tab_switches
branch
from
75201c6 to
29afa91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 515-525: The mounted-but-hidden tabs still get live
serverConfig/serverName updates and trigger resets/fetches in ToolsTab,
ResourcesTab, and AppBuilderTab; stop this by freezing per-tab server props
while inactive or by passing an explicit isActive flag so those components only
respond when visible. Update the tab rendering (where ToolsTab, ResourcesTab,
AppBuilderTab are instantiated) to supply either a frozen snapshot of
serverConfig/serverName (store the snapshot when tab becomes inactive/active) or
add isActive={activeTab === "tools"/"resources"/"appbuilder"} and then change
the components’ effects (the reset/fetch logic in ToolsTab lines ~185-203,
ResourcesTab ~114-121, AppBuilderTab ~166-172) to guard on isActive before
running; ensure visitedTabs logic still controls mounting but does not allow
hidden tabs to receive live server prop updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e4ff9a7-14c2-49ac-82c8-2bfc880f50f8
📒 Files selected for processing (2)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.keepalive.test.tsx
| <div | ||
| className={ | ||
| activeTab === "tools" ? "h-full overflow-hidden" : "hidden" | ||
| } | ||
| > | ||
| {(activeTab === "tools" || visitedTabs.has("tools")) && ( | ||
| <ToolsTab | ||
| serverConfig={selectedMCPConfig} | ||
| serverName={appState.selectedServer} | ||
| /> | ||
| </div> | ||
| )} | ||
| )} |
There was a problem hiding this comment.
Keep-alive still breaks on server changes and reconnects.
These panes stay mounted, but they still receive live serverConfig / serverName updates while hidden. At least ToolsTab clears local state on those deps in mcpjam-inspector/client/src/components/ToolsTab.tsx:185-203, ResourcesTab refetches on them in mcpjam-inspector/client/src/components/ResourcesTab.tsx:114-121, and AppBuilderTab resets on them in mcpjam-inspector/client/src/components/ui-playground/AppBuilderTab.tsx:166-172. So a server switch or reconnect can still wipe the very state this PR is trying to preserve. Consider freezing per-tab server props while inactive, or passing an isActive flag so those reset/fetch effects only run for the visible tab.
Also applies to: 537-547, 550-560, 605-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/App.tsx` around lines 515 - 525, The
mounted-but-hidden tabs still get live serverConfig/serverName updates and
trigger resets/fetches in ToolsTab, ResourcesTab, and AppBuilderTab; stop this
by freezing per-tab server props while inactive or by passing an explicit
isActive flag so those components only respond when visible. Update the tab
rendering (where ToolsTab, ResourcesTab, AppBuilderTab are instantiated) to
supply either a frozen snapshot of serverConfig/serverName (store the snapshot
when tab becomes inactive/active) or add isActive={activeTab ===
"tools"/"resources"/"appbuilder"} and then change the components’ effects (the
reset/fetch logic in ToolsTab lines ~185-203, ResourcesTab ~114-121,
AppBuilderTab ~166-172) to guard on isActive before running; ensure visitedTabs
logic still controls mounting but does not allow hidden tabs to receive live
server prop updates.
|
hey @mitchcapper, can you provide a screen recording of what this would look like and evaluate coderabbit's review? |
|
Hey @mitchcapper, thanks for the PR — preserving tab state is definitely something we want to improve. We're planning to tackle this with Zustand (lifting tab state into a store) rather than the CSS keep-alive approach. Appreciate you flagging this as a UX painpoint! |
|
ok I had revised to freezing but yeah something here in the future is a plus. |
3 participants
I thought I addressed this previously but maybe I just considered it back when adding server reconnect. This prevents the fields and state of a tab from being lost just switching between them.