Repository navigation
Refactor GitHub store slice into cohesive modules - #16183
Conversation
📝 WalkthroughWalkthroughThe change separates GitHub cache identities, models, policies, persistence, routing, request coordination, refresh handling, and Zustand action factories into dedicated modules. GitHub PR, issue, work-item, check, comment, and ProjectV2 workflows now use these modules. The change adds bounded caching, runtime-aware routing, request deduplication, optimistic updates, rollback handling, and refresh-state coordination. Tests cover cache eviction, request sequencing, concurrency limits, issue and comment fetching, project mutations, and review-thread resolution. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. 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: 3
🧹 Nitpick comments (5)
src/renderer/src/store/github/request-coordination.ts (1)
74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared repo prefix helpers.
cache-identity.tsalready exportsrepoCacheKeyPrefixesandmatchesRepoCacheKeywith the same logic (lines 175-185). Two copies must stay in sync withworkItemsCacheKey, which can prepend a host segment. Reuse the exported helpers to keep one definition of the repo key prefix.♻️ Proposed refactor
+import { matchesRepoCacheKey, repoCacheKeyPrefixes } from './cache-identity' + export function clearInflightWorkItemsForRepo(repoId: string, repoPath?: string): void { - const prefixes = [`${repoId}::`] - if (repoPath && repoPath !== repoId) { - prefixes.push(`${repoPath}::`) - } + const prefixes = repoCacheKeyPrefixes(repoId, repoPath) for (const key of Array.from(inflightWorkItemsRequests.keys())) { - if (prefixes.some((prefix) => key.startsWith(prefix))) { + if (matchesRepoCacheKey(key, prefixes)) { inflightWorkItemsRequests.delete(key) } } }src/renderer/src/store/github/pull-request-execution.ts (1)
128-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the delete-then-reinsert and document its purpose.
capPrRefreshStatesuses insertion order when it must evict active entries, so this preserves recently refreshed states.src/renderer/src/store/github/pr-result-cache.ts (1)
66-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated cache-coordination body.
setGitHubPRResultCaches(Lines 87-136) andapplyGitHubPRResultToCaches(Lines 169-219) run the same three steps in the same order: computepreserveExistingPRForFallbackMiss, runsyncHostedReviewCacheFromGitHubPRResult, then runapplyPRCacheResultgated byshouldWritePRCacheForHostedReviewSync. Only the cache source and the return shape differ. The ordering here is behavior-critical, so a future edit applied to one copy and not the other silently changes results on one path.Express the state-scoped variant in terms of the pure variant.
♻️ Proposed consolidation
export function setGitHubPRResultCaches( state: AppState, args: { prCacheKey: string ... } ): Partial<AppState> { - const preserveExistingPRForFallbackMiss = shouldPreserveExistingPRForFallbackMiss({ ... }) - const hostedReviewSync = syncHostedReviewCacheFromGitHubPRResult({ ... }) - const hostedReviewCacheKey = getHostedReviewCacheKey(...) - const nextPRCache = applyPRCacheResult(...) + const next = applyGitHubPRResultToCaches({ + ...args, + prCache: state.prCache, + hostedReviewCache: state.hostedReviewCache, + state + }) return { - ...(nextPRCache === state.prCache ? {} : { prCache: nextPRCache }), - ...(hostedReviewSync.cache === state.hostedReviewCache - ? {} - : { hostedReviewCache: hostedReviewSync.cache }) + ...(next.prCache === state.prCache ? {} : { prCache: next.prCache }), + ...(next.hostedReviewCache === state.hostedReviewCache + ? {} + : { hostedReviewCache: next.hostedReviewCache }) } }src/renderer/src/store/github/stale-worktree-refresh-actions.ts (1)
22-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing lookup helpers.
Lines 22-31 re-implement a worktree scan that
findWorktreeByIdfrom./worktree-refreshalready provides.refresh-routing-actions.tsLine 24 uses that helper for the same lookup. Line 33 performs a second linear scan overstate.repos, whilerepository-routing.tsexposesfindRepoForGitHubOwnerbacked bygetGitHubRepoLookupIndex. Both scans run on every worktree activation.♻️ Proposed consolidation
-import { buildPRRefreshCandidate } from './worktree-refresh' +import { buildPRRefreshCandidate, findWorktreeById } from './worktree-refresh' @@ const state = get() - let worktree: Worktree | undefined - for (const worktrees of Object.values(state.worktreesByRepo)) { - worktree = worktrees.find((w) => w.id === worktreeId) - if (worktree) { - break - } - } + const worktree = findWorktreeById(state, worktreeId) if (!worktree) { return }Add
findRepoForGitHubOwnerto the./repository-routingimport and replace thestate.repos.findscan with an index lookup. Remove the now-unusedWorktreetype import.src/renderer/src/store/github/cache-persistence.ts (1)
8-16: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle rejected GitHub cache writes.
setGitHubreturns a promise, but the deferred write path does not attach a rejection handler. A failed cache write can become an unhandled rejection and leave persisted GitHub data stale while in-memory state continues. Attach a.catchhandler in the timer callback and log the failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3445eedd-2a8e-4bb3-8183-4fafb70796f2
📒 Files selected for processing (70)
config/max-lines-baseline.txtsrc/renderer/src/components/github-project/ProjectViewWrapper.tsxsrc/renderer/src/components/right-sidebar/ChecksPanel.tsxsrc/renderer/src/components/right-sidebar/FolderWorkspacePrChecksPanel.test.tsxsrc/renderer/src/components/right-sidebar/checks-panel-pr-refresh-breadcrumb.test.tssrc/renderer/src/components/right-sidebar/checks-panel-pr-refresh-breadcrumb.tssrc/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.tssrc/renderer/src/components/right-sidebar/parent-pr-checks-rows.tssrc/renderer/src/components/sidebar/use-worktree-card-review-details.tssrc/renderer/src/components/sidebar/use-worktree-issue-link.tssrc/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsxsrc/renderer/src/components/task-page-cache-selectors.test.tssrc/renderer/src/components/task-page-cache-selectors.tssrc/renderer/src/components/task-page-jira-cache-selectors.tssrc/renderer/src/components/task-page-linear-cache-selectors.tssrc/renderer/src/lib/worktree-palette-document.tssrc/renderer/src/store/github/cache-identity.tssrc/renderer/src/store/github/cache-model.tssrc/renderer/src/store/github/cache-persistence.tssrc/renderer/src/store/github/cache-policy.tssrc/renderer/src/store/github/check-actions.tssrc/renderer/src/store/github/comment-fetch-actions.tssrc/renderer/src/store/github/comment-mutation-actions.tssrc/renderer/src/store/github/conversation-comment-actions.tssrc/renderer/src/store/github/issue-actions.tssrc/renderer/src/store/github/pr-check-status-cache.tssrc/renderer/src/store/github/pr-comment-cache.tssrc/renderer/src/store/github/pr-refresh-state.tssrc/renderer/src/store/github/pr-result-cache.tssrc/renderer/src/store/github/pr-result-routing.tssrc/renderer/src/store/github/project-actions.tssrc/renderer/src/store/github/project-cache.tssrc/renderer/src/store/github/project-row-actions.tssrc/renderer/src/store/github/pull-request-actions.tssrc/renderer/src/store/github/pull-request-execution.tssrc/renderer/src/store/github/refresh-event-actions.tssrc/renderer/src/store/github/refresh-routing-actions.tssrc/renderer/src/store/github/refresh-sweep-actions.tssrc/renderer/src/store/github/repository-routing.tssrc/renderer/src/store/github/request-coordination.tssrc/renderer/src/store/github/review-thread-actions.tssrc/renderer/src/store/github/slice-types.tssrc/renderer/src/store/github/stale-worktree-refresh-actions.tssrc/renderer/src/store/github/work-item-aggregate-actions.tssrc/renderer/src/store/github/work-item-fetch-actions.tssrc/renderer/src/store/github/work-item-mutation-actions.tssrc/renderer/src/store/github/work-item-routing.tssrc/renderer/src/store/github/worktree-refresh.tssrc/renderer/src/store/slices/github-branch-mismatched-linked-pr.test.tssrc/renderer/src/store/slices/github-cache-eviction-and-bounds.test.tssrc/renderer/src/store/slices/github-checks-cache.test.tssrc/renderer/src/store/slices/github-issue-state-machine.test.tssrc/renderer/src/store/slices/github-pr-branch-coordinator-events.test.tssrc/renderer/src/store/slices/github-pr-branch-direct-refresh-scope.test.tssrc/renderer/src/store/slices/github-pr-branch-fallback-results.test.tssrc/renderer/src/store/slices/github-pr-branch-hosted-review-cache.test.tssrc/renderer/src/store/slices/github-pr-branch-linked-pr-divergence.test.tssrc/renderer/src/store/slices/github-pr-checks-fetch.test.tssrc/renderer/src/store/slices/github-pr-comments.test.tssrc/renderer/src/store/slices/github-project-request-coordination.test.tssrc/renderer/src/store/slices/github-project-view-tables.test.tssrc/renderer/src/store/slices/github-provider-request-concurrency.test.tssrc/renderer/src/store/slices/github-review-thread-actions.test.tssrc/renderer/src/store/slices/github-slice-test-harness.tssrc/renderer/src/store/slices/github-work-item-cache-identity.test.tssrc/renderer/src/store/slices/github-work-items-runtime-routing.test.tssrc/renderer/src/store/slices/github.tssrc/renderer/src/store/slices/jira.tssrc/renderer/src/store/slices/linear.tssrc/renderer/src/store/slices/repos.test.ts
💤 Files with no reviewable changes (1)
- config/max-lines-baseline.txt
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| if ( | ||
| envelope.ok && | ||
| (updates.addLabels?.length || | ||
| updates.removeLabels?.length || | ||
| updates.addAssignees?.length || | ||
| updates.removeAssignees?.length || | ||
| (previousRow.itemType === 'ISSUE' && | ||
| (updates.title !== undefined || updates.body !== undefined))) | ||
| ) { | ||
| const args = { | ||
| owner, | ||
| repo, | ||
| host: table.project.host, | ||
| number, | ||
| updates: { | ||
| ...(updates.title !== undefined ? { title: updates.title } : {}), | ||
| ...(updates.body !== undefined ? { body: updates.body } : {}), | ||
| ...(updates.addLabels ? { addLabels: updates.addLabels } : {}), | ||
| ...(updates.removeLabels ? { removeLabels: updates.removeLabels } : {}), | ||
| ...(updates.addAssignees ? { addAssignees: updates.addAssignees } : {}), | ||
| ...(updates.removeAssignees ? { removeAssignees: updates.removeAssignees } : {}) | ||
| } | ||
| } | ||
| const issueRes = | ||
| target.kind === 'environment' | ||
| ? await callRuntimeRpc<GitHubProjectMutationResult>( | ||
| target, | ||
| 'github.project.updateIssueBySlug', | ||
| args, | ||
| { timeoutMs: 30_000 } | ||
| ) | ||
| : await window.api.gh.updateIssueBySlug(args) | ||
| if (!issueRes.ok) { | ||
| envelope = issueRes | ||
| } | ||
| } | ||
| if (!envelope.ok) { | ||
| rollbackRowIfPresent(set, get, cacheKey, rowId, previousRow) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Partial-success rollback can discard a mutation the server already applied.
patchProjectIssueOrPr can send two mutations for one call. For a PULL_REQUEST row with title/body plus label or assignee changes, updatePullRequestBySlug runs first, then updateIssueBySlug. If the first call succeeds and the second fails, line 167 restores previousRow in full. The row then shows the old title and body even though the server stored the new ones. The stale values persist until the next project refresh.
Consider rolling back only the fields whose mutation failed. Track the applied title/body separately, and rebuild the rollback row from previousRow plus the confirmed changes.
🩹 Sketch of a scoped rollback
if (!envelope.ok) {
- rollbackRowIfPresent(set, get, cacheKey, rowId, previousRow)
+ // Keep title/body when the PR mutation already committed them.
+ const rollbackRow: GitHubProjectRow = prCommitted
+ ? {
+ ...previousRow,
+ content: {
+ ...previousRow.content,
+ title: nextContent.title,
+ body: nextContent.body
+ }
+ }
+ : previousRow
+ rollbackRowIfPresent(set, get, cacheKey, rowId, rollbackRow)
}Set prCommitted to true after prRes.ok at line 126.
| // Optimistic toggle of isResolved for this thread; reverts if the API call fails. | ||
| const prev = get().commentsCache[cacheKey]?.data | ||
| if (prev) { | ||
| set((s) => ({ | ||
| commentsCache: { | ||
| ...s.commentsCache, | ||
| [cacheKey]: { | ||
| ...s.commentsCache[cacheKey], | ||
| data: prev.map((c) => (c.threadId === threadId ? { ...c, isResolved: resolve } : c)) | ||
| } | ||
| } | ||
| })) | ||
| } | ||
|
|
||
| const requestContext = getGitHubWorkItemRequestContext( | ||
| get(), | ||
| requestSettings, | ||
| repoId ?? repoPath, | ||
| repoPath, | ||
| options?.sourceContext | ||
| ) | ||
| let ok = false | ||
| try { | ||
| ok = | ||
| requestContext.target.kind === 'environment' | ||
| ? await callRuntimeRpc<boolean>( | ||
| { kind: 'environment', environmentId: requestContext.target.environmentId }, | ||
| 'github.resolveReviewThread', | ||
| { | ||
| repo: requestContext.target.runtimeRepoId, | ||
| threadId, | ||
| resolve, | ||
| prRepo: options?.prRepo ?? null | ||
| }, | ||
| { timeoutMs: 30_000 } | ||
| ) | ||
| : await window.api.gh.resolveReviewThread({ | ||
| repoPath, | ||
| repoId, | ||
| threadId, | ||
| resolve, | ||
| prRepo: options?.prRepo ?? null, | ||
| sourceContext: options?.sourceContext | ||
| }) | ||
| } catch (err) { | ||
| console.error('Failed to update review thread:', err) | ||
| ok = false | ||
| } | ||
| if (!ok && prev) { | ||
| // Revert optimistic update on failure | ||
| set((s) => ({ | ||
| commentsCache: { | ||
| ...s.commentsCache, | ||
| [cacheKey]: { ...s.commentsCache[cacheKey], data: prev } | ||
| } | ||
| })) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply the optimistic update and the revert to the current cache entry, not to a snapshot.
Line 41 maps over the prev snapshot, and line 86 writes data: prev back. Both writes discard every change made to commentsCache[cacheKey] while the request is in flight. fetchPRComments, addPRReviewCommentReply, addPRConversationComment, and setPRCommentReaction all write to the same key. If a user posts a reply while a resolve request runs and the resolve then fails, the reply is removed from the rendered list until the next fetch.
setPRCommentReaction in src/renderer/src/store/github/comment-mutation-actions.ts lines 192-213 already uses the correct pattern. It restores one field on the latest entry.data instead of restoring a snapshot.
Lines 40 and 86 also spread s.commentsCache[cacheKey] without a guard. If withBoundedCacheEntry evicts the key during the request, the revert writes an entry with no fetchedAt.
♻️ Proposed fix: target only the affected thread
- // Optimistic toggle of isResolved for this thread; reverts if the API call fails.
- const prev = get().commentsCache[cacheKey]?.data
- if (prev) {
- set((s) => ({
- commentsCache: {
- ...s.commentsCache,
- [cacheKey]: {
- ...s.commentsCache[cacheKey],
- data: prev.map((c) => (c.threadId === threadId ? { ...c, isResolved: resolve } : c))
- }
- }
- }))
- }
+ // Snapshot per-thread resolution only; a full-list revert would drop concurrent cache writes.
+ const previousResolved = new Map(
+ (get().commentsCache[cacheKey]?.data ?? [])
+ .filter((c) => c.threadId === threadId)
+ .map((c) => [c.id, c.isResolved])
+ )
+ const applyResolved = (next: (comment: PRComment) => boolean | undefined) =>
+ set((s) => {
+ const entry = s.commentsCache[cacheKey]
+ if (!entry?.data) return s
+ return {
+ commentsCache: {
+ ...s.commentsCache,
+ [cacheKey]: {
+ ...entry,
+ data: entry.data.map((c) =>
+ c.threadId === threadId ? { ...c, isResolved: next(c) } : c
+ )
+ }
+ }
+ }
+ })
+ applyResolved(() => resolve)Then revert with the same helper:
- if (!ok && prev) {
- // Revert optimistic update on failure
- set((s) => ({
- commentsCache: {
- ...s.commentsCache,
- [cacheKey]: { ...s.commentsCache[cacheKey], data: prev }
- }
- }))
- }
+ if (!ok && previousResolved.size > 0) {
+ applyResolved((comment) => previousResolved.get(comment.id))
+ }Add the PRComment type import:
import type { GitHubSlice } from './slice-types'
+import type { PRComment } from '../../../../shared/github/comment-types'| set((s) => { | ||
| const prefix = `${repoId}::` | ||
| const legacyPrefix = `${repoPath}::` | ||
| const next: Record<string, CacheEntry<readonly GitHubWorkItem[]>> = {} | ||
| for (const [key, entry] of Object.entries(s.workItemsCache)) { | ||
| if (!key.startsWith(prefix) && !key.startsWith(legacyPrefix)) { | ||
| next[key] = entry | ||
| } | ||
| } | ||
| // Why: the Tasks fetch effect keys on the nonce, not the cache, so bump it to re-run and re-populate the evicted entries. | ||
| return { workItemsCache: next, workItemsInvalidationNonce: s.workItemsInvalidationNonce + 1 } | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect matchesRepoCacheKey and workItemsCacheKey to confirm host-prefixed key handling.
fd -t f 'cache-identity.ts' src/renderer/src/store/github --exec cat -n {}
# Find tests asserting eviction of host-scoped work-item cache keys.
rg -n -C4 'matchesRepoCacheKey|evictRepoCacheEntries|setIssueSourcePreference' --type=ts -g '*test*'Repository: stablyai/orca
Length of output: 8804
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- mutation action imports and eviction block ---'
sed -n '1,135p' src/renderer/src/store/github/work-item-mutation-actions.ts
printf '%s\n' '--- work-item cache key call sites ---'
rg -n -C3 'workItemsCacheKey|workItemsCache|workItemsInvalidationNonce' src/renderer/src/store/github --type ts
printf '%s\n' '--- shared helper call sites and tests ---'
rg -n -C3 'repoCacheKeyPrefixes|matchesRepoCacheKey|evictRepoCacheEntries|evictGitHubRepoCaches' src/renderer/src/store/github --type tsRepository: stablyai/orca
Length of output: 32603
Use a host-aware matcher for work-item cache eviction.
repoCacheKeyPrefixes and matchesRepoCacheKey only match keys that start with ${repoId}:: or ${repoPath}::; reusing them alone would not evict workItemsCacheKey entries with an execution-host prefix. Extend the matcher or add a work-item-specific matcher, and keep the unconditional nonce bump.
(cherry picked from commit 8776b6e)
Architecture
src/renderer/src/store/slices/github.tsto state initialization and composition of concrete action groups.src/renderer/src/store/github/modules.Preserved regression contracts
Root line count
origin/main): 4,867 lines.Validation
Dependency setup, without lockfile changes:
Result: passed.
Changed/added tests:
Result: 22 files passed; 235 tests passed.
pnpm exec oxlint --deny-warningsResult: passed with zero warnings.
Result: passed.
Result: passed — 186 grandfathered suppressions, no new bypasses.