-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: show notification when the checkpoint initialization fails #7766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own code and found it surprisingly coherent. The universe must be glitching.
src/core/checkpoints/index.ts
Outdated
|
|
||
| // If the error is due to nested git repositories, show a user-friendly message | ||
| // but don't prevent the task from continuing | ||
| if (err.message?.includes("nested git repositories")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error detection using err.message?.includes("nested git repositories") is fragile. If the error message format changes in the future, this check will fail silently. Consider:
- Creating a specific error type for nested git repository detection
- Using error codes instead of message string matching
- Or at minimum, documenting this dependency on the exact error message format
src/core/task/Task.ts
Outdated
| // Kicks off the checkpoints initialization process in the background. | ||
| getCheckpointService(this) | ||
| // Don't await this - let it run in the background so task can continue even if it fails | ||
| getCheckpointService(this).catch((error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fire-and-forget pattern with .catch() allows the task to continue, which is good. However, there's a potential race condition here - the task might try to use checkpoint features before knowing they've failed to initialize.
Consider adding a flag or event to track when initialization has definitively failed, so other parts of the code can check this state if needed.
| if (task && modifiedMessages.length === 0 && !isStreaming && !isHidden) { | ||
| const timer = setTimeout(() => { | ||
| setShowCheckpointWarning(true) | ||
| // Check if the task still exists and hasn't been abandoned before showing warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good defensive programming here! The double-check for task && modifiedMessages.length === 0 prevents potential race conditions when the task is being abandoned. This redundancy is actually valuable for preventing the warning from showing in edge cases.
src/core/checkpoints/index.ts
Outdated
| // but don't prevent the task from continuing | ||
| if (err.message?.includes("nested git repositories")) { | ||
| await vscode.window.showWarningMessage( | ||
| "Checkpoints are disabled because nested git repositories were detected in the workspace. The task will continue without checkpoint support.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message could be more actionable. Consider adding suggestions like:
"Checkpoints are disabled because nested git repositories were detected in the workspace. To enable checkpoints, consider using git submodules or moving nested repositories outside the workspace."
This would help users understand how to resolve the issue.
- Show warning message when nested git repositories are detected - Add i18n support for the warning message in all 18 languages - Still throw error to disable checkpoints but inform user first
d3443c2 to
71df24b
Compare
| "checkpoint_timeout": "Expiration du délai lors de la tentative de rétablissement du checkpoint.", | ||
| "checkpoint_failed": "Échec du rétablissement du checkpoint.", | ||
| "git_not_installed": "Git est requis pour la fonctionnalité des points de contrôle. Veuillez installer Git pour activer les points de contrôle.", | ||
| "nested_git_repos_warning": "Les points de contrôle sont désactivés car des dépôts git imbriqués ont été détectés dans l'espace de travail. Pour utiliser les points de contrôle, veuillez supprimer ou déplacer les dépôts git imbriqués.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typographical suggestion: For consistency, consider capitalizing 'Git' as in other strings. For example, change "dépôts git imbriqués" to "dépôts Git imbriqués".
| "nested_git_repos_warning": "Les points de contrôle sont désactivés car des dépôts git imbriqués ont été détectés dans l'espace de travail. Pour utiliser les points de contrôle, veuillez supprimer ou déplacer les dépôts git imbriqués.", | |
| "nested_git_repos_warning": "Les points de contrôle sont désactivés car des dépôts Git imbriqués ont été détectés dans l'espace de travail. Pour utiliser les points de contrôle, veuillez supprimer ou déplacer les dépôts Git imbriqués.", |
Show notification when checkpoints fail to initialize due to nested repos