Merged
Conversation
Reviewer's GuideAdds consistent try/catch error handling and user-facing error messages around most Git command handlers in the VS Code extension, and removes some legacy config/test artifacts. Sequence diagram for generic Git command error handlingsequenceDiagram
actor VSCodeUser
participant VSCodeExtension
participant RepositoryContext
participant git
participant vscode_window
VSCodeUser->>VSCodeExtension: run cmdStatus
VSCodeExtension->>VSCodeExtension: try
VSCodeExtension->>RepositoryContext: getGitRoot()
RepositoryContext-->>VSCodeExtension: gitRoot
VSCodeExtension->>git: runGitCommand(gitRoot, [status], Git_Status)
git-->>VSCodeExtension: result
VSCodeExtension-->>VSCodeUser: command completes
alt command throws error
VSCodeExtension->>vscode_window: showErrorMessage(Failed to show status: error)
end
Sequence diagram for Add-Commit-Push command with error handlingsequenceDiagram
actor VSCodeUser
participant VSCodeExtension
participant RepositoryContext
participant git
participant resolvePushTarget
participant vscode_window
VSCodeUser->>VSCodeExtension: run cmdAddCommitPush
VSCodeExtension->>VSCodeExtension: try
VSCodeExtension->>RepositoryContext: getGitRoot()
RepositoryContext-->>VSCodeExtension: gitRoot
VSCodeExtension->>vscode_window: showInformationMessage(Running tests...)
VSCodeExtension->>git: checkTestsBeforeCommit(gitRoot)
git-->>VSCodeExtension: testResult
alt noTests
VSCodeExtension->>vscode_window: showInformationMessage(No tests found - proceeding with commit)
else testsPassed
VSCodeExtension->>vscode_window: showInformationMessage(Tests passed successfully)
end
alt !testResult.canProceed
VSCodeExtension->>vscode_window: showErrorMessage(testResult.message or Commit blocked by tests)
VSCodeExtension-->>VSCodeUser: return
else testResult.message contains Warning
VSCodeExtension->>vscode_window: showWarningMessage(testResult.message, Commit_Anyway, Cancel)
vscode_window-->>VSCodeExtension: choice
alt choice != Commit_Anyway
VSCodeExtension-->>VSCodeUser: return
end
end
VSCodeExtension->>git: execGit(gitRoot, [add, -A])
git-->>VSCodeExtension: ok
VSCodeExtension->>vscode_window: showInformationMessage(Changes added)
VSCodeExtension->>git: getProcessedPrefix(gitRoot)
git-->>VSCodeExtension: prefix
VSCodeExtension->>vscode_window: showInputBox(Enter commit message)
vscode_window-->>VSCodeExtension: message
alt no message
VSCodeExtension-->>VSCodeUser: return
else message provided
VSCodeExtension->>git: execGit(gitRoot, [commit, -m, message])
git-->>VSCodeExtension: ok
VSCodeExtension->>vscode_window: showInformationMessage(Changes committed)
VSCodeExtension->>git: getCurrentBranch(gitRoot)
git-->>VSCodeExtension: branch
VSCodeExtension->>resolvePushTarget: resolvePushTarget(gitRoot, branch)
resolvePushTarget-->>VSCodeExtension: pushTarget or null
alt no pushTarget
VSCodeExtension-->>VSCodeUser: return
else pushTarget
VSCodeExtension->>vscode_window: withProgress(Pushing to pushTarget.display)
VSCodeExtension->>git: execGit(gitRoot, [push, pushTarget.remote, branch])
git-->>VSCodeExtension: ok
VSCodeExtension->>vscode_window: showInformationMessage(Changes pushed to pushTarget.display)
end
end
opt any error in try block
VSCodeExtension->>vscode_window: showErrorMessage(Failed to add/commit/push: error)
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- There is a lot of repeated try/catch and test-before-commit logic across commands (status/add/commit/addCommitPush/history rewrite, etc.); consider extracting small helpers (e.g. a
withGitErrorHandling(label, fn)wrapper and arunTestsOrAbort(gitRoot)utility) to reduce duplication and keep behavior consistent. - Some error messages have been generalized (e.g. merge now reports
Failed to mergeinstead ofMerge conflict or errorand squash usesFailed to start squashinstead ofFailed to start rebase); double-check whether you want to preserve the more specific context to help users understand what kind of failure occurred.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is a lot of repeated try/catch and test-before-commit logic across commands (status/add/commit/addCommitPush/history rewrite, etc.); consider extracting small helpers (e.g. a `withGitErrorHandling(label, fn)` wrapper and a `runTestsOrAbort(gitRoot)` utility) to reduce duplication and keep behavior consistent.
- Some error messages have been generalized (e.g. merge now reports `Failed to merge` instead of `Merge conflict or error` and squash uses `Failed to start squash` instead of `Failed to start rebase`); double-check whether you want to preserve the more specific context to help users understand what kind of failure occurred.
## Individual Comments
### Comment 1
<location> `src/extension.ts:597-598` </location>
<code_context>
+ try {
+ const gitRoot = await RepositoryContext.getGitRoot();
+ await git.runGitCommand(gitRoot, ['status'], 'Git Status');
+ } catch (error) {
+ vscode.window.showErrorMessage(`Failed to show status: ${error}`);
+ }
}
</code_context>
<issue_to_address>
**suggestion:** Interpolating raw `error` risks showing `[object Object]`; consider normalizing error messages.
This pattern appears in several commands (e.g. ``Failed to show status: ${error}``). When `error` isn’t a string, users may see `[object Object]` instead of a useful message. A small helper like `formatError(error)` that returns `error.message` for `Error` instances and `String(error)` otherwise, reused wherever `showErrorMessage` is called, would keep messages consistent and readable.
Suggested implementation:
```typescript
=======
function formatError(error: unknown): string {
if (error instanceof Error && typeof error.message === 'string' && error.message.length > 0) {
return error.message;
}
try {
return String(error);
} catch {
return 'Unknown error';
}
}
// ========== Command Implementations ==========
```
```typescript
async function cmdStatus() {
try {
const gitRoot = await RepositoryContext.getGitRoot();
await git.runGitCommand(gitRoot, ['status'], 'Git Status');
} catch (error) {
vscode.window.showErrorMessage(`Failed to show status: ${formatError(error)}`);
}
}
```
You mentioned this error handling pattern appears in several commands. For consistency, all similar `catch (error)` blocks that currently do something like ``vscode.window.showErrorMessage(\`...: ${error}\`)`` should be updated to call `formatError(error)` instead. The exact search/replace will depend on the rest of `src/extension.ts`, but the pattern should mirror the `cmdStatus` change above.
</issue_to_address>
### Comment 2
<location> `src/extension.ts:593` </location>
<code_context>
// ========== Command Implementations ==========
async function cmdStatus() {
- const gitRoot = await RepositoryContext.getGitRoot();
- await git.runGitCommand(gitRoot, ['status'], 'Git Status');
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing shared helpers for error-handling and test-check logic so individual cmd* functions stay flatter and less repetitive.
You can keep all the new safety/error handling while significantly reducing repetition by extracting two small helpers.
### 1. Centralize per-command error handling
Most commands now follow this pattern:
```ts
async function cmdStatus() {
try {
const gitRoot = await RepositoryContext.getGitRoot();
await git.runGitCommand(gitRoot, ['status'], 'Git Status');
} catch (error) {
vscode.window.showErrorMessage(`Failed to show status: ${error}`);
}
}
```
Instead of repeating `try/catch` in every command, you can centralize it:
```ts
async function runCommand<T>(
action: string,
fn: () => Promise<T>
): Promise<T | undefined> {
try {
return await fn();
} catch (error) {
vscode.window.showErrorMessage(`Failed to ${action}: ${error}`);
return undefined;
}
}
```
Then commands become flatter and easier to scan:
```ts
async function cmdStatus() {
return runCommand('show status', async () => {
const gitRoot = await RepositoryContext.getGitRoot();
await git.runGitCommand(gitRoot, ['status'], 'Git Status');
});
}
async function cmdAdd() {
return runCommand('add changes', async () => {
const gitRoot = await RepositoryContext.getGitRoot();
const status = await git.getStatus(gitRoot);
if (!status) {
vscode.window.showInformationMessage('No changes to add');
return;
}
// ... rest of current cmdAdd body unchanged ...
});
}
```
You can apply this pattern to most of the `cmd*` functions that currently wrap their entire body in a `try/catch`.
### 2. Extract the “tests before commit” flow
The test-check logic is now duplicated in several places (`cmdCommit`, `cmdAddCommitPush`, `cmdHistoryRewriteToSingle`, `cmdCommitStaged`):
```ts
// Check tests before committing
let testResult: any;
// Show running notification (without buttons so it auto-dismisses after ~5 seconds)
vscode.window.showInformationMessage('⏳ Running tests...');
testResult = await git.checkTestsBeforeCommit(gitRoot);
// Show result message (also auto-dismisses)
if (testResult.noTests) {
vscode.window.showInformationMessage('✅ No tests found - proceeding with commit');
} else if (testResult.passed) {
vscode.window.showInformationMessage('✅ Tests passed successfully');
}
if (!testResult.canProceed) {
vscode.window.showErrorMessage(testResult.message || 'Commit blocked by tests');
return;
}
if (testResult.message && testResult.message.includes('Warning')) {
const choice = await vscode.window.showWarningMessage(
testResult.message,
'Commit Anyway', 'Cancel'
);
if (choice !== 'Commit Anyway') {
return;
}
}
```
You can move this into a helper that returns a boolean:
```ts
async function ensureTestsAllowCommit(gitRoot: string): Promise<boolean> {
vscode.window.showInformationMessage('⏳ Running tests...');
const testResult: any = await git.checkTestsBeforeCommit(gitRoot);
if (testResult.noTests) {
vscode.window.showInformationMessage('✅ No tests found - proceeding with commit');
} else if (testResult.passed) {
vscode.window.showInformationMessage('✅ Tests passed successfully');
}
if (!testResult.canProceed) {
vscode.window.showErrorMessage(testResult.message || 'Commit blocked by tests');
return false;
}
if (testResult.message && testResult.message.includes('Warning')) {
const choice = await vscode.window.showWarningMessage(
testResult.message,
'Commit Anyway', 'Cancel'
);
if (choice !== 'Commit Anyway') {
return false;
}
}
return true;
}
```
Then your commands read much more clearly:
```ts
async function cmdCommit() {
return runCommand('commit', async () => {
const gitRoot = await RepositoryContext.getGitRoot();
const status = await git.execGit(gitRoot, ['status', '--porcelain']);
const hasStagedChanges = status.split('\n').some(line => line.match(/^[MADRC]/));
if (!hasStagedChanges) {
vscode.window.showWarningMessage(
'No staged changes to commit. Use "Add → Commit → Push" to stage all changes first.'
);
return;
}
if (!(await ensureTestsAllowCommit(gitRoot))) {
return;
}
const prefix = await git.getProcessedPrefix(gitRoot);
const message = await vscode.window.showInputBox({
prompt: 'Enter commit message',
placeHolder: 'Commit message',
value: prefix
});
if (!message) {
return;
}
await git.runGitCommand(gitRoot, ['commit', '-m', message], 'Git Commit');
vscode.window.showInformationMessage('Changes committed');
});
}
```
And similarly in `cmdAddCommitPush`, `cmdHistoryRewriteToSingle`, `cmdCommitStaged`:
```ts
if (!(await ensureTestsAllowCommit(gitRoot))) {
return;
}
// continue with existing flow...
```
These two helpers keep all your new behavior (per-command messages and test gating) but remove a lot of boilerplate and nested `try/catch` blocks, making the file smaller and more maintainable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Summary by Sourcery
Add robust error handling and user-facing error messages across Git command handlers in the VS Code extension.
Bug Fixes:
Enhancements:
Tests:
Chores: