feat: Implement Auto Version Checking & Update Prompt (#26)#27
Closed
Rishiraj-Pathak-27 wants to merge 0 commit into
Closed
feat: Implement Auto Version Checking & Update Prompt (#26)#27Rishiraj-Pathak-27 wants to merge 0 commit into
Rishiraj-Pathak-27 wants to merge 0 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements automatic CLI version checking with an interactive ChangesCLI Version Checking & Update Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces automatic CLI version checking against the npm registry and adds a new kdm update command group to let users manually check updates and view version/system info. It also refactors command registration/startup flow to use dynamic imports and updates docs/dependencies accordingly.
Changes:
- Added npm-registry-based version checking with retry + caching utilities (
src/utils/versionChecker.ts) and runtime version resolution (src/utils/version.ts), then wired an auto-update prompt into CLI startup (src/commands/root.ts). - Added
kdm update {check|info|changelog}command implementation and new unit tests for update behavior. - Updated README and adjusted dependencies in
package.json(notably React-related types/devtools).
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| test-version.js | Adds a version-check “script” (currently committed as an invalid heredoc snippet). |
| src/utils/versionChecker.ts | Implements latest-version fetch, semver comparison, and caching helpers. |
| src/utils/version.ts | Implements runtime current-version detection via package.json. |
| src/utils/config.ts | Extends persisted config for version-check settings. |
| src/docker/containers.ts | Switches Docker container listing from dockerode to docker ps via exec. |
| src/docker/client.ts | Switches Docker connectivity check from dockerode to docker info via exec. |
| src/commands/update.ts | Adds kdm update subcommands (check/info/changelog). |
| src/commands/show.ts | Converts some heavy imports to dynamic imports during execution. |
| src/commands/root.ts | Integrates auto version-check prompt + dynamic command registration. |
| src/tests/update.test.ts | Adds tests for kdm update check behavior. |
| README.md | Documents version checking, update commands, and configuration. |
| package.json | Updates dependencies/devDependencies (React types currently removed). |
Comments suppressed due to low confidence (3)
src/tests/update.test.ts:67
- The update command prints "⚠ A newer version of KDM is available!" (and does not print "UPDATE AVAILABLE"), so this assertion doesn’t match the actual output and will fail. Update the expectation to assert on a string that the
update checkcommand actually logs.
expect(consoleSpy).toHaveBeenCalledWith(
expect.stringContaining('UPDATE AVAILABLE')
);
});
src/tests/update.test.ts:36
process.exitis spied and an exception is expected, but the update command does not callprocess.exit. This spy is unused and the try/catch suggests a control flow that doesn’t exist, which makes the test harder to understand and maintain.
const exitSpy = vi.spyOn(process, 'exit').mockImplementation((code) => {
throw new Error(`Process.exit(${code})`);
});
try {
await program.parseAsync(['node', 'kdm', 'update', 'check']);
} catch {
// Expected to throw
}
package.json:59
@types/reactwas removed, but the codebase includes.tsxfiles importing React (e.g.,src/commands/watch.tsx,src/ui/WatchDashboard.tsx) andtsconfig.jsonenables JSX. Without@types/react, TypeScript builds will fail due to missing React type declarations. Re-add@types/react(and@types/react-domif needed) or switch to a setup that provides React types.
"nodemailer": "^8.0.7",
"react": "^18.2.0",
"react-devtools-core": "^4.19.1"
},
"devDependencies": {
"@types/dockerode": "^3.3.26",
"@types/node": "^20.11.24",
"@types/nodemailer": "^8.0.0",
"tsup": "^8.0.2",
"typescript": "^5.3.3",
"vitest": "^1.3.1"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+3
| cat > test-version.js << 'EOF' | ||
| import { readFileSync } from 'fs'; | ||
| import { join, dirname } from 'path'; |
| email_user?: string; | ||
| email_to?: string; | ||
| alert_cooldown?: number; // in seconds | ||
| lastVersionCheckTime?: number; // Unix timestamp |
Comment on lines
+13
to
+37
| try { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), TIMEOUT); | ||
|
|
||
| const response = await fetch(NPM_REGISTRY_URL, { | ||
| signal: controller.signal, | ||
| }); | ||
|
|
||
| clearTimeout(timeoutId); | ||
|
|
||
| if (!response.ok) { | ||
| console.error(`[DEBUG] NPM Registry returned status ${response.status}`); | ||
| if (attempt < retries) continue; | ||
| return null; | ||
| } | ||
|
|
||
| const data = await response.json() as any; | ||
| const version = data['dist-tags']?.latest; | ||
|
|
||
| if (!version) { | ||
| console.error('[DEBUG] No latest version found in registry'); | ||
| return null; | ||
| } | ||
|
|
||
| return version; |
Comment on lines
+9
to
+29
| try { | ||
| // When built: dist/utils/version.js | ||
| // We need to go: ../../ to reach project root | ||
| const packagePath = join(__dirname, '../../package.json'); | ||
|
|
||
| const content = readFileSync(packagePath, 'utf-8'); | ||
| const pkg = JSON.parse(content); | ||
|
|
||
| return pkg.version; | ||
| } catch (error) { | ||
| // If that fails, try from current working directory | ||
| try { | ||
| const packagePath = join(process.cwd(), 'package.json'); | ||
| const content = readFileSync(packagePath, 'utf-8'); | ||
| const pkg = JSON.parse(content); | ||
| return pkg.version; | ||
| } catch { | ||
| // Last resort: hardcoded fallback | ||
| return '1.2.1'; | ||
| } | ||
| } |
Comment on lines
+26
to
+27
| // Last resort: hardcoded fallback | ||
| return '1.2.1'; |
Comment on lines
+16
to
+22
| global.fetch = vi.fn().mockResolvedValueOnce({ | ||
| json: async () => ({ | ||
| 'dist-tags': { | ||
| latest: '1.2.1' // Same as current | ||
| } | ||
| }) | ||
| }); |
Comment on lines
+38
to
+39
| kdm --v | ||
| -V |
Comment on lines
+23
to
+41
| if (!response.ok) { | ||
| console.error(`[DEBUG] NPM Registry returned status ${response.status}`); | ||
| if (attempt < retries) continue; | ||
| return null; | ||
| } | ||
|
|
||
| const data = await response.json() as any; | ||
| const version = data['dist-tags']?.latest; | ||
|
|
||
| if (!version) { | ||
| console.error('[DEBUG] No latest version found in registry'); | ||
| return null; | ||
| } | ||
|
|
||
| return version; | ||
| } catch (error) { | ||
| const errorMessage = (error as Error).message; | ||
| console.error(`[DEBUG] Attempt ${attempt}/${retries} failed: ${errorMessage}`); | ||
|
|
| "devDependencies": { | ||
| "@types/dockerode": "^3.3.26", | ||
| "@types/node": "^20.11.24", | ||
| "@types/nodemailer": "^8.0.0", |
| @@ -0,0 +1,157 @@ | |||
| import { getConfig, setConfig } from './config'; | |||
| import { getCurrentVersion } from './version'; | |||
![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: 9
🧹 Nitpick comments (7)
README.md (3)
43-45: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block on line 43 is missing a language identifier, which triggers a markdown lint warning (MD040). For output examples, use
textorconsoleas the language identifier.📝 Proposed fix
Output: -``` +```text 1.2.1 ```As per static analysis hints, fenced code blocks should specify a language.
🤖 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 `@README.md` around lines 43 - 45, The fenced code block containing the version string "1.2.1" is missing a language identifier which triggers MD040; edit the fenced block around that content (the triple-backtick block that currently just contains "1.2.1") and add a language token such as text or console (e.g., change ``` to ```text) so the block becomes a labeled code fence.
56-72: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block on line 56 is missing a language identifier. For output examples, use
textorconsole.📝 Proposed fix
Output: -``` +```text ⚠ A newer version of KDM is available! Current Version: 1.2.1 Latest Version: 2.0.0 Update Type: MAJOR Update using: npm install -g kdm-cli@latest 📖 Release notes: https://github.com/KDM-cli/kdm-cli/releases/tag/v2.0.0 📋 Changelog: https://github.com/KDM-cli/kdm-cli/blob/main/CHANGELOG.md ```As per static analysis hints, fenced code blocks should specify a language.
🤖 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 `@README.md` around lines 56 - 72, The fenced code block showing the KDM update message (the block containing "⚠ A newer version of KDM is available!" and the update instructions) lacks a language identifier; update that code fence to include a language token such as text or console (e.g., change ``` to ```text) so syntax highlighters and linters recognize it, leaving the block contents unchanged.
81-93: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block on line 81 is missing a language identifier. For output examples, use
textorconsole.📝 Proposed fix
Output: -``` +```text 📋 KDM Version Information Current Version: 1.2.1 Latest Version: 2.0.0 Status: Update available System Information: Node.js: v20.10.0 Platform: linux Architecture: x64 NPM: 10.2.0 ```As per static analysis hints, fenced code blocks should specify a language.
🤖 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 `@README.md` around lines 81 - 93, The fenced code block containing the "📋 KDM Version Information" output lacks a language identifier; update that block by adding a language tag (e.g., `text` or `console`) after the opening triple backticks so the snippet is marked as plain text/console output—locate the block starting with "📋 KDM Version Information" in README.md and change the opening "```" to "```text" (or "```console").src/__tests__/update.test.ts (1)
28-36: ⚡ Quick winRemove unnecessary
process.exitspy that interferes with test execution.The test mocks
process.exitto throw an error, but Commander's async action handlers don't callprocess.exitby default. This spy is unnecessary and may cause confusion. Remove it to simplify the test.♻️ Proposed fix
const { registerUpdateCommand } = await import('../commands/update'); registerUpdateCommand(program); - const exitSpy = vi.spyOn(process, 'exit').mockImplementation((code) => { - throw new Error(`Process.exit(${code})`); - }); - - try { - await program.parseAsync(['node', 'kdm', 'update', 'check']); - } catch { - // Expected to throw - } + await program.parseAsync(['node', 'kdm', 'update', 'check']); expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining('You are running the latest 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/__tests__/update.test.ts` around lines 28 - 36, Remove the unnecessary process.exit spy in the test: delete the vi.spyOn(process, 'exit').mockImplementation(...) block (the exitSpy variable) and its associated try/catch that expects a thrown error; leave the call to program.parseAsync(['node', 'kdm', 'update', 'check']) to run normally and assert behavior without mocking process.exit. This targets the test around program.parseAsync in update.test.ts and the exitSpy setup so Commander async handlers run unmodified.src/utils/version.ts (1)
27-27: ⚡ Quick winKeep the hardcoded fallback version in sync with package.json.
The hardcoded version (
1.2.1) must match the version inpackage.json. Consider extracting this value to a shared constant or documenting that it must be updated on every release to avoid version drift.🤖 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/utils/version.ts` at line 27, The hardcoded fallback version string returned by the function (currently "return '1.2.1';" in src/utils/version.ts) is out of sync risk with package.json; update this by either reading the version from package.json at build/runtime or extracting the version into a single shared constant (e.g., export const APP_VERSION) used by both package.json build step and this function, and replace the literal return with that constant so the fallback stays in sync with package.json.src/utils/versionChecker.ts (2)
86-89: ⚡ Quick winHandle build metadata in semantic versions.
The
cleanVersionfunction strips prerelease suffixes (e.g.,1.2.3-alpha) by splitting on-, but it doesn't handle build metadata (e.g.,1.2.3+20130313144700). According to semantic versioning, both prerelease and build metadata should be ignored when comparing precedence.♻️ Proposed fix
- const cleanVersion = (v: string) => v.replace(/^v/, '').split('-')[0]; + const cleanVersion = (v: string) => v.replace(/^v/, '').split(/[-+]/)[0]; const currParts = cleanVersion(current).split('.').map(Number); const latestParts = cleanVersion(latest).split('.').map(Number);🤖 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/utils/versionChecker.ts` around lines 86 - 89, The cleanVersion function currently strips a leading "v" and removes prerelease by splitting on '-' but misses build metadata (the "+..." suffix); update cleanVersion (used to derive currParts and latestParts) to remove both prerelease and build metadata by trimming anything after the first '-' or '+' (e.g., use a single regex or split on /[-+]/ and take [0]) so that comparisons ignore both prerelease and build metadata before splitting into numeric parts.
29-29: ⚡ Quick winReplace
as anywith proper type definition.The
as anyassertion on line 29 bypasses TypeScript's type safety. Define a proper interface for the npm registry response to ensure compile-time safety.🔧 Proposed fix
+interface NpmRegistryResponse { + 'dist-tags'?: { + latest?: string; + }; +} + export const checkLatestVersion = async (retries = 3): Promise<string | null> => { for (let attempt = 1; attempt <= retries; attempt++) { try { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), TIMEOUT); const response = await fetch(NPM_REGISTRY_URL, { signal: controller.signal, }); clearTimeout(timeoutId); if (!response.ok) { console.error(`[DEBUG] NPM Registry returned status ${response.status}`); if (attempt < retries) continue; return null; } - const data = await response.json() as any; + const data = await response.json() as NpmRegistryResponse; const version = data['dist-tags']?.latest;🤖 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/utils/versionChecker.ts` at line 29, The current line uses a broad "as any" for the npm registry payload; define a specific interface (e.g., NpmRegistryResponse) describing only the fields the code reads (for example dist-tags: { latest: string }, versions?: Record<string, unknown>, etc.), then replace the cast with a typed JSON parse (const data = await response.json() as NpmRegistryResponse or better: const data: NpmRegistryResponse = await response.json()), and update any subsequent access to use the typed properties (references to data.dist-tags or data.versions) so TypeScript enforces structure and prevents unsafe indexing.
🤖 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 `@package.json`:
- Line 51: Remove the unused dependency "react-devtools-core" from package.json,
then update the lockfile (run npm install or yarn install) so package-lock.json
/ yarn.lock no longer references it; also search the repo for any imports/usages
of react-devtools-core to confirm nothing relies on it and update CI/cache
configs if they explicitly reference this package.
In `@src/__tests__/update.test.ts`:
- Around line 90-96: The test's global.fetch mock in update.test.ts returns an
object without the HTTP success flag, causing code that checks response.ok to
behave incorrectly; update the mockResolvedValueOnce payload used in the
global.fetch call to include an ok: true (or appropriate boolean) property
alongside the json() method so the tested logic that inspects response.ok (in
these update tests) sees a successful response; locate the global.fetch =
vi.fn().mockResolvedValueOnce({...}) setup and add ok: true to the returned
object.
- Around line 16-22: The mocked fetch responses in src/__tests__/update.test.ts
are missing the Response-like properties checked by checkLatestVersion in
versionChecker.ts; update each mockResolvedValueOnce to return an object with
ok: true (or false for error cases) and a status number alongside the json()
function, and apply this change to the other fetch mocks in the same test (the
mocks currently at the other spots referenced in the review) so that each mock
mirrors a real Response (e.g., { ok: true, status: 200, json: async () => ({
'dist-tags': { latest: '1.2.1' } }) }).
- Around line 46-52: The fetch mock in the test (global.fetch =
vi.fn().mockResolvedValueOnce(...)) is missing the response.ok property; update
the mocked response object to include ok: true alongside the json() method
(which returns the 'dist-tags' payload) so the code path that checks response.ok
behaves correctly; locate the mock in src/__tests__/update.test.ts and add ok:
true to the mockResolvedValueOnce return object for the test case that expects a
newer 'latest' version.
- Around line 64-66: The test assertion in update.test.ts is expecting "UPDATE
AVAILABLE" but the actual logged message from update.ts (around the code that
logs the update) is "A newer version of KDM is available!"; update the expect
call that uses consoleSpy and expect.stringContaining to assert the actual
string "A newer version of KDM is available!" (or a substring of it) so the test
matches the log emitted by the code.
In `@src/commands/root.ts`:
- Line 140: Remove the unused hadError flag and simplify exit handling: delete
the hadError declaration and replace the final process.exit(hadError ? 1 : 0)
with a fixed process.exit(0) (the catch block already exits with code 1), so
only the catch path returns 1 and the normal flow returns 0; reference the
hadError variable and the process.exit(...) call in src/commands/root.ts to
locate the changes.
In `@src/commands/show.ts`:
- Around line 84-88: The dynamic imports for getRunningContainers and
getRunningPods in showRunners use Promise.all so a rejection aborts before your
Promise.allSettled fallback runs; change the import step to use
Promise.allSettled and inspect each result, assigning the real functions when
status==='fulfilled' and otherwise assigning a safe fallback (e.g., a no-op that
returns [] or a synchronous require fallback) for getRunningContainers and
getRunningPods, and ensure the spinner/cleanup logic still runs in all paths so
the spinner is finalized even when an import fails.
In `@src/commands/update.ts`:
- Around line 105-108: Wrap the execSync('npm --version') call in a try-catch
around the npmVersion assignment (the const npmVersion =
execSync(...).toString().trim() and subsequent console.log) so failures when npm
is missing or the command errors are handled gracefully; on error set npmVersion
to a safe fallback string (e.g. 'not found' or 'unknown') and log that fallback
with console.log, and optionally include the caught error in a debug-level log
or message for diagnostics.
In `@test-version.js`:
- Around line 1-17: The file contains a shell heredoc wrapper and a trailing
shell command that make the JS invalid; remove the leading "cat >
test-version.js << 'EOF'" line, the closing "EOF" line, and the final "node
test-version.js" line so the file only contains the JavaScript module code that
defines __filename/__dirname, computes packagePath, reads and parses
package.json into pkg, and logs pkg.version; keep the existing try/catch and
identifiers (__filename, __dirname, packagePath, pkg) intact.
---
Nitpick comments:
In `@README.md`:
- Around line 43-45: The fenced code block containing the version string "1.2.1"
is missing a language identifier which triggers MD040; edit the fenced block
around that content (the triple-backtick block that currently just contains
"1.2.1") and add a language token such as text or console (e.g., change ``` to
```text) so the block becomes a labeled code fence.
- Around line 56-72: The fenced code block showing the KDM update message (the
block containing "⚠ A newer version of KDM is available!" and the update
instructions) lacks a language identifier; update that code fence to include a
language token such as text or console (e.g., change ``` to ```text) so syntax
highlighters and linters recognize it, leaving the block contents unchanged.
- Around line 81-93: The fenced code block containing the "📋 KDM Version
Information" output lacks a language identifier; update that block by adding a
language tag (e.g., `text` or `console`) after the opening triple backticks so
the snippet is marked as plain text/console output—locate the block starting
with "📋 KDM Version Information" in README.md and change the opening "```" to
"```text" (or "```console").
In `@src/__tests__/update.test.ts`:
- Around line 28-36: Remove the unnecessary process.exit spy in the test: delete
the vi.spyOn(process, 'exit').mockImplementation(...) block (the exitSpy
variable) and its associated try/catch that expects a thrown error; leave the
call to program.parseAsync(['node', 'kdm', 'update', 'check']) to run normally
and assert behavior without mocking process.exit. This targets the test around
program.parseAsync in update.test.ts and the exitSpy setup so Commander async
handlers run unmodified.
In `@src/utils/version.ts`:
- Line 27: The hardcoded fallback version string returned by the function
(currently "return '1.2.1';" in src/utils/version.ts) is out of sync risk with
package.json; update this by either reading the version from package.json at
build/runtime or extracting the version into a single shared constant (e.g.,
export const APP_VERSION) used by both package.json build step and this
function, and replace the literal return with that constant so the fallback
stays in sync with package.json.
In `@src/utils/versionChecker.ts`:
- Around line 86-89: The cleanVersion function currently strips a leading "v"
and removes prerelease by splitting on '-' but misses build metadata (the "+..."
suffix); update cleanVersion (used to derive currParts and latestParts) to
remove both prerelease and build metadata by trimming anything after the first
'-' or '+' (e.g., use a single regex or split on /[-+]/ and take [0]) so that
comparisons ignore both prerelease and build metadata before splitting into
numeric parts.
- Line 29: The current line uses a broad "as any" for the npm registry payload;
define a specific interface (e.g., NpmRegistryResponse) describing only the
fields the code reads (for example dist-tags: { latest: string }, versions?:
Record<string, unknown>, etc.), then replace the cast with a typed JSON parse
(const data = await response.json() as NpmRegistryResponse or better: const
data: NpmRegistryResponse = await response.json()), and update any subsequent
access to use the typed properties (references to data.dist-tags or
data.versions) so TypeScript enforces structure and prevents unsafe indexing.
🪄 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: 161b3ee9-3986-49ba-83b2-367c9b052bdd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
README.mdpackage.jsonsrc/__tests__/update.test.tssrc/commands/root.tssrc/commands/show.tssrc/commands/update.tssrc/docker/client.tssrc/docker/containers.tssrc/utils/config.tssrc/utils/version.tssrc/utils/versionChecker.tstest-version.js
| "nodemailer": "^8.0.7", | ||
| "react": "^18.2.0" | ||
| "react": "^18.2.0", | ||
| "react-devtools-core": "^4.19.1" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for imports of react-devtools-core
rg -n --type=ts --type=js "react-devtools-core" -g '!node_modules' -g '!package.json'Repository: KDM-cli/kdm-cli
Length of output: 41
Remove unused react-devtools-core dependency.
react-devtools-core is not imported anywhere in the codebase and should be removed to reduce package bloat.
🤖 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 `@package.json` at line 51, Remove the unused dependency "react-devtools-core"
from package.json, then update the lockfile (run npm install or yarn install) so
package-lock.json / yarn.lock no longer references it; also search the repo for
any imports/usages of react-devtools-core to confirm nothing relies on it and
update CI/cache configs if they explicitly reference this package.
Comment on lines
+16
to
+22
| global.fetch = vi.fn().mockResolvedValueOnce({ | ||
| json: async () => ({ | ||
| 'dist-tags': { | ||
| latest: '1.2.1' // Same as current | ||
| } | ||
| }) | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix the mocked fetch response to include the ok property.
The test is failing because the mocked fetch response object is missing the ok property, which is checked by checkLatestVersion on line 23 of versionChecker.ts. The real fetch API returns a Response object with both ok and status properties.
🐛 Proposed fix
global.fetch = vi.fn().mockResolvedValueOnce({
+ ok: true,
+ status: 200,
json: async () => ({
'dist-tags': {
latest: '1.2.1' // Same as current
}
})
});Apply the same fix to all other mocked fetch calls in this file (lines 46, 90).
As per pipeline failures, the tests are failing with "Cannot read properties of undefined (reading 'ok')" because the mock is incomplete.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| global.fetch = vi.fn().mockResolvedValueOnce({ | |
| json: async () => ({ | |
| 'dist-tags': { | |
| latest: '1.2.1' // Same as current | |
| } | |
| }) | |
| }); | |
| global.fetch = vi.fn().mockResolvedValueOnce({ | |
| ok: true, | |
| status: 200, | |
| json: async () => ({ | |
| 'dist-tags': { | |
| latest: '1.2.1' // Same as current | |
| } | |
| }) | |
| }); |
🤖 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__/update.test.ts` around lines 16 - 22, The mocked fetch
responses in src/__tests__/update.test.ts are missing the Response-like
properties checked by checkLatestVersion in versionChecker.ts; update each
mockResolvedValueOnce to return an object with ok: true (or false for error
cases) and a status number alongside the json() function, and apply this change
to the other fetch mocks in the same test (the mocks currently at the other
spots referenced in the review) so that each mock mirrors a real Response (e.g.,
{ ok: true, status: 200, json: async () => ({ 'dist-tags': { latest: '1.2.1' }
}) }).
Comment on lines
+46
to
+52
| global.fetch = vi.fn().mockResolvedValueOnce({ | ||
| json: async () => ({ | ||
| 'dist-tags': { | ||
| latest: '2.0.0' // Newer than current 1.2.1 | ||
| } | ||
| }) | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix the mocked fetch response to include the ok property.
Same issue as the previous test - the mock is missing the ok property.
🐛 Proposed fix
global.fetch = vi.fn().mockResolvedValueOnce({
+ ok: true,
+ status: 200,
json: async () => ({
'dist-tags': {
latest: '2.0.0' // Newer than current 1.2.1
}
})
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| global.fetch = vi.fn().mockResolvedValueOnce({ | |
| json: async () => ({ | |
| 'dist-tags': { | |
| latest: '2.0.0' // Newer than current 1.2.1 | |
| } | |
| }) | |
| }); | |
| global.fetch = vi.fn().mockResolvedValueOnce({ | |
| ok: true, | |
| status: 200, | |
| json: async () => ({ | |
| 'dist-tags': { | |
| latest: '2.0.0' // Newer than current 1.2.1 | |
| } | |
| }) | |
| }); |
🤖 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__/update.test.ts` around lines 46 - 52, The fetch mock in the
test (global.fetch = vi.fn().mockResolvedValueOnce(...)) is missing the
response.ok property; update the mocked response object to include ok: true
alongside the json() method (which returns the 'dist-tags' payload) so the code
path that checks response.ok behaves correctly; locate the mock in
src/__tests__/update.test.ts and add ok: true to the mockResolvedValueOnce
return object for the test case that expects a newer 'latest' version.
Comment on lines
+64
to
+66
| expect(consoleSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('UPDATE AVAILABLE') | ||
| ); |
Contributor
There was a problem hiding this comment.
Correct the expected console output.
The test expects to find the string 'UPDATE AVAILABLE' but the actual console output uses 'A newer version of KDM is available!' (see line 48 in update.ts). Update the assertion to match the actual output.
🧪 Proposed fix
expect(consoleSpy).toHaveBeenCalledWith(
- expect.stringContaining('UPDATE AVAILABLE')
+ expect.stringContaining('A newer version of KDM is available')
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(consoleSpy).toHaveBeenCalledWith( | |
| expect.stringContaining('UPDATE AVAILABLE') | |
| ); | |
| expect(consoleSpy).toHaveBeenCalledWith( | |
| expect.stringContaining('A newer version of KDM is available') | |
| ); |
🧰 Tools
🪛 GitHub Check: test
[failure] 64-64: src/tests/update.test.ts > Update Command > update check > should display update available message for newer version
AssertionError: expected "log" to be called with arguments: [ StringContaining "UPDATE AVAILABLE" ]
Received:
1st log call:
Array [
- StringContaining "UPDATE AVAILABLE",
-
"Checking for updates...
-
",
]2nd log call:
Array [
- StringContaining "UPDATE AVAILABLE",
- "⚠ Could not check for updates. Please try again later.",
]
Number of calls: 2
❯ src/tests/update.test.ts:64:26
🤖 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__/update.test.ts` around lines 64 - 66, The test assertion in
update.test.ts is expecting "UPDATE AVAILABLE" but the actual logged message
from update.ts (around the code that logs the update) is "A newer version of KDM
is available!"; update the expect call that uses consoleSpy and
expect.stringContaining to assert the actual string "A newer version of KDM is
available!" (or a substring of it) so the test matches the log emitted by the
code.
Comment on lines
+90
to
+96
| global.fetch = vi.fn().mockResolvedValueOnce({ | ||
| json: async () => ({ | ||
| 'dist-tags': { | ||
| latest: '2.0.0' // Major update from 1.2.1 | ||
| } | ||
| }) | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix the mocked fetch response to include the ok property.
Same issue as the previous tests - the mock is missing the ok property.
🐛 Proposed fix
global.fetch = vi.fn().mockResolvedValueOnce({
+ ok: true,
+ status: 200,
json: async () => ({
'dist-tags': {
latest: '2.0.0' // Major update from 1.2.1
}
})
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| global.fetch = vi.fn().mockResolvedValueOnce({ | |
| json: async () => ({ | |
| 'dist-tags': { | |
| latest: '2.0.0' // Major update from 1.2.1 | |
| } | |
| }) | |
| }); | |
| global.fetch = vi.fn().mockResolvedValueOnce({ | |
| ok: true, | |
| status: 200, | |
| json: async () => ({ | |
| 'dist-tags': { | |
| latest: '2.0.0' // Major update from 1.2.1 | |
| } | |
| }) | |
| }); |
🤖 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__/update.test.ts` around lines 90 - 96, The test's global.fetch
mock in update.test.ts returns an object without the HTTP success flag, causing
code that checks response.ok to behave incorrectly; update the
mockResolvedValueOnce payload used in the global.fetch call to include an ok:
true (or appropriate boolean) property alongside the json() method so the tested
logic that inspects response.ok (in these update tests) sees a successful
response; locate the global.fetch = vi.fn().mockResolvedValueOnce({...}) setup
and add ok: true to the returned object.
| showWelcomeBanner(currentVersion); | ||
|
|
||
| const spinner = createSpinner('Checking connections...').start(); | ||
| let hadError = false; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove unused hadError variable.
The hadError variable is declared on line 140 but never set to true anywhere in the code. The catch block on lines 180-183 immediately exits with code 1, so line 179's process.exit(hadError ? 1 : 0) will always exit with code 0. Either remove hadError and use a fixed exit code, or properly track error states.
🔧 Proposed fix
showWelcomeBanner(currentVersion);
const spinner = createSpinner('Checking connections...').start();
- let hadError = false;
const [dockerStatus, k8sStatus, minikubeStatus] = await Promise.all([
checkDockerConnection(),
checkK8sConnection(),
checkMinikubeConnection(),
]);
spinner.stop('Connection check complete');
console.log();
const badge = (text: string, color: 'green' | 'red' | 'yellow') => {
const styles = {
green: chalk.bgGreen.black.bold,
red: chalk.bgRed.white.bold,
yellow: chalk.bgYellow.black.bold,
};
return styles[color](` ${text} `);
};
const dockerStr = dockerStatus.connected ? badge('CONNECTED', 'green') : badge('DISCONNECTED', 'red');
const k8sStr = k8sStatus.connected ? badge('CONNECTED', 'green') : badge('DISCONNECTED', 'red');
let minikubeStr = badge('NOT INSTALLED', 'red');
if (minikubeStatus.installed) {
minikubeStr = minikubeStatus.running ? badge('RUNNING', 'green') : badge('STOPPED', 'yellow');
}
console.log(`${chalk.bold('Docker:')} ${dockerStr}`);
console.log(`${chalk.bold('Kubernetes:')} ${k8sStr}`);
console.log(`${chalk.bold('Minikube:')} ${minikubeStr}\n`);
console.log(`${chalk.cyan('')} Running Containers: ${chalk.yellow.bold(dockerStatus.containerCount)}`);
console.log(`${chalk.blue('')} Running Pods: ${chalk.yellow.bold(k8sStatus.podCount)}`);
console.log(`${chalk.red('')} Unhealthy Services: ${chalk.yellow.bold('0')} (Mocked)\n`);
console.log(chalk.bold('Commands:\n'));
console.log(' kdm show runners\n kdm health all\n kdm watch\n kdm logs <name>\n');
program.outputHelp();
- process.exit(hadError ? 1 : 0);
+ process.exit(0);
} catch (error) {
console.error(chalk.red('Error:'), (error as Error).message);
process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let hadError = false; | |
| showWelcomeBanner(currentVersion); | |
| const spinner = createSpinner('Checking connections...').start(); | |
| const [dockerStatus, k8sStatus, minikubeStatus] = await Promise.all([ | |
| checkDockerConnection(), | |
| checkK8sConnection(), | |
| checkMinikubeConnection(), | |
| ]); | |
| spinner.stop('Connection check complete'); | |
| console.log(); | |
| const badge = (text: string, color: 'green' | 'red' | 'yellow') => { | |
| const styles = { | |
| green: chalk.bgGreen.black.bold, | |
| red: chalk.bgRed.white.bold, | |
| yellow: chalk.bgYellow.black.bold, | |
| }; | |
| return styles[color](` ${text} `); | |
| }; | |
| const dockerStr = dockerStatus.connected ? badge('CONNECTED', 'green') : badge('DISCONNECTED', 'red'); | |
| const k8sStr = k8sStatus.connected ? badge('CONNECTED', 'green') : badge('DISCONNECTED', 'red'); | |
| let minikubeStr = badge('NOT INSTALLED', 'red'); | |
| if (minikubeStatus.installed) { | |
| minikubeStr = minikubeStatus.running ? badge('RUNNING', 'green') : badge('STOPPED', 'yellow'); | |
| } | |
| console.log(`${chalk.bold('Docker:')} ${dockerStr}`); | |
| console.log(`${chalk.bold('Kubernetes:')} ${k8sStr}`); | |
| console.log(`${chalk.bold('Minikube:')} ${minikubeStr}\n`); | |
| console.log(`${chalk.cyan('')} Running Containers: ${chalk.yellow.bold(dockerStatus.containerCount)}`); | |
| console.log(`${chalk.blue('')} Running Pods: ${chalk.yellow.bold(k8sStatus.podCount)}`); | |
| console.log(`${chalk.red('')} Unhealthy Services: ${chalk.yellow.bold('0')} (Mocked)\n`); | |
| console.log(chalk.bold('Commands:\n')); | |
| console.log(' kdm show runners\n kdm health all\n kdm watch\n kdm logs <name>\n'); | |
| program.outputHelp(); | |
| process.exit(0); | |
| } catch (error) { | |
| console.error(chalk.red('Error:'), (error as Error).message); | |
| process.exit(1); | |
| } |
🤖 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/root.ts` at line 140, Remove the unused hadError flag and
simplify exit handling: delete the hadError declaration and replace the final
process.exit(hadError ? 1 : 0) with a fixed process.exit(0) (the catch block
already exits with code 1), so only the catch path returns 1 and the normal flow
returns 0; reference the hadError variable and the process.exit(...) call in
src/commands/root.ts to locate the changes.
Comment on lines
+84
to
+88
| const [{ getRunningContainers }, { getRunningPods }] = await Promise.all([ | ||
| import('../docker/containers'), | ||
| import('../kubernetes/pods'), | ||
| ]); | ||
|
|
Contributor
There was a problem hiding this comment.
Handle dynamic import failures in showRunners before fetch flow starts.
If either import rejects at Line 84, the function exits before your Promise.allSettled fallback logic runs, so users get no partial results and spinner state is not finalized.
Suggested fix
export const showRunners = async () => {
const spinner = createSpinner('Fetching runners (Containers + Pods)...').start();
- const [{ getRunningContainers }, { getRunningPods }] = await Promise.all([
- import('../docker/containers'),
- import('../kubernetes/pods'),
- ]);
+ const [containersMod, podsMod] = await Promise.allSettled([
+ import('../docker/containers'),
+ import('../kubernetes/pods'),
+ ]);
+
+ const containerPromise =
+ containersMod.status === 'fulfilled'
+ ? containersMod.value.getRunningContainers()
+ : Promise.reject(containersMod.reason);
+ const podPromise =
+ podsMod.status === 'fulfilled'
+ ? podsMod.value.getRunningPods()
+ : Promise.reject(podsMod.reason);
- const [containerRes, podRes] = await Promise.allSettled([
- getRunningContainers(),
- getRunningPods()
- ]);
+ const [containerRes, podRes] = await Promise.allSettled([containerPromise, podPromise]);🤖 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/show.ts` around lines 84 - 88, The dynamic imports for
getRunningContainers and getRunningPods in showRunners use Promise.all so a
rejection aborts before your Promise.allSettled fallback runs; change the import
step to use Promise.allSettled and inspect each result, assigning the real
functions when status==='fulfilled' and otherwise assigning a safe fallback
(e.g., a no-op that returns [] or a synchronous require fallback) for
getRunningContainers and getRunningPods, and ensure the spinner/cleanup logic
still runs in all paths so the spinner is finalized even when an import fails.
Comment on lines
+105
to
+108
| const npmVersion = execSync('npm --version') | ||
| .toString() | ||
| .trim(); | ||
| console.log(` NPM: ${npmVersion}\n`); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add error handling for npm --version command.
The execSync('npm --version') call on line 105 can throw if npm is not found in PATH or if the command fails. Wrap this in a try-catch block to gracefully handle the error.
🛡️ Proposed fix
// Get npm info
- const npmVersion = execSync('npm --version')
- .toString()
- .trim();
- console.log(` NPM: ${npmVersion}\n`);
+ try {
+ const npmVersion = execSync('npm --version')
+ .toString()
+ .trim();
+ console.log(` NPM: ${npmVersion}`);
+ } catch {
+ console.log(` NPM: ${chalk.dim('not available')}`);
+ }
+ console.log();
} catch (error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const npmVersion = execSync('npm --version') | |
| .toString() | |
| .trim(); | |
| console.log(` NPM: ${npmVersion}\n`); | |
| try { | |
| const npmVersion = execSync('npm --version') | |
| .toString() | |
| .trim(); | |
| console.log(` NPM: ${npmVersion}`); | |
| } catch { | |
| console.log(` NPM: ${chalk.dim('not available')}`); | |
| } | |
| console.log(); |
🤖 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/update.ts` around lines 105 - 108, Wrap the execSync('npm
--version') call in a try-catch around the npmVersion assignment (the const
npmVersion = execSync(...).toString().trim() and subsequent console.log) so
failures when npm is missing or the command errors are handled gracefully; on
error set npmVersion to a safe fallback string (e.g. 'not found' or 'unknown')
and log that fallback with console.log, and optionally include the caught error
in a debug-level log or message for diagnostics.
Comment on lines
+1
to
+17
| cat > test-version.js << 'EOF' | ||
| import { readFileSync } from 'fs'; | ||
| import { join, dirname } from 'path'; | ||
| import { fileURLToPath } from 'url'; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = dirname(__filename); | ||
|
|
||
| try { | ||
| const packagePath = join(__dirname, 'package.json'); | ||
| const pkg = JSON.parse(readFileSync(packagePath, 'utf-8')); | ||
| console.log('Version:', pkg.version); | ||
| } catch (err) { | ||
| console.error('Error:', err.message); | ||
| } | ||
| EOF | ||
| node test-version.js No newline at end of file |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the JS file no longer contains embedded shell heredoc/run commands.
nl -ba test-version.js | sed -n '1,40p'
rg -n "^(cat > .*<< 'EOF'|EOF$|node test-version\.js$)" test-version.js
# Expected after fix: rg returns no matches.Repository: KDM-cli/kdm-cli
Length of output: 182
Remove shell heredoc wrapper from test-version.js—file contains invalid JavaScript.
The file has cat > test-version.js << 'EOF' at line 1, EOF at line 16, and node test-version.js at line 17. These are shell syntax, not JavaScript, so the file will fail to parse as a Node module.
Proposed fix
-cat > test-version.js << 'EOF'
import { readFileSync } from 'fs';
import { join, dirname } from 'path';
import { fileURLToPath } from 'url';
@@
} catch (err) {
console.error('Error:', err.message);
}
-EOF
-node test-version.js🧰 Tools
🪛 Biome (2.4.15)
[error] 17-17: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
🤖 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 `@test-version.js` around lines 1 - 17, The file contains a shell heredoc
wrapper and a trailing shell command that make the JS invalid; remove the
leading "cat > test-version.js << 'EOF'" line, the closing "EOF" line, and the
final "node test-version.js" line so the file only contains the JavaScript
module code that defines __filename/__dirname, computes packagePath, reads and
parses package.json into pkg, and logs pkg.version; keep the existing try/catch
and identifiers (__filename, __dirname, packagePath, pkg) intact.
Member
|
@Rishiraj-Pathak-27 please check the coments of coderabbit for better verificaion |
Contributor
Author
|
Sure, I will do that and let you know at the earliest |
github-actions
Bot
added
documentation
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.
feat: Implement Auto Version Checking & Update Prompt (#26)
This PR resolves Issue #26 by introducing an automated version checking mechanism along with a dedicated update command. The goal is to ensure users are notified whenever a newer CLI version is available and can easily upgrade to the latest release.
Key Changes
Version Checking Logic
versionChecker.tswith semantic version comparison support.Update Command (
kdm update)update.tscommand to manually check for updates and fetch the latest release information.Root Command Integration
root.tsto integrate automatic version checking during CLI startup.Testing
update.test.ts.Documentation
Updated
README.mdwith:Dependency Resolution
Resolved dependency conflicts and aligned required packages in
package.json, including:inkreactreact-devtools-corenodemailerVerification
Tested locally with:
node kdm.js --versionnode kdm.js update checkSummary by CodeRabbit
Release Notes
New Features
Documentation
Improvements