fix: wrap subchart values on save/unwrap on read for correct Helm deployment

[HahaSula]

Summary

• deployments.js POST now wraps values under the Chart.yaml dependency name before writing to disk, so any Helm invocation (ArgoCD, Flux, direct helm install) receives correct subchart values
• deployments.js GET now unwraps the subchart key before returning to the frontend, preserving the bare-key contract with the table editor UI
• render.js removes the wrapValuesForSubchart() workaround and runs helm template directly on the deployment directory, making render behavior consistent with any Helm invocation
• Adds integration tests covering wrap/unwrap round-trip

Root Cause

The frontend intentionally uses bare keys for the table editor. The backend was supposed to act as the translation layer between bare keys and Helm subchart wrap format, but the wrap was never implemented on save. A temporary wrapValuesForSubchart() workaround in render.js masked the issue — preview looked correct while any direct Helm deployment silently used default values.

This is a Helm subchart values issue, not specific to ArgoCD. The same problem would affect Flux, helm install, or any other tool that consumes values.yaml directly.

Test plan

• tests/integration/deployments-api.test.js — 5 new tests covering POST wrap, GET unwrap, backward compatibility, and round-trip consistency
• All previously passing tests continue to pass
• Manual: verify preview render output is consistent before and after

Closes #19

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Refactor

  • Standardized how deployment configuration values are transformed and returned, with automatic unwrapping on reads and consistent wrapping on writes.
  • Improved alert counting so it correctly reflects both dependency-wrapped and legacy bare values.yaml formats.
  • Updated folder initialization and tree/list responses to use the new consistent value structure.

• Tests

  • Expanded integration coverage for Deployments API and folder initialization/tree behavior, including backward compatibility.
  • Added unit tests for the value wrapping/unwrapping and alert counting utilities.
  • Strengthened chart rendering tests using temporary workspace setup.

[rophy]

PR Preview Bot

Preview environment torn down.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 77fa72af-1a8c-48b1-8fef-7f8bb045cfc7

📥 Commits

Reviewing files that changed from the base of the PR and between 5d1551a and 097fc34.

📒 Files selected for processing (1)

• tests/unit/chartRendering.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/unit/chartRendering.test.js

---

📝 Walkthrough

Walkthrough

A new server/lib/subchart.js library provides helpers to read the subchart dependency name from Chart.yaml and conditionally wrap/unwrap values under that key. The deployments API integrates these helpers: GET unwraps stored values before returning them to the frontend (bare-key format), POST wraps submitted values before writing to disk (Helm-compatible format). The folders API uses the same helpers for alert counting and deployment initialization. The render route's temporary-file wrapping mechanism is removed. Sample deployment values.yaml files are migrated to the wrapped format. Comprehensive integration and unit tests validate wrap/unwrap behavior and backward compatibility.

Changes

Subchart Value Wrapping in Deployments API

Layer / File(s)	Summary
Subchart helper library server/lib/subchart.js	Introduces getDepName(dir) to read Chart.yaml and extract the first dependency's name, wrapValues(values, depName) to nest bare values under the subchart key, unwrapValues(parsed, depName) to extract nested values, and countAlerts(parsed, depName) to count alert arrays after unwrapping.
Deployments API GET/POST with wrap/unwrap server/routes/deployments.js	Imports helpers and applies them throughout. GET /:chart uses countAlerts for alert counting; GET /:chart/:deployment unwraps stored values via unwrapValues; POST /:chart/:deployment wraps non-string values via wrapValues before writing.
Folders API alert counting and value wrapping server/routes/folders.js	Imports wrapValues and countAlerts. Deployment alert counting now uses countAlerts instead of manual iteration. Deployment initialization wraps default values under the dependency name before writing values.yaml.
Sample deployment values wrapped under subchart name sample/deployments/mariadb-*/*/values.yaml	Migrates three sample values.yaml files to nest alert rules under the subchart dependency key (mariadb-alerts). All threshold values and list contents are preserved; only the YAML key nesting changes.
Render route removes temporary-file wrapping mechanism server/routes/render.js	Removes temporary wrapped-values-file creation, deletes fs/promises and YAML imports, eliminates finally cleanup block. The ?folder template case now directly uses deploymentsDir without conditional file wrapping.
Unit tests for subchart helpers tests/unit/subchart.test.js	Vitest test cases verify getDepName extracts the first dependency name or returns null, wrapValues/unwrapValues round-trip correctly and handle legacy bare keys, countAlerts sums array lengths after unwrapping and ignores non-array keys.
Integration tests for deployments API wrap/unwrap behavior tests/integration/deployments-api.test.js	Vitest/Supertest suite verifies POST wrapping under the dependency name, GET unwrapping to bare keys, backward compatibility when no dependencies exist, POST→GET round-trip consistency, and list endpoint alertCount for both wrapped and legacy formats.
Updated integration tests for folders and chart rendering tests/integration/folders-api.test.js, tests/integration/folders-tree.test.js, tests/unit/chartRendering.test.js	Folders API tests assert scaffolded values are wrapped under the dependency name. Folders tree test includes backward-compatibility check for legacy bare keys. Chart rendering test switches to execFileSync, copies sample/ to temp directory, and runs helm dependency build before templating.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 From bare keys the frontend sends its plea,
The backend wraps them for Helm's decree!
POST nests them tight, GET unwraps with care,
Round-trip consistent, no temp files to spare.
Chart.yaml speaks, dependency names align,
This rabbit's solved the wrap—a design so fine! 🌿

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR includes one critical out-of-scope omission: issue #19 explicitly requires a one-time migration of three existing values.yaml files to wrapped format, but this PR does not include those changes, leaving existing deployments broken.	Apply the one-time migration to wrap values.yaml in mariadb-1/production, mariadb-1/staging, and mariadb-2/production as required by issue #19. The sample files show the wrapped format but the actual gitops/deployments/ files must be updated.
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: wrapping subchart values on save and unwrapping on read to fix Helm deployment.
Linked Issues check	✅ Passed	The PR implements all coding requirements from issue #19: wrap/unwrap logic in deployments.js, render.js cleanup, helper utilities in subchart.js, and comprehensive test coverage for wrap/unwrap behavior.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/subchart-values-wrap

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[rophy]

@HahaSula Same as the comment on #19 — this is a Helm subchart values issue, not ArgoCD-specific. Could you update the PR title and body to remove ArgoCD references? e.g. title: "fix: wrap subchart values on save/unwrap on read for correct Helm deployment"

[rophy]

alertCount in the list endpoint (deployments.js:40-44) and folder tree (folders.js:48-55) will always be 0 for wrapped values — both loops look for top-level arrays, but after wrapping the only top-level key is the dep name (an object).

Please fix the counting to unwrap first, and add a test that writes a wrapped values.yaml and asserts the correct alertCount.

[rophy]

Bug: No migration for existing bare values.yaml files

The 3 existing values.yaml files under gitops/deployments/ are in bare format. After this PR, render.js no longer wraps at template time, so rendering any pre-existing deployment will silently produce wrong output until someone re-saves through the UI. The issue itself called for a one-time migration — this PR does not include it.

[rophy]

Bug: Existing tests mask the alertCount regression

folders-tree.test.js:74, deployments-crud.test.js:159, and api.test.js:182 all write bare-key fixtures to values.yaml. They pass because the old counting logic still works on bare format — but real files written by the new POST are wrapped, which yields alertCount: 0. These tests give false confidence. Please update fixtures to use the wrapped format, or add new tests that do.

[rophy]

Cleanup: Extract shared getDepName() helper

The "read Chart.yaml, return first dependency name" logic now exists in deployments.js (new getDepName) and folders.js:33+143. Previously it was also in render.js. Consider extracting to a shared utility (e.g. server/lib/subchart.js) to avoid drift between wrap/unwrap/count paths.

[HahaSula]

While testing the new-deployment flow — clicking Create in the "New Deployment" dialog (the + next to the Deployments list, which calls POST /folders/init) — I noticed a new deployment is pre-filled with the chart's full default values.yaml: all 13 alert rules, each with namespace: default, an empty owner, and the default thresholds. My subchart-wrap fix preserved this behavior; it only changed the on-disk format (now wrapped under the dependency name), not which entries get copied.

I'm wondering whether new deployments should start empty instead. As it stands, a user who creates a deployment and deploys it without editing ships 13 alerts targeting the default namespace with no owner.

One subtlety I confirmed while testing: simply writing an empty values.yaml (or mariadb-alerts: {}) does not produce an empty render — Helm falls back to the subchart's own default values and still renders all 13 rules. A truly empty start would require init to explicitly write each alert key as [] to override the subchart defaults.

Before I touch this: is the pre-filled-defaults behavior intentional (e.g. a discoverable template the user edits down), or would you prefer empty deployments? Happy to implement the empty-start variant — it's the init logic plus a one-line test change.

cc @rophy

[coderabbitai]

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 `@tests/unit/chartRendering.test.js`:
- Around line 141-142: Replace the two `execSync` calls with `execFileSync` to
avoid shell parsing of the Helm commands. For the first call executing helm
dependency build, change from using execSync with a template literal string to
execFileSync with the command 'helm' and an array argument containing
['dependency', 'build', deployDir]. For the second call executing helm template,
similarly change to execFileSync with command 'helm' and array argument
containing ['template', 'prod-release', deployDir]. This prevents shell
expansion and makes the paths safer when they contain special characters.

🪄 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: 67bb10fd-4dc5-42af-a142-3e84ed6e0deb

📥 Commits

Reviewing files that changed from the base of the PR and between 669bc7b and 5d1551a.

📒 Files selected for processing (11)

• sample/deployments/mariadb-1/production/values.yaml
• sample/deployments/mariadb-1/staging/values.yaml
• sample/deployments/mariadb-2/production/values.yaml
• server/lib/subchart.js
• server/routes/deployments.js
• server/routes/folders.js
• tests/integration/deployments-api.test.js
• tests/integration/folders-api.test.js
• tests/integration/folders-tree.test.js
• tests/unit/chartRendering.test.js
• tests/unit/subchart.test.js

✅ Files skipped from review due to trivial changes (1)

• sample/deployments/mariadb-2/production/values.yaml

🚧 Files skipped from review as they are similar to previous changes (1)

• server/routes/deployments.js

[rophy]

LGTM. All prior review comments addressed — alertCount unwrapping, deployment creation wrapping, and execFileSync migration all landed in follow-up commits. Good test coverage across unit and integration layers.

[rophy]

Good catch — let's start empty. Tracked as #23, no need to address in this PR.