feat(desktop): add batch fetching for GitHub PR statuses#2959
Closed
vaishcodescape wants to merge 1 commit into
Closed
feat(desktop): add batch fetching for GitHub PR statuses#2959vaishcodescape wants to merge 1 commit into
vaishcodescape wants to merge 1 commit into
Conversation
- Introduced `batchGetGitHubStatuses` procedure to efficiently retrieve statuses for multiple workspaces. - Enhanced GitHub utilities with `batchFetchGitHubPRStatuses` for improved performance. - Updated workspace-related components to utilize the new batch fetching mechanism. - Adjusted query policies to align with the new batch polling interval for workspace list items. - Refactored related types and functions to support the new batch processing capabilities.
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a batch GitHub PR status fetching system for workspaces. Instead of individual per-workspace queries, it implements a new TRPC procedure that fetches GitHub statuses for all workspaces in a single GraphQL query, with caching and normalization logic, plus a React hook to invoke the batch operation periodically while preserving existing per-workspace cache data. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer<br/>(useBatchGitHubStatus)
participant tRPC as tRPC<br/>(batchGetGitHubStatuses)
participant BatchUtil as Batch Utility<br/>(batchFetchGitHubPRStatuses)
participant GraphQL as GitHub<br/>GraphQL API
participant LocalDB as Local DB<br/>(cache/update)
participant Cache as React Query<br/>Cache
Renderer->>tRPC: periodic query (every 15s)
activate tRPC
tRPC->>tRPC: load all workspaces & worktrees
tRPC->>tRPC: build (workspaceId, path, branch) entries
tRPC->>BatchUtil: fetch statuses for entries
activate BatchUtil
BatchUtil->>BatchUtil: group by repo, deduplicate branches
BatchUtil->>BatchUtil: build aliased GraphQL query
BatchUtil->>GraphQL: execute batch query (30s timeout)
GraphQL-->>BatchUtil: PR nodes per alias
BatchUtil->>BatchUtil: validate with GHGraphQLPRNodeSchema
BatchUtil->>BatchUtil: normalize GraphQL→GHPRResponse
BatchUtil->>BatchUtil: filter matches, select best PR
BatchUtil->>LocalDB: cache status (when changed)
BatchUtil-->>tRPC: Map<workspaceId, GitHubStatus>
deactivate BatchUtil
tRPC->>LocalDB: update worktree githubStatus (if changed)
tRPC-->>Renderer: batch result
deactivate tRPC
Renderer->>Cache: seed workspace cache only if empty
Cache-->>Renderer: cache updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx (1)
39-39:hasHoveredstate appears to be unused after this change.The
hasHoveredstate and itssetHasHoveredcall at line 72 no longer gate any behavior sinceisActivein the query policy (line 44) no longer depends on it. Consider removing this dead state.♻️ Suggested cleanup
-const [hasHovered, setHasHovered] = useState(false); ... -onMouseEnter={() => !hasHovered && setHasHovered(true)} +onMouseEnter={() => {}}Or remove the
onMouseEnterhandler entirely if no other hover behavior is needed.Also applies to: 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx` at line 39, Remove the dead hover state by deleting the hasHovered/useState declaration and any calls to setHasHovered (e.g., the onMouseEnter handler inside the WorkspaceRow component); since isActive no longer depends on hasHovered, eliminate the unused state and the onMouseEnter prop so no hover-related state is kept in WorkspaceRow.apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts (1)
328-339: Consider filtering out null reviewers instead of returning empty objects.When
revieweris null at line 330, returning an empty object{}may introduce entries that don't matchGHReviewRequestSchemaexpectations (which has optional fields but expects meaningful data). Downstream code iterating overreviewRequestsmight encounter unexpected empty objects.♻️ Suggested improvement
const reviewRequests: GHPRResponse["reviewRequests"] = node.reviewRequests?.nodes ?.filter((rr): rr is NonNullable<typeof rr> => rr !== null) + .filter((rr) => rr.requestedReviewer !== null) .map((rr) => { const reviewer = rr.requestedReviewer; - if (!reviewer) return {}; + // reviewer is guaranteed non-null after filter if (reviewer.__typename === "User") { return { login: reviewer.login, type: "User" as const }; } return { slug: reviewer.slug, name: reviewer.name, type: "Team" as const, }; }) ?? null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts` around lines 328 - 339, The map that transforms rr.requestedReviewer (in the reviewRequests creation) returns {} when reviewer is falsy which can produce invalid empty objects; instead filter out falsy reviewers before mapping (or use flatMap to skip nulls) so only real User or Team objects are returned; update the mapping that references rr.requestedReviewer to either pre-filter rr.requestedReviewer != null or return null and then .filter(Boolean) so the resulting array conforms to GHReviewRequestSchema and contains only valid reviewer objects.apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts (2)
282-293:branchExistsOnRemote: truemay be inaccurate.Both the "PR found" and "no PR" code paths hardcode
branchExistsOnRemote: true. This doesn't reflect actual remote branch existence—the batch query only checks for PRs by head branch name, not whether the branch itself exists on the remote.For workspaces with no PR, the branch may or may not exist on the remote. Consider either:
- Omitting
branchExistsOnRemotefrom batch results (let per-workspace queries populate it accurately)- Adding a separate check for branch existence
Also applies to: 301-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts` around lines 282 - 293, The code is incorrectly hardcoding branchExistsOnRemote: true in the GitHubStatus objects (in the batch PR path inside batch-pr-status and the corresponding no-PR branch around lines 301-311), which misrepresents remote branch existence; remove the hardcoded branchExistsOnRemote from the GitHubStatus construction (both the "PR found" and "no PR" branches) so the batch result does not claim the branch exists, and instead let per-workspace queries populate branchExistsOnRemote accurately (update the GitHubStatus type/nullable handling if necessary so setCachedGitHubStatus and results.set accept a missing/undefined branchExistsOnRemote).
317-319: Consider escaping additional control characters in GraphQL strings.The current escape function handles
\and", which prevents basic injection. However, GraphQL string literals also interpret\n,\r,\t, and other escape sequences. If a branch name contains a literal newline or carriage return (unusual but possible), it could produce malformed queries.More complete escape function
function escapeGraphQLString(value: string): string { - return value.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); + return value + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"') + .replace(/\n/g, "\\n") + .replace(/\r/g, "\\r") + .replace(/\t/g, "\\t"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts` around lines 317 - 319, The escapeGraphQLString function only handles backslashes and double quotes but must also escape control characters (e.g., newline \n, carriage return \r, tab \t, backspace \b, form feed \f and other non-printables) to avoid producing malformed GraphQL string literals; update escapeGraphQLString to replace those characters with their corresponding escaped sequences (e.g., "\n" -> "\\n", "\r" -> "\\r", "\t" -> "\\t", "\b" -> "\\b", "\f" -> "\\f") or use a robust serializer approach (e.g., via JSON.stringify on the value and strip surrounding quotes) to ensure all control/Unicode characters are safely encoded when building GraphQL queries that include branch names or other user data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts`:
- Around line 282-293: The code is incorrectly hardcoding branchExistsOnRemote:
true in the GitHubStatus objects (in the batch PR path inside batch-pr-status
and the corresponding no-PR branch around lines 301-311), which misrepresents
remote branch existence; remove the hardcoded branchExistsOnRemote from the
GitHubStatus construction (both the "PR found" and "no PR" branches) so the
batch result does not claim the branch exists, and instead let per-workspace
queries populate branchExistsOnRemote accurately (update the GitHubStatus
type/nullable handling if necessary so setCachedGitHubStatus and results.set
accept a missing/undefined branchExistsOnRemote).
- Around line 317-319: The escapeGraphQLString function only handles backslashes
and double quotes but must also escape control characters (e.g., newline \n,
carriage return \r, tab \t, backspace \b, form feed \f and other non-printables)
to avoid producing malformed GraphQL string literals; update escapeGraphQLString
to replace those characters with their corresponding escaped sequences (e.g.,
"\n" -> "\\n", "\r" -> "\\r", "\t" -> "\\t", "\b" -> "\\b", "\f" -> "\\f") or
use a robust serializer approach (e.g., via JSON.stringify on the value and
strip surrounding quotes) to ensure all control/Unicode characters are safely
encoded when building GraphQL queries that include branch names or other user
data.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts`:
- Around line 328-339: The map that transforms rr.requestedReviewer (in the
reviewRequests creation) returns {} when reviewer is falsy which can produce
invalid empty objects; instead filter out falsy reviewers before mapping (or use
flatMap to skip nulls) so only real User or Team objects are returned; update
the mapping that references rr.requestedReviewer to either pre-filter
rr.requestedReviewer != null or return null and then .filter(Boolean) so the
resulting array conforms to GHReviewRequestSchema and contains only valid
reviewer objects.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx`:
- Line 39: Remove the dead hover state by deleting the hasHovered/useState
declaration and any calls to setHasHovered (e.g., the onMouseEnter handler
inside the WorkspaceRow component); since isActive no longer depends on
hasHovered, eliminate the unused state and the onMouseEnter prop so no
hover-related state is kept in WorkspaceRow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e492b70-dd08-46b7-a43f-9b2b4b1b5137
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.tsapps/desktop/src/renderer/hooks/useBatchGitHubStatus.tsapps/desktop/src/renderer/lib/githubQueryPolicy/githubQueryPolicy.test.tsapps/desktop/src/renderer/lib/githubQueryPolicy/githubQueryPolicy.tsapps/desktop/src/renderer/lib/githubQueryPolicy/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsx
Contributor
Author
|
@saddlepaddle Can you please review this PR I believe the checks are passing correctly ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
batchGetGitHubStatusesprocedure to efficiently retrieve statuses for multiple workspaces.batchFetchGitHubPRStatusesfor improved performance.Related Issues
Closes PR #2958
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Adds batch fetching for GitHub PR statuses in the desktop app to replace N+1 lookups with a single GraphQL request. Improves sidebar responsiveness and aligns polling to a 15s interval.
New Features
batchGetGitHubStatusesto return a map of workspace IDs toGitHubStatus, persisting meaningful changes to local DB.batchFetchGitHubPRStatusesthat groups entries by repo and runs one GraphQL query viagh, then normalizes results.useBatchGitHubStatusto fetch once at the sidebar level and seed per-workspace caches without overwriting richer data.Refactors
sortPRCandidatesandformatPRData; added GraphQL schemas andnormalizeGraphQLPR.BATCH_GITHUB_STATUS_REFETCH_INTERVAL_MSto 15s and aligned workspace list item stale time.Written for commit 1c29b5b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes