DO NOT MERGE 2#609
Conversation
* Use status bar update message to pass notifications to status bar * oops
Version bump and changelog
Production
|
@echolayer-karl /review |
![echolayer-dev-karl[bot]](https://avatars.githubusercontent.com/u/85580729?s=60&v=4){kind=small}
|
|
||
| async execute(): Promise<ExecutedCommandResponse<MESSAGE_TYPES, Notification>> { | ||
| try { | ||
| handleSetStatusBarMessage(this.statusData); |
There was a problem hiding this comment.
Consider validating statusData before passing it to handleSetStatusBarMessage to ensure it meets expected structure and content, enhancing security and robustness.
| remoteCheck | ||
| .filter((it) => !it.hasRemote) | ||
| .forEach((repoCheck) => { | ||
| vscode.window.showErrorMessage( | ||
| `Uh-oh. No remote detected for this repo: ${repoCheck.rootUri}. Please connect a remote to continue.` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Consider handling the case where multiple repositories lack remotes more gracefully. Accumulating error messages and displaying them in a single dialog or notification could improve user experience by reducing potential spamming of error messages.
| sendStatusBarUpdateMessage(clientView); | ||
| break; | ||
| } | ||
| case MESSAGE_TYPES.deleteNotification: { | ||
| const deleteNotificationCommand: DeleteNotificationCommand = new DeleteNotificationCommand(message.value); | ||
| clientView.webview.postMessage(await deleteNotificationCommand.execute()); | ||
| sendStatusBarUpdateMessage(clientView); | ||
| break; | ||
| } | ||
| case MESSAGE_TYPES.deleteNotifications: { | ||
| const deleteNotificationsCommand: DeleteNotificationsCommand = new DeleteNotificationsCommand(message.value); | ||
| clientView.webview.postMessage(await deleteNotificationsCommand.execute()); | ||
| sendStatusBarUpdateMessage(clientView); |
There was a problem hiding this comment.
Ensure that the sendStatusBarUpdateMessage function is defined and properly handles any potential exceptions, as it is being called after postMessage operations which could throw errors.
| const notificationMessage = formatStatusBarMessage(notifications); | ||
| function handleSetStatusBarMessage(statusData: StatusBarData): vscode.Disposable { | ||
| const notificationMessage = formatStatusBarMessage(statusData.notifications); | ||
| return updateStatusBar(notificationMessage, vscode.window); |
There was a problem hiding this comment.
The refactoring from a direct array of notifications to using a StatusBarData interface is a good encapsulation. However, ensure that all callers of handleSetStatusBarMessage are updated to pass the new StatusBarData structure instead of the raw array.
| return this.gitAPI.repositories.map((repository) => { | ||
| return { | ||
| rootUri: repository.rootUri.toString(), | ||
| hasRemote: repository?.state?.remotes?.length > 0, |
There was a problem hiding this comment.
The method checkForRemotes assumes this.gitAPI and this.gitAPI.repositories are not null. To prevent potential runtime errors, add a null check for this.gitAPI and this.gitAPI.repositories before accessing the repositories property.
| import type { Notification } from "@usecodex/common-library"; | ||
| import { MESSAGE_TYPES } from "../../globals"; | ||
| import { handleStatusBarUpdate } from "../../helpers/notifications.helper"; | ||
| import { UserService } from "../../services/user.service"; |
There was a problem hiding this comment.
The removal of handleStatusBarUpdate import suggests that the function is no longer used; ensure that its removal does not affect other parts of the system that might rely on it. If it's indeed unused, the deletion is appropriate.
| import type { Notification } from "@usecodex/common-library"; | ||
| import { MESSAGE_TYPES } from "../../globals"; | ||
| import { handleStatusBarUpdate } from "../../helpers/notifications.helper"; | ||
| import { UserService } from "../../services/user.service"; |
There was a problem hiding this comment.
The removal of handleStatusBarUpdate import suggests that the status bar update functionality is no longer needed. Confirm that this functionality is indeed obsolete or ensure that its removal does not affect other parts of the application that might rely on it.
| import { Notification } from "@usecodex/common-library"; | ||
| import { MESSAGE_TYPES } from "../../globals"; | ||
| import { handleSetStatusBarMessage } from "../../helpers/notifications.helper"; | ||
| import { UserService } from "../../services/user.service"; |
There was a problem hiding this comment.
The removal of 'handleSetStatusBarMessage' import suggests it is no longer used. Confirm its removal is intentional and that there are no unused references to it in the codebase.
| import { Notification } from "@usecodex/common-library"; | ||
| import { MESSAGE_TYPES } from "../../globals"; | ||
| import { handleStatusBarUpdate } from "../../helpers/notifications.helper"; | ||
| import { UserService } from "../../services/user.service"; |
There was a problem hiding this comment.
The removal of handleStatusBarUpdate import suggests it is no longer used. Verify if this is intentional and ensure that any related functionality is either removed or refactored accordingly.
| } | ||
|
|
||
| export { handleStatusBarUpdate, handleSetStatusBarMessage, formatStatusBarMessage, displayInformationMessage }; | ||
| export { handleSetStatusBarMessage, formatStatusBarMessage, displayInformationMessage }; |
There was a problem hiding this comment.
The removal of handleStatusBarUpdate from the export statement suggests that it is no longer used. Confirm that this function has been fully deprecated or replaced, and if so, ensure that all references to it have been removed from the codebase to prevent dead code.
Pull Request Summary by EchoLayerSummaryThis pull request introduces a new StatusBarUpdateCommand class to streamline status bar notifications, refactors notification handling to prepend new items and correct update logic, and enhances repository checks with GitService integration. It also cleans up redundant status bar update calls across various commands and handlers, and updates the messaging models to support the new statusBarUpdate message type. The version bump to 0.5.5 reflects these improvements and bug fixes, including a fix for a bug where notifications would disappear and improved user feedback when no remotes are found. File Changes
New Files
|
3 participants
Description
Type of Change
How Has This Been Tested?
Test Configuration:
Checklist:
Screenshots: