Show git submodules in Source Control#5936
Conversation
|
Ready to review this PR? Stage has broken it down into 8 individual chapters for you: 
Chapters generated by Stage for commit f1b499f on Jun 21, 2026 9:19pm UTC. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds end-to-end git submodule status support. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. 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 (3)
src/shared/git-submodule-status.test.ts (1)
5-39: ⚡ Quick winAdd a regression case for submodule paths containing
(/)characters.Current tests won’t catch path truncation when a valid submodule path includes parentheses and a trailing description exists.
💡 Suggested test addition
it('parses clean, uninitialized, modified, and conflicted submodules', () => { @@ }) + + it('keeps full path when path contains parentheses', () => { + expect( + parseGitSubmoduleStatusOutput( + ' 1111111111111111111111111111111111111111 vendor/lib (legacy)/core (heads/main)\n' + ) + ).toEqual([ + { + path: 'vendor/lib (legacy)/core', + head: '1111111111111111111111111111111111111111', + status: 'clean', + description: 'heads/main' + } + ]) + }) })src/renderer/src/components/right-sidebar/SourceControl.tsx (1)
737-739: ⚡ Quick winUse a stable empty-array sentinel for the submodule selector fallback.
The inline
[]fallback creates a new reference on each store update when submodules are missing, which can trigger unnecessary rerenders. Reuse a module-level constant like the other status selectors.♻️ Suggested change
const EMPTY_GIT_STATUS_ENTRIES: GitStatusEntry[] = [] const EMPTY_BRANCH_CHANGE_ENTRIES: GitBranchChangeEntry[] = [] +const EMPTY_GIT_SUBMODULE_ENTRIES: GitSubmoduleEntry[] = [] @@ const submodules = useAppStore((s) => - activeWorktreeId ? (s.gitSubmodulesByWorktree?.[activeWorktreeId] ?? []) : [] + activeWorktreeId + ? (s.gitSubmodulesByWorktree?.[activeWorktreeId] ?? EMPTY_GIT_SUBMODULE_ENTRIES) + : EMPTY_GIT_SUBMODULE_ENTRIES )tests/e2e/source-control-submodules.spec.ts (1)
70-79: ⚡ Quick winUse path utilities and document the file-protocol override.
Line 70 hardcodes
/, and Lines 72-74 apply a non-obvious Git override without context. Please switch to a Node path utility and add a short why-comment.Suggested patch
- const submodulePath = `vendor/e2e-submodule-${Date.now()}` + const submodulePath = path.posix.join('vendor', `e2e-submodule-${Date.now()}`) await git(worktreePath, [ + // Allow local temp repos as submodule remotes in CI/e2e. '-c', 'protocol.file.allow=always', 'submodule',As per coding guidelines,
Use path.join or Electron/Node path utilities for file paths — never assume / or \ for cross-platform compatibility, andWhen writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b869531-c150-4aba-a638-f7792a670917
📒 Files selected for processing (14)
src/main/git/status-submodules.integration.test.tssrc/main/git/status.test.tssrc/main/git/status.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/store/slices/editor.tssrc/renderer/src/store/slices/worktrees.tssrc/shared/git-status-types.tssrc/shared/git-submodule-status.test.tssrc/shared/git-submodule-status.tssrc/shared/types.tstests/e2e/source-control-submodules.spec.ts
| function parseGitSubmoduleStatusLine(line: string): GitSubmoduleEntry | null { | ||
| const prefix = line[0] | ||
| const rest = line.slice(1) | ||
| const match = rest.match(/^([0-9a-fA-F]{40}) (.+?)(?: \((.*)\))?$/) | ||
| if (!match) { | ||
| return null | ||
| } | ||
|
|
||
| return { | ||
| path: match[2], | ||
| head: match[1], | ||
| status: parseGitSubmoduleStatusPrefix(prefix), | ||
| ...(match[3] ? { description: match[3] } : {}) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix submodule path parsing for paths containing parentheses.
At Line 22, the non-greedy path capture can truncate valid paths like vendor/lib (legacy)/core by treating the first ( as description start.
💡 Suggested fix
function parseGitSubmoduleStatusLine(line: string): GitSubmoduleEntry | null {
const prefix = line[0]
const rest = line.slice(1)
- const match = rest.match(/^([0-9a-fA-F]{40}) (.+?)(?: \((.*)\))?$/)
+ const match = rest.match(/^([0-9a-fA-F]{40}) (.+)$/)
if (!match) {
return null
}
+
+ const tail = match[2]
+ const descStart = tail.lastIndexOf(' (')
+ const hasDescription = descStart !== -1 && tail.endsWith(')')
+ const path = hasDescription ? tail.slice(0, descStart) : tail
+ const description = hasDescription ? tail.slice(descStart + 2, -1) : undefined
return {
- path: match[2],
+ path,
head: match[1],
status: parseGitSubmoduleStatusPrefix(prefix),
- ...(match[3] ? { description: match[3] } : {})
+ ...(description ? { description } : {})
}
}| test.afterEach(async () => { | ||
| if (libraryPath) { | ||
| await rm(libraryPath, { recursive: true, force: true }) | ||
| libraryPath = null | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate e2e harness files =="
fd -i 'orca-app' tests/e2e/helpers -t f
fd -i 'store' tests/e2e/helpers -t f
echo "== Inspect lifecycle/setup for workspace isolation =="
rg -n -C4 'beforeAll|beforeEach|afterEach|afterAll|mkdtemp|rm\(|reset|checkout|clone|worktree|workspace' tests/e2e/helpers
echo "== Inspect this spec's mutation/cleanup pattern =="
rg -n -C3 'createCommittedSubmodule|commit|afterEach|rm\(' tests/e2e/source-control-submodules.spec.tsRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect source-control-submodules.spec.ts setup and cleanup ==="
wc -l tests/e2e/source-control-submodules.spec.ts
head -150 tests/e2e/source-control-submodules.spec.ts | tail -80
echo "=== Check createSeededTestRepo and fixture setup pattern ==="
grep -n "seedTestRepo\|testRepoPath\|createSeededTestRepo\|beforeAll\|afterAll" tests/e2e/helpers/orca-app.ts | head -20
echo "=== Inspect if test repo is per-test or shared ==="
grep -A5 "seedTestRepo:" tests/e2e/helpers/orca-app.ts | head -10Repository: stablyai/orca
Length of output: 3971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check fixture scope for sharedPage and electronApp ==="
grep -B5 -A15 "electronApp: async" tests/e2e/helpers/orca-app.ts | head -40
echo "=== Confirm testRepoPath is per-test ==="
grep -B5 -A10 "testRepoPath:" tests/e2e/helpers/orca-app.ts
echo "=== Check if createSeededTestRepo creates truly isolated repos ==="
sed -n '135,175p' tests/e2e/helpers/orca-app.tsRepository: stablyai/orca
Length of output: 4214
Clean up the committed submodule changes from the active worktree in afterEach.
The test repo is worker-scoped and shared across all tests in the same worker. While each test gets a fresh Electron app instance, the underlying git worktree is not reset between tests. Since line 80 commits the submodule to the worktree but afterEach (lines 92-97) only removes the temporary library, the commit persists and can leak to subsequent tests, causing order-dependent failures.
Add a git reset to afterEach to clean up worktree state:
Example cleanup pattern
test.afterEach(async ({ orcaPage }) => {
if (libraryPath) {
await rm(libraryPath, { recursive: true, force: true })
libraryPath = null
}
// Reset the active worktree to clean up any committed changes
const worktreePath = await getActiveWorktreePath(orcaPage)
if (worktreePath) {
await git(worktreePath, ['reset', '--hard', 'HEAD~1'])
}
})Also applies to: 99-135
Jinwoo-H
force-pushed
the
Jinwoo-H/issue-5868-investigate
branch
from
58f39e2 to
67b0396
Compare
Jinwoo-H
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/shared/gitmodules-parser.test.ts (1)
32-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression case for section-boundary reset behavior.
Please add a test where a non-submodule section appears after a valid submodule section and ensure its
path/urllines are ignored for submodule output.src/relay/git-handler-status-ops.ts (1)
72-72: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
GitSubmoduleEntry[]instead ofRecord<string, unknown>[]for relay submodules.This keeps the relay payload strongly aligned with the shared submodule contract and avoids silent shape drift.
Proposed fix
-import type { GitUpstreamStatus } from '../shared/types' +import type { GitSubmoduleEntry, GitUpstreamStatus } from '../shared/types' @@ - submodules?: Record<string, unknown>[] + submodules?: GitSubmoduleEntry[] @@ -async function getSubmodulesOp(worktreePath: string): Promise<Record<string, unknown>[]> { +async function getSubmodulesOp(worktreePath: string): Promise<GitSubmoduleEntry[]> {Also applies to: 178-186
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf45051d-4d74-4550-95a5-f6fd9e0b671b
📒 Files selected for processing (25)
src/main/git/status-submodules.integration.test.tssrc/main/git/status.test.tssrc/main/git/status.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/renderer/src/components/right-sidebar/FileExplorer.test.tsxsrc/renderer/src/components/right-sidebar/FileExplorer.tsxsrc/renderer/src/components/right-sidebar/FileExplorerRow.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/store/slices/editor.tssrc/renderer/src/store/slices/worktrees.tssrc/shared/git-status-types.tssrc/shared/gitmodules-parser.test.tssrc/shared/gitmodules-parser.tssrc/shared/types.tstests/e2e/source-control-submodules.spec.ts
✅ Files skipped from review due to trivial changes (4)
- src/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsx
- src/shared/types.ts
- src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx
- src/renderer/src/components/right-sidebar/SourceControl.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/store/slices/worktrees.ts
| const sectionMatch = line.match(/^\s*\[submodule\s+"(.+)"\]\s*(?:[#;].*)?$/) | ||
| if (sectionMatch) { | ||
| currentName = sectionMatch[1] | ||
| if (!entriesByName.has(currentName)) { | ||
| entriesByName.set(currentName, { name: currentName }) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if (!currentName) { | ||
| continue | ||
| } | ||
|
|
||
| const propertyMatch = line.match(/^\s*([A-Za-z][A-Za-z0-9.-]*)\s*=\s*(.*?)\s*$/) | ||
| if (!propertyMatch) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Reset currentName when a non-submodule section header is encountered.
Line 17 currently keeps the previous submodule context active, so a later path=/url= under another section can be misattributed to the prior submodule.
Proposed fix
for (const line of raw.split(/\r?\n/)) {
const sectionMatch = line.match(/^\s*\[submodule\s+"(.+)"\]\s*(?:[#;].*)?$/)
if (sectionMatch) {
currentName = sectionMatch[1]
if (!entriesByName.has(currentName)) {
entriesByName.set(currentName, { name: currentName })
}
continue
}
+
+ const nonSubmoduleSectionMatch = line.match(/^\s*\[[^\]]+\]\s*(?:[#;].*)?$/)
+ if (nonSubmoduleSectionMatch) {
+ currentName = null
+ continue
+ }
if (!currentName) {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sectionMatch = line.match(/^\s*\[submodule\s+"(.+)"\]\s*(?:[#;].*)?$/) | |
| if (sectionMatch) { | |
| currentName = sectionMatch[1] | |
| if (!entriesByName.has(currentName)) { | |
| entriesByName.set(currentName, { name: currentName }) | |
| } | |
| continue | |
| } | |
| if (!currentName) { | |
| continue | |
| } | |
| const propertyMatch = line.match(/^\s*([A-Za-z][A-Za-z0-9.-]*)\s*=\s*(.*?)\s*$/) | |
| if (!propertyMatch) { | |
| continue | |
| } | |
| const sectionMatch = line.match(/^\s*\[submodule\s+"(.+)"\]\s*(?:[#;].*)?$/) | |
| if (sectionMatch) { | |
| currentName = sectionMatch[1] | |
| if (!entriesByName.has(currentName)) { | |
| entriesByName.set(currentName, { name: currentName }) | |
| } | |
| continue | |
| } | |
| const nonSubmoduleSectionMatch = line.match(/^\s*\[[^\]]+\]\s*(?:[#;].*)?$/) | |
| if (nonSubmoduleSectionMatch) { | |
| currentName = null | |
| continue | |
| } | |
| if (!currentName) { | |
| continue | |
| } | |
| const propertyMatch = line.match(/^\s*([A-Za-z][A-Za-z0-9.-]*)\s*=\s*(.*?)\s*$/) | |
| if (!propertyMatch) { | |
| continue | |
| } |
Jinwoo-H
force-pushed
the
Jinwoo-H/issue-5868-investigate
branch
from
67b0396 to
1e4c2bf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/source-control-submodules.spec.ts (1)
147-153: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a short “why” comment for the clean-submodule assertion.
The expectation that the Submodules section is absent even when
status.submodulesexists is non-obvious; add a 1–2 line rationale near this assertion to preserve intent.As per coding guidelines, "When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does."
Suggested tweak
- await expect(orcaPage.getByText('Submodules')).toHaveCount(0) + // Source Control rows come from porcelain status entries only. + // Clean submodules remain in metadata but should not render rows/sections. + await expect(orcaPage.getByText('Submodules')).toHaveCount(0)Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d17c037-5369-4371-97b6-141b1e4deb64
📒 Files selected for processing (25)
src/main/git/status-submodules.integration.test.tssrc/main/git/status.test.tssrc/main/git/status.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/renderer/src/components/right-sidebar/FileExplorer.test.tsxsrc/renderer/src/components/right-sidebar/FileExplorer.tsxsrc/renderer/src/components/right-sidebar/FileExplorerRow.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/store/slices/editor.tssrc/renderer/src/store/slices/worktrees.tssrc/shared/git-status-types.tssrc/shared/gitmodules-parser.test.tssrc/shared/gitmodules-parser.tssrc/shared/types.tstests/e2e/source-control-submodules.spec.ts
✅ Files skipped from review due to trivial changes (2)
- src/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsx
- src/renderer/src/components/right-sidebar/SourceControl.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- src/shared/gitmodules-parser.test.ts
- src/main/git/status-submodules.integration.test.ts
- src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx
- src/renderer/src/components/right-sidebar/FileExplorer.tsx
- src/renderer/src/components/right-sidebar/FileExplorer.test.tsx
- src/renderer/src/components/right-sidebar/FileExplorerRow.tsx
- src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx
- src/shared/git-status-types.ts
- src/shared/gitmodules-parser.ts
- src/relay/git-handler-status-ops.test.ts
- src/main/git/status.ts
- src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsx
- src/shared/types.ts
- src/relay/git-handler-status-ops.ts
- src/renderer/src/i18n/locales/en.json
- src/main/git/status.test.ts
- src/renderer/src/store/slices/worktrees.ts
- src/renderer/src/store/slices/editor.ts
- src/renderer/src/i18n/locales/es.json
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/i18n/locales/ja.json
- src/renderer/src/i18n/locales/zh.json
Co-authored-by: Orca <help@stably.ai>
Jinwoo-H
force-pushed
the
Jinwoo-H/issue-5868-investigate
branch
from
1e4c2bf to
e20823a
Compare
Jinwoo-H
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/e2e/source-control-submodules.spec.ts (1)
173-182: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider escaping special regex characters in path-based patterns.
The static analysis flags
new RegExp(path.basename(...))as potentially unsafe. While here the paths are test-controlled (000-e2e-*-submodule-${stamp}) with no regex metacharacters, escaping would add robustness if the naming pattern ever changes.- const cleanExplorerRow = orcaPage.getByRole('button', { - name: new RegExp(path.basename(clean.submodulePath)) - }) - const dirtyExplorerRow = orcaPage.getByRole('button', { - name: new RegExp(path.basename(dirty.submodulePath)) - }) + const escapeRegExp = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + const cleanExplorerRow = orcaPage.getByRole('button', { + name: new RegExp(escapeRegExp(path.basename(clean.submodulePath))) + }) + const dirtyExplorerRow = orcaPage.getByRole('button', { + name: new RegExp(escapeRegExp(path.basename(dirty.submodulePath))) + })Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d562ab6a-8f6d-4297-9177-c57609403785
📒 Files selected for processing (28)
src/main/git/status-submodules.integration.test.tssrc/main/git/status.test.tssrc/main/git/status.tssrc/main/ipc/filesystem-auth.test.tssrc/main/ipc/filesystem-auth.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/renderer/src/components/right-sidebar/FileExplorer.test.tsxsrc/renderer/src/components/right-sidebar/FileExplorer.tsxsrc/renderer/src/components/right-sidebar/FileExplorerRow.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsxsrc/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsxsrc/renderer/src/components/right-sidebar/source-control-discard-dialog.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/store/slices/editor.tssrc/renderer/src/store/slices/worktrees.tssrc/shared/git-status-types.tssrc/shared/gitmodules-parser.test.tssrc/shared/gitmodules-parser.tssrc/shared/types.tstests/e2e/source-control-submodules.spec.ts
✅ Files skipped from review due to trivial changes (4)
- src/shared/gitmodules-parser.test.ts
- src/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsx
- src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx
- src/renderer/src/i18n/locales/ko.json
🚧 Files skipped from review as they are similar to previous changes (18)
- src/shared/types.ts
- src/shared/git-status-types.ts
- src/relay/git-handler-status-ops.test.ts
- src/main/git/status-submodules.integration.test.ts
- src/renderer/src/components/right-sidebar/FileExplorer.tsx
- src/renderer/src/components/right-sidebar/FileExplorer.test.tsx
- src/main/git/status.ts
- src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsx
- src/renderer/src/store/slices/editor.ts
- src/main/git/status.test.ts
- src/shared/gitmodules-parser.ts
- src/relay/git-handler-status-ops.ts
- src/renderer/src/store/slices/worktrees.ts
- src/renderer/src/i18n/locales/zh.json
- src/renderer/src/components/right-sidebar/FileExplorerRow.tsx
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/i18n/locales/es.json
- src/renderer/src/i18n/locales/ja.json
|
|
||
| export type PendingDiscardConfirmation = | ||
| | { kind: 'entry'; entry: GitStatusEntry } | ||
| | { kind: 'submodule-entry'; submoduleWorktreePath: string; entry: GitStatusEntry } |
There was a problem hiding this comment.
Show the submodule path in destructive discard confirmations.
The new submodule-entry variant carries only entry.path into the dialog details, so discarding README.md from packages/nested looks identical to discarding any other README.md. Include the submodule-relative path in the pending payload and render that full context before confirming an irreversible discard.
🐛 Proposed fix
+import { joinPath } from '`@/lib/path`'
+
export type PendingDiscardConfirmation =
| { kind: 'entry'; entry: GitStatusEntry }
- | { kind: 'submodule-entry'; submoduleWorktreePath: string; entry: GitStatusEntry }
+ | {
+ kind: 'submodule-entry'
+ submodulePath: string
+ submoduleWorktreePath: string
+ entry: GitStatusEntry
+ }
| { kind: 'area'; area: DiscardAllArea; paths: readonly string[] }
@@
const PendingDiscardIcon = pendingDiscardCopy?.confirmLabel.startsWith('Delete') ? Trash : Undo2
+ const entryDisplayPath =
+ pendingDiscard?.kind === 'submodule-entry'
+ ? joinPath(pendingDiscard.submodulePath, pendingDiscard.entry.path)
+ : pendingDiscard?.kind === 'entry'
+ ? pendingDiscard.entry.path
+ : null
@@
- ) : pendingDiscard?.kind === 'entry' || pendingDiscard?.kind === 'submodule-entry' ? (
+ ) : entryDisplayPath ? (
<div className="rounded-md border border-border/70 bg-muted/35 px-3 py-2 text-xs">
- <div className="break-all font-medium text-foreground">{pendingDiscard.entry.path}</div>
+ <div className="break-all font-medium text-foreground">{entryDisplayPath}</div>
</div>
) : null}And pass the submodule path from SourceControl.tsx:
- const requestDiscardSubmoduleEntry = useCallback(
- (submoduleWorktreePath: string, entry: GitStatusEntry): void => {
+ const requestDiscardSubmoduleEntry = useCallback(
+ (submodulePath: string, submoduleWorktreePath: string, entry: GitStatusEntry): void => {
setPendingDiscard({
kind: 'submodule-entry',
+ submodulePath,
submoduleWorktreePath,
entry
})
@@
onDiscard={(rowEntry) =>
- requestDiscardSubmoduleEntry(section.worktreePath, rowEntry)
+ requestDiscardSubmoduleEntry(
+ section.submodule.path,
+ section.worktreePath,
+ rowEntry
+ )
}Also applies to: 103-105
| const sourceControlGitSubmodules = useMemo((): GitSubmoduleEntry[] => { | ||
| if (gitSubmodules.length > 0) { | ||
| return gitSubmodules | ||
| } | ||
| return entries | ||
| .filter((entry) => entry.submodule) | ||
| .map((entry) => ({ name: entry.path, path: entry.path })) | ||
| }, [entries, gitSubmodules]) |
There was a problem hiding this comment.
Only fetch nested status for dirty submodule rows.
sourceControlGitSubmodules currently returns every .gitmodules entry whenever metadata exists, so opening Source Control runs one nested git status per clean submodule and repeats that work on refresh. Since the parent porcelain rows are the change source here, filter the metadata list to submodules that already have a parent entry.submodule row.
🐛 Proposed fix
const sourceControlGitSubmodules = useMemo((): GitSubmoduleEntry[] => {
+ const dirtySubmoduleEntries = entries.filter((entry) => entry.submodule)
if (gitSubmodules.length > 0) {
- return gitSubmodules
+ const dirtySubmodulePaths = new Set(dirtySubmoduleEntries.map((entry) => entry.path))
+ return gitSubmodules.filter((submodule) => dirtySubmodulePaths.has(submodule.path))
}
- return entries
- .filter((entry) => entry.submodule)
- .map((entry) => ({ name: entry.path, path: entry.path }))
+ return dirtySubmoduleEntries.map((entry) => ({ name: entry.path, path: entry.path }))
}, [entries, gitSubmodules])| useEffect(() => { | ||
| if (rightSidebarTab !== 'source-control') { | ||
| return | ||
| } | ||
| void refreshSubmoduleSourceControlStatuses() | ||
| }, [entries, gitSubmoduleSignature, refreshSubmoduleSourceControlStatuses, rightSidebarTab]) |
There was a problem hiding this comment.
Gate submodule status refreshes on the sidebar being visible.
This effect runs whenever the tab is source-control, even when the right sidebar is closed. The component stays mounted while closed, so dirty submodules can still spawn hidden git/SSH status calls; mirror the existing isBranchVisible gate.
🐛 Proposed fix
useEffect(() => {
- if (rightSidebarTab !== 'source-control') {
+ if (!isBranchVisible) {
return
}
void refreshSubmoduleSourceControlStatuses()
- }, [entries, gitSubmoduleSignature, refreshSubmoduleSourceControlStatuses, rightSidebarTab])
+ }, [entries, gitSubmoduleSignature, isBranchVisible, refreshSubmoduleSourceControlStatuses])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (rightSidebarTab !== 'source-control') { | |
| return | |
| } | |
| void refreshSubmoduleSourceControlStatuses() | |
| }, [entries, gitSubmoduleSignature, refreshSubmoduleSourceControlStatuses, rightSidebarTab]) | |
| useEffect(() => { | |
| if (!isBranchVisible) { | |
| return | |
| } | |
| void refreshSubmoduleSourceControlStatuses() | |
| }, [entries, gitSubmoduleSignature, isBranchVisible, refreshSubmoduleSourceControlStatuses]) |
| const handleOpenSubmoduleDiff = useCallback( | ||
| ( | ||
| submoduleWorktreePath: string, | ||
| entry: GitStatusEntry, | ||
| event?: SourceControlRowOpenEvent | ||
| ): void => { | ||
| if (!activeWorktreeId) { | ||
| return | ||
| } | ||
| const targetGroupId = resolveSplitTargetGroupId(event) | ||
| const openAsPreview = shouldOpenSourceControlRowAsPreview(event, targetGroupId) | ||
| const language = detectLanguage(entry.path) | ||
| const filePath = joinPath(submoduleWorktreePath, entry.path) | ||
| if (language === 'markdown' && entry.area === 'unstaged') { | ||
| openFile( | ||
| { | ||
| filePath, | ||
| relativePath: entry.path, | ||
| worktreeId: activeWorktreeId, | ||
| language, | ||
| mode: 'edit' | ||
| }, | ||
| { targetGroupId, preview: openAsPreview } | ||
| ) | ||
| setEditorViewMode(filePath, 'changes') | ||
| return | ||
| } | ||
| openDiff(activeWorktreeId, filePath, entry.path, language, entry.area === 'staged', { | ||
| targetGroupId, | ||
| preview: openAsPreview | ||
| }) | ||
| }, | ||
| [activeWorktreeId, openDiff, openFile, resolveSplitTargetGroupId, setEditorViewMode] | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect whether openDiff/diff loading derives git context from
# active worktreeId/relativePath or from the absolute filePath/nested repo path.
# Expected: submodule diff opening must carry or derive the nested worktree path.
rg -nP -C4 '\bopenDiff\s*[:=]\s*\(|\bopenDiff\s*\(' --type=ts --type=tsx
rg -nP -C4 '\b(getRuntimeGitDiff|window\.api\.git\.(diff|show)|diffSource|relativePath|worktreePath)\b' --type=ts --type=tsxRepository: stablyai/orca
Length of output: 113
🏁 Script executed:
# Search for openDiff definition and usages
rg -n 'openDiff\s*[:=]|function\s+openDiff|const\s+openDiff' --type=ts
# Also search in tsx files by using file listing
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec rg -l 'openDiff' {} \;Repository: stablyai/orca
Length of output: 2236
🏁 Script executed:
# Get the function signature and first few lines of openDiff
rg -n -A 20 '(const|function)\s+openDiff\s*[:=]|openDiff\s*\(' . --type=tsRepository: stablyai/orca
Length of output: 40445
🏁 Script executed:
# Get the full openDiff implementation in editor.ts
sed -n '2383,2450p' src/renderer/src/store/slices/editor.tsRepository: stablyai/orca
Length of output: 2829
🏁 Script executed:
# Look at how openDiff is actually implemented and what it does with parameters
rg -A 30 'openDiff:\s*\(' src/renderer/src/store/slices/editor.tsRepository: stablyai/orca
Length of output: 2304
🏁 Script executed:
# Search for how diff content is loaded/fetched downstream
rg -n 'getRuntimeGitDiff|getDiffContent|loadDiff|diffSource' src/renderer/src --type=ts -A 3Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
# Find the getRuntimeGitDiff implementation
sed -n '238,280p' src/renderer/src/runtime/runtime-git-client.tsRepository: stablyai/orca
Length of output: 1400
🏁 Script executed:
# Check what parameters are used in the git diff request
rg -A 25 'export async function getRuntimeGitDiff' src/renderer/src/runtime/runtime-git-client.tsRepository: stablyai/orca
Length of output: 892
🏁 Script executed:
# Look for how filePath vs worktreeId is used for diff requests
rg -B 5 -A 15 'window\.api\.git\.(diff|show)' src/renderer/src --type=tsRepository: stablyai/orca
Length of output: 1703
🏁 Script executed:
# Find where RuntimeGitContext is built for diff loading
rg -B 5 -A 10 'getRuntimeGitDiff' src/renderer/src/components/editor/useEditorPanelContentState.ts | head -60Repository: stablyai/orca
Length of output: 1273
🏁 Script executed:
# Check what fileSettings and worktreePath is used
rg -B 10 'getRuntimeGitDiff.*{' src/renderer/src/components/editor/useEditorPanelContentState.ts | grep -A 15 'const gitScope'Repository: stablyai/orca
Length of output: 161
🏁 Script executed:
# Look at the full context where getRuntimeGitDiff is called
sed -n '200,240p' src/renderer/src/components/editor/useEditorPanelContentState.tsRepository: stablyai/orca
Length of output: 1574
🏁 Script executed:
# Find where gitScope and fileSettings are determined
rg -B 15 'fileSettings.*file.worktreeId' src/renderer/src/components/editor/useEditorPanelContentState.tsRepository: stablyai/orca
Length of output: 39
🏁 Script executed:
# Find where worktreePath is obtained and how fileSettings relates to worktreeId
sed -n '148,190p' src/renderer/src/components/editor/useEditorPanelContentState.tsRepository: stablyai/orca
Length of output: 1878
🏁 Script executed:
# Look for getFileSettings or settingsForRuntimeOwner
rg -B 3 -A 8 'fileSettings.*=' src/renderer/src/components/editor/useEditorPanelContentState.ts | head -50Repository: stablyai/orca
Length of output: 784
🏁 Script executed:
# Examine the full handleOpenSubmoduleDiff function context to see how it compares to stage/unstage
sed -n '4900,4950p' src/renderer/src/components/right-sidebar/SourceControl.tsxRepository: stablyai/orca
Length of output: 1683
🏁 Script executed:
# Look at how stage/unstage handle submodule worktreeId
sed -n '4949,5000p' src/renderer/src/components/right-sidebar/SourceControl.tsxRepository: stablyai/orca
Length of output: 1752
Pass worktreeId: null for submodule diffs to match stage/unstage behavior.
handleOpenSubmoduleDiff calls openDiff(activeWorktreeId, ...) where activeWorktreeId is the parent repo's ID. However, stage and unstage operations correctly pass worktreeId: null with the submodule path (see the comment at line 4955: "submodule rows are normal git rows in the nested repo"). When getRuntimeGitDiff receives a non-null worktreeId, it uses that for git operations instead of the worktreePath, causing the diff to be loaded from the parent repo context rather than the nested submodule.
| const handleStageAllSubmodulePaths = useCallback( | ||
| async (submoduleWorktreePath: string, filePaths: readonly string[]): Promise<void> => { | ||
| if (filePaths.length === 0) { | ||
| return | ||
| } | ||
| try { | ||
| await bulkStageRuntimeGitPaths( | ||
| { | ||
| // Why: submodule bulk actions must mutate the nested repo, not the | ||
| // parent repo that owns the gitlink row. | ||
| settings: activeRepoSettings, | ||
| worktreeId: null, | ||
| worktreePath: submoduleWorktreePath, | ||
| connectionId: activeConnectionId ?? undefined | ||
| }, | ||
| [...filePaths] | ||
| ) | ||
| await refreshSubmoduleSourceControlStatuses() | ||
| await refreshActiveGitStatusAfterMutation() | ||
| } catch { | ||
| // git operation failed silently | ||
| } | ||
| }, | ||
| [ | ||
| activeConnectionId, | ||
| activeRepoSettings, | ||
| refreshActiveGitStatusAfterMutation, | ||
| refreshSubmoduleSourceControlStatuses | ||
| ] | ||
| ) | ||
|
|
||
| const handleUnstageAllSubmodulePaths = useCallback( | ||
| async (submoduleWorktreePath: string, filePaths: readonly string[]): Promise<void> => { | ||
| if (filePaths.length === 0) { | ||
| return | ||
| } | ||
| try { | ||
| await bulkUnstageRuntimeGitPaths( | ||
| { | ||
| // Why: submodule bulk actions must mutate the nested repo, not the | ||
| // parent repo that owns the gitlink row. | ||
| settings: activeRepoSettings, | ||
| worktreeId: null, | ||
| worktreePath: submoduleWorktreePath, | ||
| connectionId: activeConnectionId ?? undefined | ||
| }, | ||
| [...filePaths] | ||
| ) | ||
| await refreshSubmoduleSourceControlStatuses() | ||
| await refreshActiveGitStatusAfterMutation() | ||
| } catch { | ||
| // git operation failed silently | ||
| } | ||
| }, | ||
| [ | ||
| activeConnectionId, | ||
| activeRepoSettings, | ||
| refreshActiveGitStatusAfterMutation, | ||
| refreshSubmoduleSourceControlStatuses | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Use the bulk in-flight guard for submodule bulk actions.
The parent bulk handlers disable and serialize git writes with isExecutingBulk, but the new submodule bulk handlers can be double-clicked and run concurrent index mutations against the same nested repo.
🐛 Proposed fix
const handleStageAllSubmodulePaths = useCallback(
async (submoduleWorktreePath: string, filePaths: readonly string[]): Promise<void> => {
- if (filePaths.length === 0) {
+ if (isExecutingBulk || filePaths.length === 0) {
return
}
+ setIsExecutingBulk(true)
try {
await bulkStageRuntimeGitPaths(
@@
await refreshSubmoduleSourceControlStatuses()
await refreshActiveGitStatusAfterMutation()
} catch {
// git operation failed silently
+ } finally {
+ setIsExecutingBulk(false)
}
},
[
activeConnectionId,
activeRepoSettings,
+ isExecutingBulk,
refreshActiveGitStatusAfterMutation,
refreshSubmoduleSourceControlStatuses
]
)
@@
const handleUnstageAllSubmodulePaths = useCallback(
async (submoduleWorktreePath: string, filePaths: readonly string[]): Promise<void> => {
- if (filePaths.length === 0) {
+ if (isExecutingBulk || filePaths.length === 0) {
return
}
+ setIsExecutingBulk(true)
try {
await bulkUnstageRuntimeGitPaths(
@@
await refreshSubmoduleSourceControlStatuses()
await refreshActiveGitStatusAfterMutation()
} catch {
// git operation failed silently
+ } finally {
+ setIsExecutingBulk(false)
}
},
[
activeConnectionId,
activeRepoSettings,
+ isExecutingBulk,
refreshActiveGitStatusAfterMutation,
refreshSubmoduleSourceControlStatuses
]
)
@@
<ActionButton
icon={Plus}
@@
onClick={(event) => {
event.stopPropagation()
void handleStageAllSubmodulePaths(section.worktreePath, stageAllPaths)
}}
+ disabled={isExecutingBulk}
/>
@@
<ActionButton
icon={Minus}
@@
onClick={(event) => {
event.stopPropagation()
void handleUnstageAllSubmodulePaths(
section.worktreePath,
unstageAllPaths
)
}}
+ disabled={isExecutingBulk}
/>Also applies to: 6011-6040
| const hasFilteredSubmoduleEntries = submoduleSourceControlSections.length > 0 | ||
| const hasFilteredBranchEntries = filteredBranchEntries.length > 0 | ||
| const showGenericEmptyState = | ||
| !hasUncommittedEntries && branchSummary?.status === 'ready' && branchEntries.length === 0 | ||
| !hasUncommittedEntries && | ||
| !hasFilteredSubmoduleEntries && | ||
| branchSummary?.status === 'ready' && | ||
| branchEntries.length === 0 | ||
| const showCleanBranchEmptyState = showGenericEmptyState |
There was a problem hiding this comment.
Include submodule matches in the filter empty-state gate.
hasFilteredSubmoduleEntries prevents the clean-branch empty state, but the "No matching files" branch still only checks uncommitted and branch entries. A filter that matches only a nested submodule entry will render both the submodule section and the no-match empty state.
🐛 Proposed fix
- {normalizedFilter && !hasFilteredUncommittedEntries && !hasFilteredBranchEntries && (
+ {normalizedFilter &&
+ !hasFilteredUncommittedEntries &&
+ !hasFilteredSubmoduleEntries &&
+ !hasFilteredBranchEntries && (
<EmptyState
heading="No matching files"
supportingText={`No changed files match "${filterQuery}"`}
/>Co-authored-by: Orca <help@stably.ai>
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e312b918-2ef8-4b0a-958a-73eea4e2f168
📒 Files selected for processing (8)
src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsontests/e2e/source-control-submodules.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/source-control-submodules.spec.ts
- src/renderer/src/i18n/locales/zh.json
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/i18n/locales/es.json
- src/renderer/src/i18n/locales/ja.json
| const [submoduleCommitDrafts, setSubmoduleCommitDrafts] = useState<Record<string, string>>({}) | ||
| const [submoduleCommitErrors, setSubmoduleCommitErrors] = useState<Record<string, string | null>>( | ||
| {} | ||
| ) | ||
| const [submoduleCommitInFlight, setSubmoduleCommitInFlight] = useState<Record<string, boolean>>( | ||
| {} | ||
| ) |
There was a problem hiding this comment.
Scope submodule commit state by worktree, not only path.
packages/nested in two different worktrees currently shares the same draft, error, and in-flight state. Since this component stays mounted across worktree switches, a draft or spinner from one workspace can leak into another and be committed with the wrong submodule.
🐛 Suggested direction
+const getSubmoduleCommitStateKey = (worktreeId: string | null | undefined, submodulePath: string) =>
+ `${worktreeId ?? ''}\0${submodulePath}`
-const submoduleCommitMessage = submoduleCommitDrafts[section.submodule.path] ?? ''
+const submoduleStateKey = getSubmoduleCommitStateKey(activeWorktreeId, section.submodule.path)
+const submoduleCommitMessage = submoduleCommitDrafts[submoduleStateKey] ?? ''Then use submoduleStateKey consistently for submoduleCommitDrafts, submoduleCommitErrors, and submoduleCommitInFlight.
| const handleCommitSubmodule = useCallback( | ||
| async ( | ||
| submodulePath: string, | ||
| submoduleWorktreePath: string, | ||
| stagedEntries: readonly GitStatusEntry[] | ||
| ): Promise<void> => { | ||
| const message = (submoduleCommitDrafts[submodulePath] ?? '').trim() | ||
| if (!message || stagedEntries.length === 0 || submoduleCommitInFlight[submodulePath]) { | ||
| return | ||
| } | ||
| setSubmoduleCommitInFlight((prev) => ({ ...prev, [submodulePath]: true })) | ||
| setSubmoduleCommitErrors((prev) => ({ ...prev, [submodulePath]: null })) |
There was a problem hiding this comment.
Add a synchronous in-flight guard for submodule commits.
This mirrors the parent commit path’s ref guard. setSubmoduleCommitInFlight is async, so a rapid double-click can start two non-idempotent commitRuntimeGit calls before the disabled state renders.
🐛 Suggested fix
+ const submoduleCommitInFlightRef = useRef<Record<string, boolean>>({})
+
const handleCommitSubmodule = useCallback(
@@
- if (!message || stagedEntries.length === 0 || submoduleCommitInFlight[submodulePath]) {
+ if (
+ !message ||
+ stagedEntries.length === 0 ||
+ submoduleCommitInFlight[submodulePath] ||
+ submoduleCommitInFlightRef.current[submodulePath]
+ ) {
return
}
+ submoduleCommitInFlightRef.current[submodulePath] = true
setSubmoduleCommitInFlight((prev) => ({ ...prev, [submodulePath]: true }))
@@
} finally {
setSubmoduleCommitInFlight((prev) => ({ ...prev, [submodulePath]: false }))
+ submoduleCommitInFlightRef.current[submodulePath] = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCommitSubmodule = useCallback( | |
| async ( | |
| submodulePath: string, | |
| submoduleWorktreePath: string, | |
| stagedEntries: readonly GitStatusEntry[] | |
| ): Promise<void> => { | |
| const message = (submoduleCommitDrafts[submodulePath] ?? '').trim() | |
| if (!message || stagedEntries.length === 0 || submoduleCommitInFlight[submodulePath]) { | |
| return | |
| } | |
| setSubmoduleCommitInFlight((prev) => ({ ...prev, [submodulePath]: true })) | |
| setSubmoduleCommitErrors((prev) => ({ ...prev, [submodulePath]: null })) | |
| const submoduleCommitInFlightRef = useRef<Record<string, boolean>>({}) | |
| const handleCommitSubmodule = useCallback( | |
| async ( | |
| submodulePath: string, | |
| submoduleWorktreePath: string, | |
| stagedEntries: readonly GitStatusEntry[] | |
| ): Promise<void> => { | |
| const message = (submoduleCommitDrafts[submodulePath] ?? '').trim() | |
| if ( | |
| !message || | |
| stagedEntries.length === 0 || | |
| submoduleCommitInFlight[submodulePath] || | |
| submoduleCommitInFlightRef.current[submodulePath] | |
| ) { | |
| return | |
| } | |
| submoduleCommitInFlightRef.current[submodulePath] = true | |
| setSubmoduleCommitInFlight((prev) => ({ ...prev, [submodulePath]: true })) | |
| setSubmoduleCommitErrors((prev) => ({ ...prev, [submodulePath]: null })) |
| const stageAllPaths = section.visibleEntries | ||
| .filter(isStageableStatusEntry) | ||
| .map((entry) => entry.path) | ||
| const stagedEntries = section.groups.staged | ||
| const submoduleHasStageableChanges = | ||
| section.groups.unstaged.some(isStageableStatusEntry) || | ||
| section.groups.untracked.some(isStageableStatusEntry) | ||
| const submoduleHasUnresolvedConflicts = section.visibleEntries.some( | ||
| (entry) => entry.conflictStatus === 'unresolved' | ||
| ) | ||
| const submoduleCommitMessage = submoduleCommitDrafts[section.submodule.path] ?? '' | ||
| const submodulePrimaryAction = resolveSubmoduleRepositoryPrimaryAction({ | ||
| stagedCount: stagedEntries.length, | ||
| hasStageableChanges: submoduleHasStageableChanges, | ||
| hasMessage: submoduleCommitMessage.trim().length > 0, | ||
| hasUnresolvedConflicts: submoduleHasUnresolvedConflicts, | ||
| isCommitting: submoduleCommitInFlight[section.submodule.path] ?? false | ||
| }) |
There was a problem hiding this comment.
Make the submodule primary action use unfiltered action state.
The primary action is enabled from unfiltered groups, but Stage All receives section.visibleEntries, which is filter-dependent. With a file filter active, the button can stage only the visible subset while labeled as “Stage All”; conflict detection also ignores hidden unresolved conflicts.
🐛 Suggested fix
- const stageAllPaths = section.visibleEntries
- .filter(isStageableStatusEntry)
- .map((entry) => entry.path)
+ const allSubmoduleEntries = [
+ ...section.groups.staged,
+ ...section.groups.unstaged,
+ ...section.groups.untracked
+ ]
+ const stageAllPaths = [...section.groups.unstaged, ...section.groups.untracked]
+ .filter(isStageableStatusEntry)
+ .map((entry) => entry.path)
const stagedEntries = section.groups.staged
- const submoduleHasStageableChanges =
- section.groups.unstaged.some(isStageableStatusEntry) ||
- section.groups.untracked.some(isStageableStatusEntry)
- const submoduleHasUnresolvedConflicts = section.visibleEntries.some(
+ const submoduleHasStageableChanges = stageAllPaths.length > 0
+ const submoduleHasUnresolvedConflicts = allSubmoduleEntries.some(
(entry) => entry.conflictStatus === 'unresolved'
)Also applies to: 6137-6143
|
related PR "#6350" |
2 participants
Summary
.gitmodulesas{ name, path, url }and include it in git status results for local and relay/SSH worktrees.Gitsection.Ssubmodule badge for clean and dirty submodules..gitmodules.Validation
pnpm exec vitest run --config config/vitest.config.ts src/main/ipc/filesystem-auth.test.ts src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxpnpm exec vitest run --config config/vitest.config.ts src/shared/gitmodules-parser.test.ts src/main/git/status-submodules.integration.test.ts src/main/git/status.test.ts src/relay/git-handler-status-ops.test.ts src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx src/renderer/src/components/right-sidebar/FileExplorer.test.tsx src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx src/renderer/src/components/right-sidebar/FileExplorerVirtualRowsAddProject.test.tsxpnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsxpnpm run typecheckpnpm run lintpnpm run test:e2e -- tests/e2e/source-control-submodules.spec.ts --workers=1Manual Electron Check
000-DIRTY-SUBMODULE GIThas its own action surface.Stage All, confirmed only the submodule README staged inside the submodule.Commit, confirmed the submodule commit landed and the nested section disappeared once clean.