refactor(extensions/podman): move the mac podman notification #13320
Conversation
📝 WalkthroughWalkthroughThis change refactors the handling of the Podman Mac Helper notification logic. The manual notification setup and command registration previously present in the extension activation code are now encapsulated in a new method, Estimated code review effort2 (~15 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
extensions/podman/packages/extension/src/extension.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs extensions/podman/packages/extension/src/utils/notifications.spec.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs extensions/podman/packages/extension/src/utils/notifications.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧠 Learnings (2)📓 Common learningsextensions/podman/packages/extension/src/utils/notifications.spec.ts (1)Learnt from: benoitf 💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsextensions/podman/packages/extension/src/utils/notifications.spec.ts (1)Learnt from: benoitf ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Simon Rey <sfbrey+eqqe@gmail.com>
0a98816 to
c37c556
Compare
There was a problem hiding this comment.
Hey @eqqe - I've reviewed your changes - here's some feedback:
- Consider managing the disposable returned by setupNotificationMacPodman within ExtensionNotifications to ensure it’s properly cleaned up.
- Rename setupNotificationMacPodan to more clearly indicate that it registers a command (e.g., registerMacHelperNotificationCommand) for better readability.
- Explicitly declare and initialize the doNotShowMacHelperSetup property in ExtensionNotifications to prevent potential undefined behavior when it’s accessed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider managing the disposable returned by setupNotificationMacPodman within ExtensionNotifications to ensure it’s properly cleaned up.
- Rename setupNotificationMacPodan to more clearly indicate that it registers a command (e.g., registerMacHelperNotificationCommand) for better readability.
- Explicitly declare and initialize the doNotShowMacHelperSetup property in ExtensionNotifications to prevent potential undefined behavior when it’s accessed.
## Individual Comments
### Comment 1
<location> `extensions/podman/packages/extension/src/utils/notifications.ts:71` </location>
<code_context>
);
}
+ public setupNotificationMacPodman(): extensionApi.Disposable {
+ // Get if we should never show the podman-mac-helper notification ever again
+ this.doNotShowMacHelperSetup = this.getDoNotShowMacHelperSetting();
</code_context>
<issue_to_address>
Consider removing the extra class state and helper methods by reading the 'don't show' flag at notification time and registering the command inline.
```markdown
You can collapse the extra method and state by reading the “don’t-show” flag at show-time and registering the command in your `activate` (or wherever you drive notifications). For example:
```ts
// at top‐level of your extension (e.g. inside activate())
const DONT_SHOW_KEY = 'setting.doNotShowMacHelperNotification';
// 1) register the "don't show again" command inline
const ignoreDisposable = extensionApi.commands.registerCommand(
'podman.doNotShowMacHelperNotification',
async () => {
await extensionApi.configuration
.getConfiguration('podman')
.update(DONT_SHOW_KEY, true);
// disposes the notification when user clicks "Don't show again"
notificationDisposable.dispose();
},
);
// 2) only show the notification if the flag is false
const shouldShow = !extensionApi.configuration
.getConfiguration('podman')
.get<boolean>(DONT_SHOW_KEY, false);
let notificationDisposable: extensionApi.Disposable;
if (shouldShow) {
notificationDisposable = extensionApi.window.showNotification({
title: 'Podman Desktop needs Mac helper setup',
body: 'Please install/configure the podman-mac-helper for full functionality.',
actions: [{ title: 'Don’t show again', command: 'podman.doNotShowMacHelperNotification' }],
});
context.subscriptions.push(ignoreDisposable, notificationDisposable);
}
```
Then you can remove:
- the `doNotShowMacHelperSetup` field and its setter/getter
- the `setupNotificationMacPodman()` helper
- the `getDoNotShowMacHelperSetting()` method
This keeps identical behavior but removes extra class state and indirection.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ); | ||
| } | ||
|
|
||
| public setupNotificationMacPodman(): extensionApi.Disposable { |
There was a problem hiding this comment.
issue (complexity): Consider removing the extra class state and helper methods by reading the 'don't show' flag at notification time and registering the command inline.
You can collapse the extra method and state by reading the “don’t-show” flag at show-time and registering the command in your `activate` (or wherever you drive notifications). For example:
```ts
// at top‐level of your extension (e.g. inside activate())
const DONT_SHOW_KEY = 'setting.doNotShowMacHelperNotification';
// 1) register the "don't show again" command inline
const ignoreDisposable = extensionApi.commands.registerCommand(
'podman.doNotShowMacHelperNotification',
async () => {
await extensionApi.configuration
.getConfiguration('podman')
.update(DONT_SHOW_KEY, true);
// disposes the notification when user clicks "Don't show again"
notificationDisposable.dispose();
},
);
// 2) only show the notification if the flag is false
const shouldShow = !extensionApi.configuration
.getConfiguration('podman')
.get<boolean>(DONT_SHOW_KEY, false);
let notificationDisposable: extensionApi.Disposable;
if (shouldShow) {
notificationDisposable = extensionApi.window.showNotification({
title: 'Podman Desktop needs Mac helper setup',
body: 'Please install/configure the podman-mac-helper for full functionality.',
actions: [{ title: 'Don’t show again', command: 'podman.doNotShowMacHelperNotification' }],
});
context.subscriptions.push(ignoreDisposable, notificationDisposable);
}Then you can remove:
- the
doNotShowMacHelperSetupfield and its setter/getter - the
setupNotificationMacPodman()helper - the
getDoNotShowMacHelperSetting()method
This keeps identical behavior but removes extra class state and indirection.
There are a few tests already, what scenario were you thinking? |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
What does this PR do?
For the refactoring of the podman extension, I have introduced a class for the notifications. This PR moves the mac setup of notification and its tests to the newly created class.
Screenshot / video of UI
What issues does this PR fix or reference?
Closes #13250
How to test this PR?