Kdm config setup#106
Conversation
|
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:
Note
|
| Layer / File(s) | Summary |
|---|---|
Config setup reconfiguration flow src/commands/config.ts, src/__tests__/config.test.ts |
Detect existing notification_service (not none) and ask whether to reconfigure; declining exits early, confirming continues to service selection and credential prompts. Tests cover decline (no setConfig), confirm (sets service and webhook), no-existing-config (skips reconfigure prompt), and prompt/setConfig error handling. |
Config set deprecation warnings src/commands/config.ts, src/__tests__/config.test.ts |
config set logs a deprecation warning when setting credential-related keys (notification service, discord webhook, email SMTP fields) advising config setup, while still parsing values (e.g., email_port → int) and calling setConfig. Non-credential keys (e.g., alert_cooldown) parse and persist without warnings. |
Config list formatting src/commands/config.ts |
Tweaked per-entry console formatting for config list output (spacing/padding adjustments). |
Welcome banner simplification src/ui/banner.ts |
Replaced multi-line ASCII signature with a single-line horizontal rule and removed the embedded trailing newline from the version output. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant ConfigSetup as "config setup"
participant TUI as "tui.select"
participant Storage as "Config storage (getConfig/setConfig)"
User->>ConfigSetup: run 'config setup'
ConfigSetup->>Storage: read notification_service
alt notification_service exists and != 'none'
ConfigSetup->>TUI: ask "reconfigure?"
TUI->>User: show reconfigure prompt
alt User declines
User->>TUI: selects 'cancel'
TUI->>ConfigSetup: return cancel
ConfigSetup->>User: exit without setConfig
else User confirms
User->>TUI: selects 'reconfigure'
TUI->>ConfigSetup: return confirm
ConfigSetup->>TUI: prompt service selection
ConfigSetup->>TUI: prompt credentials (if needed)
ConfigSetup->>Storage: setConfig(service & creds)
end
else no notification_service
ConfigSetup->>TUI: skip reconfigure prompt
ConfigSetup->>TUI: prompt service selection
ConfigSetup->>Storage: setConfig(service & creds)
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- KDM-cli/kdm-cli#14: Related prior changes to the interactive notification setup and tests that this PR extends.
- KDM-cli/kdm-cli#23: Earlier edits to the config setup flow and tests; this PR adds reconfiguration prompts and deprecation warnings.
Suggested reviewers
- utkarsh232005
- Rishiraj-Pathak-27
Poem
✨ A prompt asks gently, "Reconfigure true?"
A warning whispers, "Use setup — see it through."
Banner trimmed to a single tidy line,
Tests watch flows where prompts and warnings chime.
Small fixes, tidy shine.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'Kdm config setup' is partially related to the changeset but lacks specificity about the main additions: reconfiguration detection, credential deprecation warnings, and banner simplification. | Consider a more descriptive title like 'Add config setup reconfiguration flow and credential deprecation warnings' to better convey the scope and main objectives of this PR. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| 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
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@Yuvraj-Sarathe please consider the codescene review and. apply the fix asap thankyou:) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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`:
- Around line 92-125: Add tests that exercise error paths: mock tui.select to
reject (e.g., vi.mocked(tui.select).mockRejectedValueOnce(new Error('select
failed'))), call program.parseAsync(['node','test','config','setup']) and assert
console.error was called and the command terminated (spy on process.exit or
assert no further setConfig calls); similarly add a test where
configUtils.setConfig is mocked to throw
(vi.mocked(configUtils.setConfig).mockImplementation(() => { throw new
Error('write failed') })), run program.parseAsync and assert console.error was
called and the CLI exits gracefully. Reference tui.select,
configUtils.setConfig, program.parseAsync, console.error, and process.exit in
the new assertions.
- Line 123: The test uses a brittle negative assertion on consoleLogSpy with a
partial string ('currently set to') that doesn't match the real prompt; replace
it by asserting the prompt invocation shape instead: remove the
expect(consoleLogSpy).not.toHaveBeenCalledWith(expect.stringContaining('currently
set to')) check and instead assert against the tui.select mock (e.g.,
expect(tui.select).toHaveBeenCalledTimes(...) or
expect(tui.select).toHaveBeenCalledWith(expect.objectContaining({ message:
'<exact prompt token>' }))), or match the exact message token used by the
command; reference the consoleLogSpy and tui.select mocks to implement this
change.
In `@src/ui/banner.ts`:
- Around line 4-21: The file fails to parse because const banner has no
initializer, signature uses unquoted multiline ASCII inside chalk.gray(...) and
the chalk.blue.bold(...) call is given an unquoted invalid argument; fix by
giving banner a proper string or template literal initializer (e.g., const
banner = `...`), wrap the ASCII art passed to signature in a string/template
literal when calling chalk.gray (signature = chalk.gray(`...`)), and replace the
unquoted argument in console.log(chalk.blue.bold(...)) with a quoted/template
string that includes the version variable via interpolation (e.g.,
chalk.blue.bold(`Kubernetes & Docker Monitor v${version}`)), ensuring the
version variable is defined/imported where used.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f58bc72-1dc6-4a09-890c-be0462fc8ca2
📒 Files selected for processing (3)
src/__tests__/config.test.tssrc/commands/config.tssrc/ui/banner.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…kdm-cli into kdm-config-setup
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Updated the welcome banner design in the showWelcomeBanner function to a new style using cyan color.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/banner.ts (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove signature output per maintainer feedback.
This line contradicts the previous review comment requesting removal of the signature print.
🗑️ Proposed fix
console.log(banner); - console.log(signature); console.log(chalk.blue.bold(` Kubernetes & Docker Monitor v${version}`));🤖 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/ui/banner.ts` at line 18, Remove the stray console.log that prints the signature in src/ui/banner.ts: locate the statement referencing the signature variable (console.log(signature)) and delete it so the banner no longer outputs the signature; ensure no other code depends on that console output and leave the signature variable unused or remove it if unused to avoid linter warnings.
🤖 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.
Outside diff comments:
In `@src/ui/banner.ts`:
- Line 18: Remove the stray console.log that prints the signature in
src/ui/banner.ts: locate the statement referencing the signature variable
(console.log(signature)) and delete it so the banner no longer outputs the
signature; ensure no other code depends on that console output and leave the
signature variable unused or remove it if unused to avoid linter warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6472444-4242-4380-8ed3-afbd6c31091c
📒 Files selected for processing (1)
src/ui/banner.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@utkarsh232005 rewrote the entire file |
|
@Yuvraj-Sarathe config.test.ts test is falling |
run npm test first then push changes |
ran tests all working! |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/config.ts (1)
129-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
email_passwordin deprecation warnings forconfig set.
email_passwordis a credential key in the config contract, but it is excluded fromcredentialKeys. That leaves the most sensitive value settable via CLI args without the deprecation warning.Suggested patch
- const credentialKeys = ['notification_service', 'discord_webhook', 'email_host', 'email_port', 'email_user', 'email_to']; + const credentialKeys = [ + 'notification_service', + 'discord_webhook', + 'email_host', + 'email_port', + 'email_user', + 'email_to', + 'email_password', + ];🤖 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/commands/config.ts` around lines 129 - 136, The deprecation warning list omits the sensitive credential named email_password; update the credentialKeys array used to build credentialKeyPattern in src/commands/config.ts to include 'email_password' so credentialKeyPattern.test(key) will match it and trigger the existing deprecation console warnings when key === email_password (symbols: credentialKeys, credentialKeyPattern, key).
🤖 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/commands/config.ts`:
- Around line 119-121: Replace the direct console.error calls in the catch
blocks with the project's logger (e.g., logger.error or processLogger.error) so
errors follow standard observability; update the two catch blocks that currently
use console.error(`\n✗ Set up cancelled or failed: ${(error as Error).message}`)
(and the similar block at 146-148) to call the project logger with the same
descriptive message and include the full error object/stack (pass error as a
second argument or include error.stack) and add/import the logger symbol if it's
not already available in src/commands/config.ts.
- Around line 140-142: The code applies parseInt to value for keys
'alert_cooldown' and 'email_port' without validation, which can persist NaN;
before assigning finalValue from parseInt(value, 10) in the config handling
logic, validate that value is a valid integer string (e.g., /^\s*[+-]?\d+\s*$/)
or call parsed = parseInt(value,10) and guard with Number.isFinite(parsed) /
!Number.isNaN(parsed); if validation fails, either keep finalValue as the
original value, set a safe default, or surface a clear validation error—update
the branch that references key, value, and finalValue to use this guarded
parsing.
---
Outside diff comments:
In `@src/commands/config.ts`:
- Around line 129-136: The deprecation warning list omits the sensitive
credential named email_password; update the credentialKeys array used to build
credentialKeyPattern in src/commands/config.ts to include 'email_password' so
credentialKeyPattern.test(key) will match it and trigger the existing
deprecation console warnings when key === email_password (symbols:
credentialKeys, credentialKeyPattern, key).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f05ff653-0002-44e2-a013-24f087387962
📒 Files selected for processing (1)
src/commands/config.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Rishiraj-Pathak-27
left a comment
Rishiraj-Pathak-27
left a comment
There was a problem hiding this comment.
Good improvements overall, especially the reconfiguration flow and test coverage.
Requesting a few changes before merge:
- Validate
parseInt()values foralert_cooldownandemail_portto avoid storingNaN. - Replace
console.errorwith the project logger for consistent logging. - CodeScene is still failing due to method complexity, so breaking the setup flow into smaller functions would help maintainability.
Other than that, the feature implementation looks good.
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Complex Method, Bumpy Road Ahead)
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| config.ts | 2 rules in this hotspot | 9.35 → 9.13 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| config.ts | 1 critical rule | 9.35 → 9.13 | Suppress |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| // Check for existing configuration | ||
| const shouldProceed = await promptReconfigurationIfNeeded(); | ||
| if (!shouldProceed) return; | ||
|
|
There was a problem hiding this comment.
❌ Getting worse: Complex Method
registerConfigCommand increases in cyclomatic complexity from 17 to 21, threshold = 9
| // Check for existing configuration | ||
| const shouldProceed = await promptReconfigurationIfNeeded(); | ||
| if (!shouldProceed) return; | ||
|
|
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
registerConfigCommand has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
3 participants
📝 Changes Summary
src/commands/config.ts— 2 changes1. Reconfiguration Detection
getConfig()to check for an existingnotification_servicenoneor no prior config), no prompt is shown — setup continues as usual2. Credential Key Deprecation
Defines a list of 6 credential keys:
notification_servicediscord_webhookemail_hostemail_portemail_useremail_toWhen
config setis used with any credential key:kdm config setupinsteadNon-credential keys (e.g.
alert_cooldown) are not affected by this changesrc/__tests__/config.test.ts— 6 new testsnotification_servicefound)config setalert_cooldownproduces no deprecation warningSummary at a Glance
config setupcommandconfig setcommandSummary by CodeRabbit
Tests
New Features
Style