Skip to content

perf(desktop): fix N+1 query in getAllGrouped workspace procedure#688

Merged
Kitenite merged 1 commit intomainfrom
fix-workspace-concerns
Jan 9, 2026
Merged

perf(desktop): fix N+1 query in getAllGrouped workspace procedure#688
Kitenite merged 1 commit intomainfrom
fix-workspace-concerns

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Jan 9, 2026

Summary

  • Preload all worktrees in a single query at the start of getAllGrouped
  • Use already-loaded project data from groupsMap for branch workspace paths
  • Resolve worktree paths from the preloaded map instead of per-workspace DB queries

Performance improvement:

  • Before: 2 + N queries (where N = number of workspaces)
  • After: 3 queries (constant, regardless of workspace count)

Test plan

  • Verify workspaces load correctly in the sidebar
  • Confirm worktree paths display properly for worktree-type workspaces
  • Confirm main repo paths display properly for branch-type workspaces
  • Run typecheck: bun run typecheck --filter=@superset/desktop

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Improved workspace loading performance through optimized data handling and lookup efficiency.

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

Preload all worktrees once and use already-loaded project data from
groupsMap instead of calling getWorkspacePath per workspace, which was
causing N additional DB queries (one per workspace).

Before: 2 + N queries (where N = number of workspaces)
After: 3 queries (constant)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 9, 2026

📝 Walkthrough

Walkthrough

This PR optimizes worktree path resolution in the getAllGrouped query by preloading all worktrees into a map once, then using map lookups instead of resolving paths individually per-workspace during grouping assembly.

Changes

Cohort / File(s) Summary
Worktree path preloading optimization
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
Introduces worktreePathMap to batch-load worktree ID-to-path mappings upfront. Replaces per-workspace path resolution with map lookups for worktree and branch type workspaces. Adjusts grouping logic to check group existence before pushing workspaces.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Poem

🐰✨ One map to load them all, once and true,
No loops within loops, our paths shine through!
Worktrees in cache, grouped with care,
Optimization hops, swift as air! 🚀

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: a performance optimization fixing an N+1 query issue in the getAllGrouped workspace procedure.
Description check ✅ Passed The description includes a clear summary of changes, performance metrics, and a test plan, but is missing related issues, type of change selection, and additional structured sections from the template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts (1)

98-103: Consider logging when worktree lookup fails.

When a workspace has a worktreeId but it's not found in worktreePathMap, this silently falls back to an empty string. This could mask data inconsistency (orphaned workspace referencing a deleted worktree).

Per coding guidelines on error handling, consider logging this case:

🔧 Suggested improvement
 				let worktreePath = "";
 				if (workspace.type === "worktree" && workspace.worktreeId) {
-					worktreePath = worktreePathMap.get(workspace.worktreeId) ?? "";
+					const resolvedPath = worktreePathMap.get(workspace.worktreeId);
+					if (resolvedPath) {
+						worktreePath = resolvedPath;
+					} else {
+						console.warn(
+							`[workspaces/getAllGrouped] Worktree ${workspace.worktreeId} not found for workspace ${workspace.id}`,
+						);
+					}
 				} else if (workspace.type === "branch") {
 					worktreePath = group.project.mainRepoPath;
 				}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d5dcf0 and 8230090.

📒 Files selected for processing (1)
  • apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc
Use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from @trpc/server/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

  • apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid using any type - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness

Files:

  • apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Drizzle ORM for all database operations - never use raw SQL

Files:

  • apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Biome for formatting and linting - run at root level with bun run lint:fix or biome check --write

Files:

  • apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
  • localDb (82-82)
packages/local-db/src/schema/schema.ts (1)
  • worktrees (50-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts (3)

10-11: LGTM!

The type alias provides good self-documentation for the map's purpose.


39-43: Good optimization - reduces N+1 to constant queries.

The preloading approach correctly eliminates the per-workspace query overhead. The map construction is clean and efficient.

Optionally, you could filter worktrees to only those belonging to active projects using a WHERE IN clause on projectId, but the current approach is simpler and likely sufficient given the expected table size.


192-195: The path resolution logic is already consistent between getAllGrouped and getActive. Both approaches produce identical results: getWorkspacePath returns the mainRepoPath for branch workspaces and the worktreePath for worktree workspaces, which matches the inline logic in getAllGrouped (lines 99-103). The only difference is that getAllGrouped uses preloaded data for optimization while getActive queries per-workspace, which is appropriate for fetching a single workspace.

@Kitenite Kitenite merged commit 1ae2bcf into main Jan 9, 2026
5 checks passed
@Kitenite Kitenite deleted the fix-workspace-concerns branch January 9, 2026 04:56
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 9, 2026

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch
  • ⚠️ Electric Fly.io app

Thank you for your contribution! 🎉

saddlepaddle pushed a commit that referenced this pull request Jan 10, 2026
Preload all worktrees once and use already-loaded project data from
groupsMap instead of calling getWorkspacePath per workspace, which was
causing N additional DB queries (one per workspace).

Before: 2 + N queries (where N = number of workspaces)
After: 3 queries (constant)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant