Improve config file change handling#135
Conversation
…rowsnest.conf - simplify the logic of writing files under ratos_generated: resetting these files is already handled by macros specifically created for that purpose, don't try to provide a reset story through the setup wizard.
…white space between neighbouring replaced sections
…ailing inline comments for retained keys.
…ailing decoration is preserved
…1]` section - don't set the `[crowsnest]` section as this too easily stomps on user changes, and the `[crowsnest]` section is not strictly a VAOC-specific concern anyhow
…amera - use /dev/RatOS/rr-vaoc-camera instead of /dev/video0. This aims to pick the right camera even if the user has an additional camera installed.
…also restart crowsnest before restarting klipper
📝 WalkthroughWalkthroughThis PR enables scoped service restarts during configuration updates. It refactors ChangesScoped Service Restart and Configuration Updates
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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.
🧹 Nitpick comments (6)
src/server/helpers/klipper.ts (1)
119-134: ⚡ Quick winConsider validating the Moonraker service restart response.
The
serviceRestartfunction currently treats any successfulfetchas success, but doesn't verify that Moonraker accepted the restart request or that the service actually exists. If Moonraker returns an error response (e.g., HTTP 4xx/5xx or a JSON error payload), this function will still returntrue.This could lead to silent failures where the configurator believes a service restarted successfully, but Moonraker rejected the request.
🔍 Proposed validation enhancement
export const serviceRestart = async (service: PermittedServices) => { getLogger().info(`Attempting to restart service ${service} via Moonraker...`); try { - await fetch('http://localhost:7125/machine/services/restart', { + const response = await fetch('http://localhost:7125/machine/services/restart', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ service }), }); + if (!response.ok) { + getLogger().error(`Moonraker returned error status ${response.status} for service ${service}`); + return false; + } getLogger().info(`${service} restart command sent successfully.`); return true; } catch (e) { getLogger().error(`Failed to send ${service} restart command: ${getErrorMessage(e)}`); } return false; };🤖 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/server/helpers/klipper.ts` around lines 119 - 134, The serviceRestart function currently treats any successful fetch as success; update serviceRestart to validate the Moonraker HTTP response: await the fetch response, check response.ok and if not ok log the status and response text (using getLogger().error and getErrorMessage for exceptions) and return false; if response.ok parse JSON and detect an error payload (e.g., presence of an "error" field or missing success indicator) and log details and return false; only return true when response.ok and the parsed JSON indicates the restart was accepted. Use the existing symbols serviceRestart, PermittedServices, getLogger, getErrorMessage and the same endpoint URL to locate the code to change.src/templates/toolhead-alignment-systems/rat-rig-vaoc.ts (1)
63-80: ⚡ Quick winWrap variable declarations in blocks to prevent scope leakage.
Biome correctly flags that variable declarations in switch cases can be accessed by other cases. While this currently works because only one case is executed, wrapping the declarations in blocks is a defensive best practice that prevents future refactoring errors.
🛡️ Proposed fix to scope variables correctly
case 'config-helpers': + { // If overwrite is requested (or overwrite details are not available), reset dc-endstop.cfg. // Otherwise, use the existing content or the default content if the file does not exist. const dcEndstopCfgFileName = 'ratos_generated/dc-endstop.cfg'; const dcEndstopContent = getDcEndstopConfigurationFileContent(dcEndstopCfgFileName); ctx.extrasGenerator.addFileToRender({ fileName: dcEndstopCfgFileName, content: dcEndstopContent, overwrite: true, }); // Ditto, for adjust-y-max.cfg. const adjustYMaxCfgFileName = 'ratos_generated/adjust-y-max.cfg'; const adjustYMaxContent = getAdjustYMaxConfigurationFileContent(adjustYMaxCfgFileName); ctx.extrasGenerator.addFileToRender({ fileName: adjustYMaxCfgFileName, content: adjustYMaxContent, overwrite: true, }); return ` ######################################## # Configuration Helpers for Rat Rig VAOC ######################################## # # These includes must come after any user-defined stepper sections # to ensure that overrides work correctly. # [include ratos_generated/dc-endstop.cfg] # Managed by CONFIGURE_DC_ENDSTOP macro [include ratos_generated/adjust-y-max.cfg] # Managed by INCREASE_Y_MAX macro `; + } default: throw new Error(`Unknown purpose '${ctx.section}' for template rendering.`);As per coding guidelines from static analysis tools.
🤖 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/templates/toolhead-alignment-systems/rat-rig-vaoc.ts` around lines 63 - 80, The variables dcEndstopCfgFileName, dcEndstopContent and adjustYMaxCfgFileName, adjustYMaxContent are declared directly in a switch case and can leak scope; wrap each case's related declarations and the calls to ctx.extrasGenerator.addFileToRender inside their own block { ... } so the names are block-scoped and cannot be accessed from other cases (i.e., enclose the dc-endstop.cfg declarations/call and the adjust-y-max.cfg declarations/call each in their own braces).src/server/routers/printer.ts (1)
1174-1179: Document the restart scoping design and plan for service config extensibility.The code only restarts
crowsnestwhencrowsnest.confchanges, which is correct for the current implementation where onlycrowsnest.confis generated. However, thePermittedServicestype includes other services (e.g.,KlipperScreen,sonar) that may require similar handling if their config files are added in the future.Consider adding a comment documenting why only
crowsnestis currently restarted, and note that this logic (which appears in multiple places) will need to be extended if other service-specific config files are introduced.🤖 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/server/routers/printer.ts` around lines 1174 - 1179, Add an explanatory comment above the servicesToRestart computation describing that only 'crowsnest' is restarted because currently only 'crowsnest.conf' is generated, and that PermittedServices includes other services (e.g., KlipperScreen, sonar) which will require extending this conditional when their config files are introduced; reference the servicesToRestart variable, the PermittedServices type, the fileName check for 'crowsnest.conf', and the klipperRestart call so future maintainers know to update all similar occurrences when adding service-specific config handling.src/server/helpers/file-operations.ts (3)
333-357: 💤 Low valueHoist
preserveTrailingInlineCommentsout of the section loop.
preserveTrailingInlineCommentsis redeclared on every section that needs replacement (it's inside thefor (let i = 0; i < sections.length; i++)body via theelsebranch at line 332). Hoisting it to module/file scope (or just outside the loop) avoids the repeated closure allocation and makes the helper independently testable. Behavior would be unchanged.🤖 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/server/helpers/file-operations.ts` around lines 333 - 357, The helper function preserveTrailingInlineComments is currently declared inside the section replacement loop causing repeated allocations; move its declaration out of the for (let i = 0; i < sections.length; i++) loop to module/file scope or at least immediately above the loop so it is created once and can be unit-tested independently; ensure you preserve its signature (originalBody: string, updatedBody: string) and behavior, and if you need to test it export or attach it so tests can import it without changing its internal logic.
360-368: 💤 Low valueVerify intentional loss of new-body trailing whitespace.
newBodyWithPreservedComments.replace(/\n+$/, '')strips all trailing newlines from the caller-supplied body before re-applying the original section'strailingDecoration. The effect is that any trailing blank-line/comment structure provided by the caller inupdate.bodyis discarded in favor of whatever already existed at the end of the original section.This is fine for the documented goal (preserve formatting), but it means the caller cannot intentionally add or remove a trailing separator via
body. If that's by design, a one-line comment explaining the priority (original decoration wins over new body's trailing whitespace) would prevent future confusion. The existing tests (idempotent CRLF / preserves blank-line-between-sections) cover the happy paths, but not the "caller adds extra trailing blank lines" case.🤖 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/server/helpers/file-operations.ts` around lines 360 - 368, The code intentionally strips trailing newlines from the caller-provided body (newBodyWithPreservedComments.replace(/\n+$/, '')) and re-applies the original section's trailingDecoration (derived from originalBodyWithTrailing), which causes any caller-added trailing blank lines/comments in update.body to be ignored; add a concise one-line comment above this block (near newBodyWithPreservedComments, trailingDecoration and originalBodyWithTrailing) stating that the original section's trailing decoration takes precedence over caller-supplied trailing whitespace (i.e., intentional behavior) so future readers/tests understand that caller cannot override the section trailing separator.
334-343: 💤 Low valueFirst-occurrence-wins for duplicate keys in original body — confirm this is intentional.
commentByKeyonly records the first inline comment seen for any given key (line 340:if (!commentByKey.has(key))). If the original section legitimately contains the same key on multiple lines (e.g., repeated config entries) with different trailing comments, only the first comment is preserved when remapping to the new body. For typical Klipper/INI sections this is unlikely to matter, but worth a confirmation given the new behavior aims to preserve user-authored comments faithfully.If duplicate keys are expected to be rare or invalid, a brief inline comment documenting this assumption would help future maintainers.
🤖 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/server/helpers/file-operations.ts` around lines 334 - 343, The code that builds commentByKey from originalBody currently ignores later inline comments for duplicate keys (see commentByKey, originalBody, and the if (!commentByKey.has(key)) guard) — either make this behavior explicit or change it: if you want first-occurrence-wins, add a concise inline comment above the guard explaining the assumption that duplicate keys are invalid/rare and first comment is authoritative; if you want last-occurrence-wins (to preserve the most recent inline comment), remove the if-check so commentByKey.set(key, commentSuffix) always overwrites previous entries; update tests if necessary to reflect the chosen 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.
Nitpick comments:
In `@src/server/helpers/file-operations.ts`:
- Around line 333-357: The helper function preserveTrailingInlineComments is
currently declared inside the section replacement loop causing repeated
allocations; move its declaration out of the for (let i = 0; i <
sections.length; i++) loop to module/file scope or at least immediately above
the loop so it is created once and can be unit-tested independently; ensure you
preserve its signature (originalBody: string, updatedBody: string) and behavior,
and if you need to test it export or attach it so tests can import it without
changing its internal logic.
- Around line 360-368: The code intentionally strips trailing newlines from the
caller-provided body (newBodyWithPreservedComments.replace(/\n+$/, '')) and
re-applies the original section's trailingDecoration (derived from
originalBodyWithTrailing), which causes any caller-added trailing blank
lines/comments in update.body to be ignored; add a concise one-line comment
above this block (near newBodyWithPreservedComments, trailingDecoration and
originalBodyWithTrailing) stating that the original section's trailing
decoration takes precedence over caller-supplied trailing whitespace (i.e.,
intentional behavior) so future readers/tests understand that caller cannot
override the section trailing separator.
- Around line 334-343: The code that builds commentByKey from originalBody
currently ignores later inline comments for duplicate keys (see commentByKey,
originalBody, and the if (!commentByKey.has(key)) guard) — either make this
behavior explicit or change it: if you want first-occurrence-wins, add a concise
inline comment above the guard explaining the assumption that duplicate keys are
invalid/rare and first comment is authoritative; if you want
last-occurrence-wins (to preserve the most recent inline comment), remove the
if-check so commentByKey.set(key, commentSuffix) always overwrites previous
entries; update tests if necessary to reflect the chosen behavior.
In `@src/server/helpers/klipper.ts`:
- Around line 119-134: The serviceRestart function currently treats any
successful fetch as success; update serviceRestart to validate the Moonraker
HTTP response: await the fetch response, check response.ok and if not ok log the
status and response text (using getLogger().error and getErrorMessage for
exceptions) and return false; if response.ok parse JSON and detect an error
payload (e.g., presence of an "error" field or missing success indicator) and
log details and return false; only return true when response.ok and the parsed
JSON indicates the restart was accepted. Use the existing symbols
serviceRestart, PermittedServices, getLogger, getErrorMessage and the same
endpoint URL to locate the code to change.
In `@src/server/routers/printer.ts`:
- Around line 1174-1179: Add an explanatory comment above the servicesToRestart
computation describing that only 'crowsnest' is restarted because currently only
'crowsnest.conf' is generated, and that PermittedServices includes other
services (e.g., KlipperScreen, sonar) which will require extending this
conditional when their config files are introduced; reference the
servicesToRestart variable, the PermittedServices type, the fileName check for
'crowsnest.conf', and the klipperRestart call so future maintainers know to
update all similar occurrences when adding service-specific config handling.
In `@src/templates/toolhead-alignment-systems/rat-rig-vaoc.ts`:
- Around line 63-80: The variables dcEndstopCfgFileName, dcEndstopContent and
adjustYMaxCfgFileName, adjustYMaxContent are declared directly in a switch case
and can leak scope; wrap each case's related declarations and the calls to
ctx.extrasGenerator.addFileToRender inside their own block { ... } so the names
are block-scoped and cannot be accessed from other cases (i.e., enclose the
dc-endstop.cfg declarations/call and the adjust-y-max.cfg declarations/call each
in their own braces).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e122e982-8a09-48f7-b8e2-25003f877912
📒 Files selected for processing (6)
src/__tests__/server.test.tssrc/instrumentation.tssrc/server/helpers/file-operations.tssrc/server/helpers/klipper.tssrc/server/routers/printer.tssrc/templates/toolhead-alignment-systems/rat-rig-vaoc.ts
1 participant
Release Notes
New Features
crowsnest.confis changed.Bug Fixes
/dev/RatOS/rr-vaoc-camera, improving configuration robustness.