setupTab loading spinner by Kitenite · Pull Request #156 · superset-sh/superset

Kitenite · 2025-11-27T01:43:40Z

Summary by CodeRabbit

Bug Fixes
- Fixed setup tab completion and auto-close behavior to function properly across workspace tabs, even when not actively displayed.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-27T01:43:48Z

Walkthrough

The PR modifies the TabsContent component to render all Setup-type tabs in the DOM simultaneously, keeping them mounted but hidden when inactive, instead of rendering only the active tab. A memoized filter collects Setup tabs matching the current workspace, and each tab is conditionally displayed based on whether its ID matches the active tab ID.

Changes

Cohort / File(s)	Summary
TabsContent Rendering Refactor `apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx`	Introduces memoized filter to collect all Setup-type tabs by workspace ID; replaces single active tab rendering with all setup tabs wrapped in conditionally hidden containers; adds comments documenting the mounted-tabs pattern to preserve completion and auto-close behavior

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Single file modified with localized, consistent logic changes
Memoization pattern is straightforward and follows established React patterns
Conditional rendering logic is clear and easy to validate
Primary concern: verify memoization dependency array is correct to avoid stale filters

Possibly related PRs

fix worktree setup #146: Introduces SetupTabView handling for the active tab; this PR extends that foundation by rendering all setup tabs while keeping them mounted.
conditional render #152: Previously simplified to single-tab rendering; this PR reverses that approach by rendering all Setup-type tabs with conditional visibility.
keep terminal tabs rendered #147: Also implements a kept-mounted pattern for non-active tabs to preserve UI state and behavior across tab switches.

Poem

🐰 Tabs now dance in DOM's delight,
All mounted, some hidden from sight,
Setup tabs complete their dance,
Auto-close gets its second chance, 🎭
Performance and behavior, just right!

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Description check	⚠️ Warning	The pull request has no description provided by the author, missing all required sections from the template.	Add a comprehensive pull request description following the template, including: clear description of changes, related issue links, type of change classification, testing details, and any additional context needed for review.
Title check	❓ Inconclusive	The title 'setupTab loading spinner' is too vague and does not clearly describe the actual changes made to the file. The PR actually modifies how setup tabs are rendered and kept mounted, but the title only mentions a loading spinner which is not the primary focus of the changes.	Consider a more descriptive title such as 'Keep setup tabs mounted for completion and auto-close behavior' or 'Render all setup tabs while keeping inactive ones hidden' to better reflect the core changes.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch jade-canyon-28

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (2)
34-41: Setup tabs filtering logic is correct.

The memoization correctly filters setup tabs by workspace. The type predicate combines type narrowing with workspace filtering, which is functionally correct and concise.

If you prefer a stricter separation of concerns, you could split the type predicate from the workspace filter:
-	const setupTabs = useMemo(() => {
-		if (!activeWorkspaceId) return [];
-		return allTabs.filter(
-			(tab): tab is SetupTab =>
-				tab.type === TabType.Setup && tab.workspaceId === activeWorkspaceId,
-		);
-	}, [allTabs, activeWorkspaceId]);
+	const setupTabs = useMemo(() => {
+		if (!activeWorkspaceId) return [];
+		return allTabs
+			.filter((tab): tab is SetupTab => tab.type === TabType.Setup)
+			.filter(tab => tab.workspaceId === activeWorkspaceId);
+	}, [allTabs, activeWorkspaceId]);
This makes the type predicate purely about type narrowing, with workspace filtering as a separate step. However, the current implementation is acceptable and more concise.

49-54: Hidden setup tabs in empty state work as designed.

The logic correctly keeps setup tabs mounted (but hidden) when there's no active tab, allowing them to complete and auto-close as intended.

For improved accessibility, consider adding aria-hidden="true" to hidden setup tabs so they're excluded from the accessibility tree:
-				{setupTabs.map((tab) => (
-					<div key={tab.id} className="hidden">
-						<SetupTabView tab={tab} />
-					</div>
-				))}
+				{setupTabs.map((tab) => (
+					<div key={tab.id} className="hidden" aria-hidden="true">
+						<SetupTabView tab={tab} />
+					</div>
+				))}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c1c25e and dec1638.

📒 Files selected for processing (1)

apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (2 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

**/*.{ts,tsx,js,jsx,json}