Feat: interactive notification setup#14
Merged
utkarsh232005 merged 7 commits intoMay 14, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an interactive kdm config setup flow for choosing a notification service and expands the config model/tests to recognize that selection.
Changes:
- Adds
notification_serviceto stored configuration. - Adds
config setupusing@vr_patel/tuiselection prompts. - Adds unit test coverage for the new setup command registration and Discord selection path.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/config.ts |
Adds the notification service setting to the config interface. |
src/commands/config.ts |
Adds the interactive setup subcommand and follow-up guidance. |
src/__tests__/config.test.ts |
Mocks the TUI selector and tests setup command registration/Discord selection. |
package.json |
Adds the TUI dependency. |
package-lock.json |
Locks the new TUI dependency version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ], | ||
| }); | ||
|
|
||
| setConfig('notification_service', choice); |
| if (choice === 'discord') { | ||
| console.log(chalk.yellow('! Remember to set your webhook URL:'), chalk.cyan('kdm config set discord_webhook <url>')); | ||
| } else if (choice === 'email') { | ||
| console.log(chalk.yellow('! Remember to set your SMTP details (email_host, email_port, email_user, email_to)')); |
Comment on lines
+17
to
+21
| message: "Select notification service:", | ||
| options: [ | ||
| { label: "Discord", value: "discord", description: "Send alerts to a Discord channel via Webhook" }, | ||
| { label: "Email (SMTP)", value: "email", description: "Send alerts via Email SMTP" }, | ||
| { label: "None", value: "none", description: "Disable notifications" }, |
Comment on lines
+28
to
+31
| if (choice === 'discord') { | ||
| console.log(chalk.yellow('! Remember to set your webhook URL:'), chalk.cyan('kdm config set discord_webhook <url>')); | ||
| } else if (choice === 'email') { | ||
| console.log(chalk.yellow('! Remember to set your SMTP details (email_host, email_port, email_user, email_to)')); |
|
|
||
| config | ||
| .command('setup') | ||
| .description('Interactively setup notification service') |
Contributor
|
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:
Warning
|
| Layer / File(s) | Summary |
|---|---|
Config schema and TUI dependency src/utils/config.ts, package.json |
KDMConfig gains optional notification_service ('discord' | 'email' | 'none') and exports clearNotificationCredentials(). @vr_patel/tui added to dependencies; @types/react devDependency bumped. |
Interactive setup command src/commands/config.ts |
Adds config setup using select/input from TUI, validates Discord webhook and SMTP fields (including port parsing), clears prior notifier credentials, stores values via setConfig, and logs success/error. |
Setup command testing src/__tests__/config.test.ts |
Mocks @vr_patel/tui select/input, adds async tests for config setup covering none/discord/email flows, updates command-registration expectation to include setup, and adapts existing set/list/clear tests to use configUtils namespace mocks. |
Show command test tweak src/__tests__/show.test.ts |
Replaces dynamic import with static tableUtils import and asserts tableUtils.renderTable is called on Kubernetes connection failure. |
Repository review config .coderabbit.yaml |
Adds CodeRabbit review configuration with path filters and path-specific instructions for commands, config utils, and tests. |
Sequence Diagram
sequenceDiagram
participant User
participant ConfigSetup as config setup
participant TUISelect as select()
participant TUIInput as input()
participant ConfigClear as clearNotificationCredentials()
participant ConfigStore as setConfig()
participant Console
User->>ConfigSetup: run `config setup`
ConfigSetup->>TUISelect: prompt for notification service
TUISelect-->>ConfigSetup: returns selected service
ConfigSetup->>ConfigClear: clearNotificationCredentials()
alt discord
ConfigSetup->>TUIInput: prompt for Discord webhook
TUIInput-->>ConfigSetup: webhook URL
ConfigSetup->>ConfigStore: setConfig('discord_webhook', url)
else email
ConfigSetup->>TUIInput: prompt for SMTP host/port/user/to
TUIInput-->>ConfigSetup: smtp values
ConfigSetup->>ConfigStore: setConfig('email_host'/'email_port'/'email_user'/'email_to', values)
else none
ConfigSetup->>ConfigStore: setConfig('notification_service','none')
end
ConfigSetup->>ConfigStore: setConfig('notification_service', selected_service)
ConfigSetup->>Console: log success or error
Estimated Code Review Effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
A rabbit hops through prompts with care,
Selects Discord or Email, types with flair,
Secrets cleared, new keys tucked in tight,
Green success glows, errors flee the night,
Hooray — the notifier is set just right! 🐰✨
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Feat: interactive notification setup' directly and clearly summarizes the main change—adding an interactive TUI-based notification service configuration flow with service selection and credential validation. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/config.test.ts`:
- Line 5: The test misses mocking input() so when select() returns 'discord'
setup calls input() in src/commands/config.ts and throws; update the test import
to include input (alongside select) from '@vr_patel/tui', add a jest.mock for
input (or mockImplementation) where select is mocked, set input to return the
webhook URL expected by the command, and add an assertion that input was called
with the expected prompt and/or that its return value was used (e.g.,
expect(input).toHaveBeenCalled() or matching args) to allow the success
console.log path to run.
In `@src/commands/config.ts`:
- Line 25: The code currently calls setConfig('notification_service', choice)
immediately which persists the selection even if subsequent setup fails or is
cancelled; instead, keep the user's choice in a local variable (e.g.,
notificationChoice) and run the rest of the setup flow (the logic surrounding
the existing setup/validation and the error handling at the block that catches
failures around lines 64-66); only call setConfig('notification_service',
notificationChoice) after the setup completes successfully, and ensure no
persistence occurs in the failure/cancel paths.
- Around line 40-44: The input validator for SMTP port (the input(...) call that
assigns portStr) currently uses parseInt and accepts values like "587abc";
change the validator to require a fully numeric string (e.g. /^\d+$/) and then
convert to a Number and validate the range 1–65535 before accepting; also ensure
the code that stores the port (the SMTP port variable assignment that uses
portStr) saves it as a numeric value (Number or parseInt result) after the range
check.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4cc1d3e-1a4b-41aa-bb6b-4db29f9d45f6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/__tests__/config.test.tssrc/commands/config.tssrc/utils/config.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
src/commands/config.ts:57
- Switching to email setup does not clear an existing
discord_webhook, and the current alert code sends Discord notifications whenever that key is present. A user who previously configured Discord and then selects Email here will still receive Discord alerts as well, contrary to the single-service selection flow.
setConfig('email_host', host);
setConfig('email_port', parseInt(portStr, 10));
setConfig('email_user', user);
setConfig('email_to', to);
src/commands/config.ts:32
- Switching to Discord setup does not clear existing SMTP settings, and the alert code sends email whenever the SMTP fields and password are present. A user who previously configured email and then selects Discord here will still receive email alerts too, so the selected service is not actually exclusive.
setConfig('discord_webhook', webhook);
src/commands/config.ts:51
- The email validation accepts any string containing
@, including invalid values like@oruser@. Those values are then persisted and later passed to Nodemailer, so setup can report success while saving unusable addresses.
validate: (v) => v.includes('@') || "Must be a valid email",
});
const to = await input({
message: "Alert Recipient Email:",
validate: (v) => v.includes('@') || "Must be a valid email",
Comment on lines
+21
to
+25
| { label: "None", value: "none", description: "Disable notifications" }, | ||
| ], | ||
| }); | ||
|
|
||
| setConfig('notification_service', choice); |
| ], | ||
| }); | ||
|
|
||
| setConfig('notification_service', choice); |
Comment on lines
+43
to
+55
| validate: (v) => !isNaN(parseInt(v)) || "Must be a number", | ||
| }); | ||
| const user = await input({ | ||
| message: "SMTP User:", | ||
| validate: (v) => v.includes('@') || "Must be a valid email", | ||
| }); | ||
| const to = await input({ | ||
| message: "Alert Recipient Email:", | ||
| validate: (v) => v.includes('@') || "Must be a valid email", | ||
| }); | ||
|
|
||
| setConfig('email_host', host); | ||
| setConfig('email_port', parseInt(portStr, 10)); |
| options: [ | ||
| { label: "Discord", value: "discord", description: "Send alerts to a Discord channel via Webhook" }, | ||
| { label: "Email (SMTP)", value: "email", description: "Send alerts via Email SMTP" }, | ||
| { label: "None", value: "none", description: "Disable notifications" }, |
Comment on lines
+28
to
+31
| const webhook = await input({ | ||
| message: "Discord Webhook URL:", | ||
| validate: (v) => v.startsWith('https://discord.com/api/webhooks/') || v.startsWith('https://ptb.discord.com/api/webhooks/') || "Must be a valid Discord webhook URL", | ||
| }); |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/config.test.ts`:
- Line 42: The tests re-import `setConfig` inside the setup blocks which breaks
the mock call tracking; remove the inline imports at the three spots and use the
originally imported mocked `setConfig` (the one brought in at the top of the
file) for assertions and call checks (replace any `await
import('../utils/config')` usages with references to the top-level `setConfig`
mock so all expectations target the same mock instance).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6199e713-c914-48d2-a0e4-516aab8fcd72
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/__tests__/config.test.tssrc/commands/config.tssrc/utils/config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/commands/config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/config.test.ts (1)
43-82: ⚡ Quick winAdd one abort-path test to lock in atomic-save behavior.
Line 43-82 currently covers only success flows. Add a failure/cancel scenario and assert that no config mutations occur, so the atomic-save requirement is explicitly protected.
Suggested additional test
+ it('should not persist partial config if setup aborts mid-flow', async () => { + vi.mocked(tui.select).mockResolvedValue('discord'); + vi.mocked(tui.input).mockRejectedValue(new Error('cancelled')); + + await program.parseAsync(['node', 'test', 'config', 'setup']); + + expect(configUtils.clearNotificationCredentials).not.toHaveBeenCalled(); + expect(configUtils.setConfig).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/config.test.ts` around lines 43 - 82, Add a new test that simulates the abort/failure path of the config setup (e.g., mock tui.select or tui.input to return a cancel value or throw) and assert that no configuration mutations occur: verify configUtils.clearNotificationCredentials and configUtils.setConfig were NOT called after await program.parseAsync(['node','test','config','setup']); use the same helpers referenced in the file (tui.select, tui.input, program.parseAsync, configUtils.clearNotificationCredentials, configUtils.setConfig) to locate where to insert the test and to assert atomic-save behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/config.test.ts`:
- Around line 67-82: The test currently verifies setting email fields but
doesn't assert that previous notification credentials are cleared; update the
'should call select, multiple inputs and setConfig on email setup' test to also
assert that configUtils.clearNotificationCredentials() is invoked when switching
services: keep the existing mocks for tui.select and tui.input, run
program.parseAsync(['node','test','config','setup']) as before, then add an
expectation like
expect(configUtils.clearNotificationCredentials).toHaveBeenCalled() after the
parse to ensure credentials cleanup occurs alongside the setConfig assertions
for 'notification_service', 'email_host', 'email_port', 'email_user', and
'email_to'.
---
Nitpick comments:
In `@src/__tests__/config.test.ts`:
- Around line 43-82: Add a new test that simulates the abort/failure path of the
config setup (e.g., mock tui.select or tui.input to return a cancel value or
throw) and assert that no configuration mutations occur: verify
configUtils.clearNotificationCredentials and configUtils.setConfig were NOT
called after await program.parseAsync(['node','test','config','setup']); use the
same helpers referenced in the file (tui.select, tui.input, program.parseAsync,
configUtils.clearNotificationCredentials, configUtils.setConfig) to locate where
to insert the test and to assert atomic-save behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4fa6479-203f-4a85-86d6-a741f2105cdd
📒 Files selected for processing (2)
src/__tests__/config.test.tssrc/commands/config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/config.ts
Comment on lines
+67
to
82
| it('should call select, multiple inputs and setConfig on email setup', async () => { | ||
| vi.mocked(tui.select).mockResolvedValue('email'); | ||
| vi.mocked(tui.input) | ||
| .mockResolvedValueOnce('smtp.gmail.com') // host | ||
| .mockResolvedValueOnce('587') // port | ||
| .mockResolvedValueOnce('user@test.com') // user | ||
| .mockResolvedValueOnce('to@test.com'); // to | ||
|
|
||
| await program.parseAsync(['node', 'test', 'config', 'setup']); | ||
|
|
||
| expect(configUtils.setConfig).toHaveBeenCalledWith('notification_service', 'email'); | ||
| expect(configUtils.setConfig).toHaveBeenCalledWith('email_host', 'smtp.gmail.com'); | ||
| expect(configUtils.setConfig).toHaveBeenCalledWith('email_port', 587); | ||
| expect(configUtils.setConfig).toHaveBeenCalledWith('email_user', 'user@test.com'); | ||
| expect(configUtils.setConfig).toHaveBeenCalledWith('email_to', 'to@test.com'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert credential cleanup for the email setup path.
Line 67-82 validates email field persistence, but it doesn’t verify that configUtils.clearNotificationCredentials() is called when switching services. This leaves the cleanup requirement only partially covered.
Suggested test adjustment
it('should call select, multiple inputs and setConfig on email setup', async () => {
vi.mocked(tui.select).mockResolvedValue('email');
vi.mocked(tui.input)
.mockResolvedValueOnce('smtp.gmail.com') // host
.mockResolvedValueOnce('587') // port
.mockResolvedValueOnce('user@test.com') // user
.mockResolvedValueOnce('to@test.com'); // to
await program.parseAsync(['node', 'test', 'config', 'setup']);
+ expect(tui.select).toHaveBeenCalled();
+ expect(tui.input).toHaveBeenCalledTimes(4);
+ expect(configUtils.clearNotificationCredentials).toHaveBeenCalled();
expect(configUtils.setConfig).toHaveBeenCalledWith('notification_service', 'email');
expect(configUtils.setConfig).toHaveBeenCalledWith('email_host', 'smtp.gmail.com');
expect(configUtils.setConfig).toHaveBeenCalledWith('email_port', 587);
expect(configUtils.setConfig).toHaveBeenCalledWith('email_user', 'user@test.com');
expect(configUtils.setConfig).toHaveBeenCalledWith('email_to', 'to@test.com');
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/config.test.ts` around lines 67 - 82, The test currently
verifies setting email fields but doesn't assert that previous notification
credentials are cleared; update the 'should call select, multiple inputs and
setConfig on email setup' test to also assert that
configUtils.clearNotificationCredentials() is invoked when switching services:
keep the existing mocks for tui.select and tui.input, run
program.parseAsync(['node','test','config','setup']) as before, then add an
expectation like
expect(configUtils.clearNotificationCredentials).toHaveBeenCalled() after the
parse to ensure credentials cleanup occurs alongside the setConfig assertions
for 'notification_service', 'email_host', 'email_port', 'email_user', and
'email_to'.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/commands/config.ts:43
- This sequence is not atomic: credentials are cleared and then the new Discord values are written in separate operations. If the second
setConfigfails, the command catches the error after leaving a partially updated config, which contradicts the setup flow's atomicity guarantee and can leave notifications misconfigured.
clearNotificationCredentials();
setConfig('discord_webhook', webhook);
setConfig('notification_service', 'discord');
src/commands/config.ts:73
- This sequence is not atomic: existing credentials are cleared and the email fields are then written one by one. If any later
setConfigcall fails, the old notification config is already deleted and a partial email config may remain, so the setup flow can leave notifications broken after a failed persistence step.
clearNotificationCredentials();
setConfig('email_host', host);
setConfig('email_port', parseInt(portStr, 10));
setConfig('email_user', user);
setConfig('email_to', to);
setConfig('notification_service', 'email');
| import Conf from 'conf'; | ||
|
|
||
| interface KDMConfig { | ||
| notification_service?: 'discord' | 'email' | 'none'; |
Comment on lines
+54
to
+58
| it('should call select, input and setConfig on discord setup', async () => { | ||
| vi.mocked(tui.select).mockResolvedValue('discord'); | ||
| vi.mocked(tui.input).mockResolvedValue('https://discord.com/api/webhooks/123456789/token-here'); | ||
|
|
||
| await program.parseAsync(['node', 'test', 'config', 'setup']); |
Comment on lines
+25
to
+27
| if (choice === 'none') { | ||
| clearNotificationCredentials(); | ||
| setConfig('notification_service', 'none'); |
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
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.
This PR introduces a comprehensive interactive set up flow for notifications:
Summary by CodeRabbit
New Features
Tests
Chores