fix(dashmate): restart for rs-dapi and envoy #2821
Conversation
WalkthroughBoth Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/dashmate/src/listr/tasks/stopNodeTaskFactory.js (1)
19-28: Refactor to avoid mutating aconstarray.While mutating a
constarray is valid JavaScript, it violates the Airbnb style guide convention (referenced in coding guidelines) which prefers signaling mutation intent withletor avoiding mutation entirely.Apply this diff to use a functional approach without mutation:
function getPlatformProfiles(config) { const platformProfiles = getConfigProfiles(config) .filter((profile) => profile.startsWith('platform')); - - if (platformProfiles.length === 0) { - platformProfiles.push('platform'); - } - - return Array.from(new Set(platformProfiles)); + + const result = platformProfiles.length === 0 ? ['platform'] : platformProfiles; + + return Array.from(new Set(result)); }Or more concisely:
function getPlatformProfiles(config) { const platformProfiles = getConfigProfiles(config) .filter((profile) => profile.startsWith('platform')); - - if (platformProfiles.length === 0) { - platformProfiles.push('platform'); - } - - return Array.from(new Set(platformProfiles)); + + return Array.from(new Set( + platformProfiles.length > 0 ? platformProfiles : ['platform'] + )); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dashmate/src/listr/tasks/startNodeTaskFactory.js(4 hunks)packages/dashmate/src/listr/tasks/stopNodeTaskFactory.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/**/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/**/**/*.{js,ts,jsx,tsx}: Adhere to ESLint with Airbnb/TypeScript configs for JS/TS code
Use camelCase for JS/TS variables and functions
Use PascalCase for JS/TS classes
Prefer kebab-case filenames within JS packages
Files:
packages/dashmate/src/listr/tasks/startNodeTaskFactory.jspackages/dashmate/src/listr/tasks/stopNodeTaskFactory.js
🔇 Additional comments (6)
packages/dashmate/src/listr/tasks/startNodeTaskFactory.js (3)
22-22: LGTM!The
getConfigProfilesdependency has been correctly added to both the JSDoc and function signature, maintaining consistency.Also applies to: 34-34
105-105: LGTM!The use of
getPlatformProfiles(config)correctly centralizes platform profile derivation and aligns with the PR objective to enhance restart functionality for platform services.
160-160: LGTM!Consistent with the profile derivation logic at line 105. The centralized approach ensures both service checking and startup use the same platform profile resolution.
packages/dashmate/src/listr/tasks/stopNodeTaskFactory.js (3)
10-10: LGTM: Dependency injection looks correct.The
getConfigProfilesparameter is properly documented and added to the function signature, following camelCase conventions.Also applies to: 17-17
43-43: LGTM: Profile derivation logic is correct.The conditional logic properly uses
getPlatformProfiles(config)when platform-only mode is enabled, falling back to an empty array otherwise.
83-84: LGTM: Consistent profile handling.The profile derivation here mirrors the logic in the "Check node is running" task, ensuring consistent behavior across both stop operations.
| function getPlatformProfiles(config) { | ||
| const platformProfiles = getConfigProfiles(config) | ||
| .filter((profile) => profile.startsWith('platform')); | ||
|
|
||
| if (platformProfiles.length === 0) { | ||
| platformProfiles.push('platform'); | ||
| } | ||
|
|
||
| return Array.from(new Set(platformProfiles)); | ||
| } |
There was a problem hiding this comment.
Fix const mutation error.
platformProfiles is declared as const but is mutated on line 41 with .push(). This will cause a runtime error when no platform profiles exist.
Apply this diff to fix the issue:
- function getPlatformProfiles(config) {
- const platformProfiles = getConfigProfiles(config)
- .filter((profile) => profile.startsWith('platform'));
+ function getPlatformProfiles(config) {
+ let platformProfiles = getConfigProfiles(config)
+ .filter((profile) => profile.startsWith('platform'));📝 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.
| function getPlatformProfiles(config) { | |
| const platformProfiles = getConfigProfiles(config) | |
| .filter((profile) => profile.startsWith('platform')); | |
| if (platformProfiles.length === 0) { | |
| platformProfiles.push('platform'); | |
| } | |
| return Array.from(new Set(platformProfiles)); | |
| } | |
| function getPlatformProfiles(config) { | |
| let platformProfiles = getConfigProfiles(config) | |
| .filter((profile) => profile.startsWith('platform')); | |
| if (platformProfiles.length === 0) { | |
| platformProfiles.push('platform'); | |
| } | |
| return Array.from(new Set(platformProfiles)); | |
| } |
🤖 Prompt for AI Agents
In packages/dashmate/src/listr/tasks/startNodeTaskFactory.js around lines 36 to
45, platformProfiles is declared as const but mutated with .push(), causing an
error; change the declaration to let (or create a new array when adding the
default) so it can be mutated — e.g., declare let platformProfiles =
getConfigProfiles(config).filter(...); then if length is 0 push 'platform', and
return Array.from(new Set(platformProfiles)).
QuantumExplorer
changed the title
Enhance the dashmate restart functionality to ensure that both rs-dapi and envoy services restart correctly when using the
--platformoption. This includes improvements to profile handling in the start and stop node tasks.Summary by CodeRabbit