feat: deployment sync — follow a source deployment with eager propagation - #44
Merged
Conversation
Pure registry CRUD for the deployment-sync feature (#39): read/write sync.yaml, query source/target relationships, and mutate the registry for sync/unlink operations. Enforces the flat-tree role-exclusivity rule (a deployment can't be both a source and a target) and rejects unsafe paths (traversal, absolute, or rooted at the charts dir). Part of #39, stage 1/8 (see issue for full plan).
GET/POST/DELETE /api/v2/sync per the contract in #39: full-registry query, per-source/per-target lookup, create-or-switch a sync, and unlink. POST validates both paths (traversal/absolute/charts-dir rejection, isDeployment check) and copies source content into the target, creating it if it doesn't exist yet. Part of #39, stage 2/8.
POST /:chart/:deployment now propagates saved content to every registered target when the deployment is itself a sync source (folder-mode only, matched by the existing `folder` query param). Each target write is best-effort — one failing target doesn't undo the source's own successful save. DELETE /:chart/:deployment auto-removes the deployment from the sync registry first if it's currently a target, so deleting it never leaves a dangling registry entry. Part of #39, stage 3/8.
GET /api/v2/folders/deployments walks the whole gitops tree (skipping CHARTS_DIR) and returns every recognized deployment as a flat list, for the Sync to/from modals' candidate pickers. Deliberately uncached and separate from the lazy-loaded folder tree — it only runs when a sync modal opens, not on every navigation, so a fresh scan each time is simpler than maintaining an index. Also adds the frontend chartApi.js client for both this endpoint and the sync registry API from stage 2 (getSyncRegistry, getSyncTargets, getSyncSource, createSync, unlinkSync, listAllDeployments). Part of #39, stage 4/8.
The DELETE route only ever removed a legacy-named sibling file (<deployment>-values.yaml), which doesn't exist for folder-mode deployments (Chart.yaml + values.yaml living directly in the folder) — so deleting one silently left it fully intact on disk. Now it checks for a direct values.yaml and, if present, removes the whole directory instead. Found while wiring up a delete action for the deployment-sync feature (#39) — needed a working delete before DeleteSourceModal could make sense. Pre-existing bug, not introduced by sync.
…tTree DeploymentTree — the real active sidebar (DeploymentSelector.jsx turned out to be unused dead code) — now fetches the sync registry alongside the folder tree and shows a source/synced badge on every deployment node. Right-click menu is gated by role per Issue #39's flat-tree rule: targets get Sync from.../Unlink sync (no Sync to, they can't also be a source); sources get Sync to... (no Sync from, they can't also be a target); independent nodes get both. New modals: - SyncToModal: multi-select targets with the 🟢/🔴/🟠/⚫ status system, ack checkboxes gate Confirm, best-effort per-target submission - SyncFromModal: single source picker with the mirrored ⚫ condition (disabled if the candidate is itself already a target) - DeleteSourceModal: per-target Keep/Delete, composed from the existing DELETE /:chart/:deployment and new DELETE /sync calls refreshAll() deliberately isn't memoized with useCallback — an earlier version froze expandedKeys in a stale closure, so handlers fired after the tree had been expanded would refresh against the mount-time (empty) expansion set. Part of #39, stage 5/8.
Selecting a deployment now checks getSyncSource() and, if it's a synced target, disables every input across both Single and Overview modes (AlertTable gets a readOnly prop; the Common Values fields are disabled directly) and shows a "Synced from X" banner. Save stays disabled the same way it already was — readOnly inputs mean `dirty` can never become true, so no special-case Save logic was needed. DeploymentTree now takes an onSyncChange callback, fired after any sync mutation, so the banner updates immediately if the currently open deployment's sync state changes from the sidebar rather than only refreshing on reselect. Preview already worked correctly against frozen deployments without changes: it only calls handleSave() first when dirty, which frozen inputs never trigger. Part of #39, stage 6/8.
…tests Tree node titles embed handlers (Dropdown onClick -> handleUnlink -> refreshAll) that get frozen at whichever render first built that specific node — typically much earlier than when it's actually clicked. refreshAll was reading expandedKeys from that frozen closure, so after further navigation it would refresh using a stale (often empty) expansion set and silently wipe out already-loaded children. Fixed by reading from expandedKeysRef.current instead, which every closure shares as the same ref object regardless of which render created it. Found via a genuinely flaky E2E "Unlink sync" test — reproduced with a throwaway debug spec + console logging before landing on the ref fix (also caught along the way: the E2E server only picks up source changes after `npm run build`, since it serves the static dist/ bundle, not a live dev server). Adds tests/e2e/deployment-sync.spec.js covering: Sync to... via the UI creating a target and showing the synced badge, frozen read-only state with the "Synced from" banner, Unlink sync restoring editability, and eager sync propagation from an edited-and-saved source to its target. Part of #39, stage 7/8.
refreshAll and expandPath were awaiting getSyncRegistry() and each loadChildren() call sequentially, adding enough latency to flake 3 pre-existing E2E tests (nested-deployment.spec.js) that click through the tree shortly after page load without a robustness buffer. Running them concurrently via Promise.all brings initial load time back down and fixes the flakiness — confirmed via a full Playwright run (52/52 passing) after the change. Part of #39, stage 7/8.
Contributor
|
Preview environment failed to start. |
|
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:
📝 WalkthroughWalkthroughThis PR adds sync registry storage and APIs, sync-aware deployment save/delete behavior, deployment discovery, read-only UI handling for synced deployments, and test coverage across unit, integration, and end-to-end flows. ChangesDeployment Sync Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DeploymentTree
participant SyncToModal
participant SyncAPI
participant SyncRegistry
participant Filesystem
User->>DeploymentTree: choose "Sync to..."
DeploymentTree->>SyncToModal: open with source
User->>SyncToModal: select targets and confirm
SyncToModal->>SyncAPI: POST /api/v2/sync
SyncAPI->>SyncRegistry: applySync(source, target)
SyncAPI->>Filesystem: copy values.yaml / Chart.yaml
SyncAPI->>SyncRegistry: writeSyncRegistry()
SyncToModal->>DeploymentTree: onSuccess -> refreshAll
sequenceDiagram
participant User
participant AlertUserView
participant DeploymentsRoute
participant SyncRegistry
participant TargetDeployment
User->>AlertUserView: save source deployment values
AlertUserView->>DeploymentsRoute: POST /:chart/:deployment?folder=...
DeploymentsRoute->>SyncRegistry: getTargetsForSource(folder)
SyncRegistry-->>DeploymentsRoute: targets
loop each target
DeploymentsRoute->>TargetDeployment: write values.yaml
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
HahaSula
marked this pull request as ready for review
July 1, 2026 11:42
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/AlertUserView.jsx (1)
149-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
frozenSourceagainst stale async responses.Both
getSyncSource(...)call sites unconditionally write the resolved value into state. If the user changes deployments while an earlier request is still in flight, the late response can mark the wrong deployment read-only or editable. Please key this off the latestselectedFolder(for example via an effect/request token) before callingsetFrozenSource.Also applies to: 232-236
🤖 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/pages/AlertUserView.jsx` around lines 149 - 161, The handleFolderSelect flow in AlertUserView updates frozenSource from an async getSyncSource(path) response without checking whether the response is still current, so a late request can overwrite state for a newer selectedFolder. Add a staleness guard in the handleFolderSelect and other getSyncSource call site logic (for example a request token, cancellation flag, or selectedFolder comparison) before calling setFrozenSource, and only commit the resolved source when it matches the latest selection.
🧹 Nitpick comments (2)
tests/e2e/deployment-sync.spec.js (2)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrittle input selector could target the wrong field.
page.locator('input.ant-input:visible').first()selects whichever visible Ant input happens to be first in the DOM. If the "Common Values" section renders more than one visible input (or its layout changes), the test could silently assert against the wrong field.Also applies to: 131-135
🤖 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 `@tests/e2e/deployment-sync.spec.js` around lines 89 - 93, The test is using a brittle first-match locator for an Ant input, which can target the wrong field when multiple visible inputs exist. Update the deployment sync e2e assertions to scope the input lookup to the “Common Values” section or another stable label/role-based target instead of page.locator('input.ant-input:visible').first(), and apply the same locator change in both affected assertion blocks. Use the surrounding section heading and the existing Save button check to anchor the selector to the intended field.
39-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd teardown for the
e2e-sync-testfixtures.POST /api/v2/folders/initwon’t fail on an already-initialized folder, but this suite still leavesprod,staging, anddevbehind, so reruns can inherit stale sync/value state from a previous run.🤖 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 `@tests/e2e/deployment-sync.spec.js` around lines 39 - 43, Add teardown for the e2e-sync-test fixtures so reruns start clean instead of inheriting stale state. Update the deployment-sync suite by adding cleanup alongside the existing test.beforeAll/initDeployment setup for ROOT/prod, ROOT/staging, and ROOT/dev, and remove those initialized folders after the suite finishes. Use the existing initDeployment helper and the suite-level lifecycle hooks in deployment-sync.spec.js to locate and implement the cleanup.
🤖 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 `@server/lib/sync.js`:
- Around line 7-18: Serialize the sync registry updates in readSyncRegistry and
writeSyncRegistry so concurrent POST/DELETE requests for the same gitopsDir
cannot overwrite each other’s changes. Add a per-gitopsDir mutex or queued
update helper around the read-modify-write path that uses readSyncRegistry and
writeSyncRegistry, and make writeSyncRegistry persist via an atomic temp-file
rename for SYNC_FILE instead of writing directly.
- Around line 7-14: readSyncRegistry currently treats every read failure as an
empty registry, which hides malformed or unreadable sync.yaml cases. Update the
catch in readSyncRegistry to only return { syncs: [] } when the file is missing
(the missing-registry case), and rethrow or surface all other errors such as
YAML parse, permission, and I/O failures. Use the existing readSyncRegistry and
SYNC_FILE logic to distinguish ENOENT from other error types so we don’t
silently overwrite valid sync links.
- Around line 51-57: `isSafeSyncPath` currently validates raw strings, so
variants like `cpu/./prod`, `cpu//prod`, and `cpu/prod/` can evade the registry
checks. Canonicalize the candidate path first in `isSafeSyncPath` (using a
normalized/clean relative form) and then perform the `..`, absolute-path, and
`chartsDirName` checks against that normalized value. Make sure the same
canonical form is what later compares against `source === target` and the
role-exclusivity logic in the sync flow.
In `@server/routes/deployments.js`:
- Around line 117-122: The propagation loop in the deployments route is
swallowing write failures, so the endpoint still succeeds even when one or more
target syncs fail. Update the target write logic in the loop that writes each
target’s values file to collect any errors instead of ignoring them, and after
processing all targets return a non-200 response if any propagation failed. Keep
the source save behavior intact, but make the failure visible to the caller when
target writes break.
- Around line 115-121: Revalidate each sync target before using it in the
deployment save flow: `readSyncRegistry()` and `getTargetsForSource()` can
return untrusted raw paths, so the `for (const target of targets)` block in the
deploy route must reject absolute paths and any path that escapes
`req.gitopsDir`. Add a safe path check around the `path.join(req.gitopsDir,
target)` / `fs.mkdir` / `fs.writeFile` sequence so only normalized target
directories under the repo are written to, and skip or error on invalid entries.
In `@server/routes/sync.js`:
- Around line 73-89: The sync flow in server/routes/sync.js overwrites the
target folder before writeSyncRegistry succeeds, which can leave target content
orphaned on failure. Update the sync handler around the sourceDir/targetDir copy
logic to make the target write and registry persistence transactional: back up
the existing target files before writing, and restore them if writeSyncRegistry
throws, or defer applying the target mutation until after the registry write
succeeds. Use the existing sync route logic and writeSyncRegistry as the main
points to adjust.
- Around line 90-92: The sync route error handling currently returns err.message
from fs failures, which can leak absolute server paths to clients. Update the
catch blocks in the sync route handler in server/routes/sync.js to log the
detailed error server-side, then respond with a stable generic 500 JSON error
instead of exposing err.message. Apply the same change in both affected catch
paths so the route behavior is consistent.
In `@src/components/DeleteSourceModal.jsx`:
- Around line 18-32: The handleConfirm flow in DeleteSourceModal.jsx treats
deleteDeployment and unlinkSync as always successful, so the modal can close
with a success message even when a step fails. Update handleConfirm to inspect
each result from deleteDeployment(...) and unlinkSync(...), stop on the first {
ok: false } outcome, and avoid calling message.success, onSuccess, or onClose
unless every step succeeds. Keep setSubmitting(true) at the start and move
setSubmitting(false) into a finally block so it always resets.
In `@src/components/DeploymentTree.jsx`:
- Around line 217-235: Clear the current selection after a successful deployment
delete, since both delete flows currently only call refreshAll() and leave
AlertUserView bound to a removed selectedFolder/selectedChart. Update
handleDeleteClick and the DeleteSourceModal success path to invalidate or
redirect the selection when the deleted node matches the current selection, then
refresh the tree. Use the existing selection/state setters in DeploymentTree.jsx
to reset the active folder/chart consistently for both delete paths.
- Around line 45-73: The deployment actions in DeploymentTree.jsx are only
exposed through the context-menu Dropdown, so add a visible button or similar
trigger alongside the existing menu in the node title rendering. Update the
DeploymentTree JSX around titleContent and the onMenuClick/menuItems setup so
the same sync/unlink/delete items can be opened from a non-context-menu trigger
while keeping the current context menu behavior intact.
In `@src/components/SyncToModal.jsx`:
- Around line 67-79: `SyncToModal` currently adds `newPathTrimmed` directly in
`handleConfirm`, which lets an existing deployment path bypass the same
overwrite checks used for selected rows. Update the `newPath` flow to compare
`newPathTrimmed` against the existing deployment list/targets before confirming,
and treat matches as an existing target that must go through the same red/orange
classification and overwrite acknowledgment gate as `selected` items. Keep
`canConfirm` and `handleConfirm` aligned so the modal cannot submit an
already-synced path without the required confirmation.
In `@src/pages/AlertUserView.jsx`:
- Around line 373-378: Preview in AlertUserView still bypasses the read-only
guard because handlePreview() can call handleSave() while dirty is true even
when frozenSource is set. Update the Preview path in AlertUserView (including
handlePreview and the related Preview button) to respect frozenSource the same
way handleSave does, either by disabling Preview when frozenSource is present or
by preventing handlePreview from invoking save in that state; if appropriate,
also clear dirty when frozenSource becomes truthy.
In `@src/utils/chartApi.js`:
- Around line 188-191: The wrapper functions in chartApi only handle non-OK
responses and still let transport errors from apiFetch or res.json escape.
Update listAllDeployments and the other affected wrappers in this file to use
try/catch around the request/parse path, and return their existing fallback
shapes on network failures as well. Keep the same contracts for each helper: []
for listAllDeployments-style calls, { syncs: [] } for sync loaders, and { ok:
false, error: ... } for action wrappers so callers like the modal Promise.all
flows do not see unhandled rejections.
In `@tests/e2e/deployment-sync.spec.js`:
- Around line 38-142: These deployment sync e2e tests depend on shared mutable
state across test cases, so make the suite explicitly serial by wrapping the
existing deployment sync block with test.describe.serial in
deployment-sync.spec.js. Keep the current test flow and stateful assumptions
intact, but ensure the ordering dependency between the sync, unlink, and
propagation tests is enforced and later tests auto-skip if an earlier step
fails.
---
Outside diff comments:
In `@src/pages/AlertUserView.jsx`:
- Around line 149-161: The handleFolderSelect flow in AlertUserView updates
frozenSource from an async getSyncSource(path) response without checking whether
the response is still current, so a late request can overwrite state for a newer
selectedFolder. Add a staleness guard in the handleFolderSelect and other
getSyncSource call site logic (for example a request token, cancellation flag,
or selectedFolder comparison) before calling setFrozenSource, and only commit
the resolved source when it matches the latest selection.
---
Nitpick comments:
In `@tests/e2e/deployment-sync.spec.js`:
- Around line 89-93: The test is using a brittle first-match locator for an Ant
input, which can target the wrong field when multiple visible inputs exist.
Update the deployment sync e2e assertions to scope the input lookup to the
“Common Values” section or another stable label/role-based target instead of
page.locator('input.ant-input:visible').first(), and apply the same locator
change in both affected assertion blocks. Use the surrounding section heading
and the existing Save button check to anchor the selector to the intended field.
- Around line 39-43: Add teardown for the e2e-sync-test fixtures so reruns start
clean instead of inheriting stale state. Update the deployment-sync suite by
adding cleanup alongside the existing test.beforeAll/initDeployment setup for
ROOT/prod, ROOT/staging, and ROOT/dev, and remove those initialized folders
after the suite finishes. Use the existing initDeployment helper and the
suite-level lifecycle hooks in deployment-sync.spec.js to locate and implement
the cleanup.
🪄 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: 90e2a517-d9ac-410a-961f-84e2958c47ce
📒 Files selected for processing (19)
server.jsserver/lib/sync.jsserver/routes/deployments.jsserver/routes/folders.jsserver/routes/sync.jssrc/components/AlertOverviewWorkspace.jsxsrc/components/AlertTable.jsxsrc/components/DeleteSourceModal.jsxsrc/components/DeploymentTree.jsxsrc/components/SyncFromModal.jsxsrc/components/SyncToModal.jsxsrc/pages/AlertUserView.jsxsrc/utils/chartApi.jstests/e2e/deployment-sync.spec.jstests/integration/deployments-api.test.jstests/integration/deployments-sync.test.jstests/integration/folders-deployments.test.jstests/integration/sync-api.test.jstests/unit/sync.test.js
Contributor
Author
|
Went through CodeRabbit's 14 findings against the actual diff. Scoping one out, fixing the rest here:
Will push fixes incrementally and re-request review once done. |
…istry paths on write POST /:chart/:deployment now rejects writes to a folder that's currently a sync target (409), instead of relying solely on the frontend disabling Save — a race in AlertUserView's getSyncSource call could otherwise let a save through before the UI catches up. The eager-sync propagation loop also re-validates each target path with isSafeSyncPath before writing to it, since sync.yaml is a plain file in the gitops repo and isn't automatically trustworthy just because an entry is in the registry. Found during review of the CodeRabbit findings on this PR.
…can't bypass role exclusivity isSafeSyncPath validated the raw candidate string, so 'cpu/prod', 'cpu/./prod', 'cpu//prod', and 'cpu/prod/' all pointed at the same deployment but were treated as different strings by the registry's strict equality checks (isSource/isTarget/applySync). An unnormalized spelling of an existing source could therefore slip past the "can't also be a target" role-exclusivity guard. Adds normalizeSyncPath and runs it in isSafeSyncPath and at the sync route boundary (GET/POST/DELETE) before any validation, comparison, or registry storage, so the registry always holds one canonical spelling per path. This codebase has shipped two prior path-traversal bugs (#33/#34), which PR #44's own commit history calls out as the reason path validation here is a hard requirement, not a nice-to-have.
…yncToModal The "Add new path" input pushed its value straight into the submit list without ever going through classify() — typing an existing deployment's path bypassed the red/orange overwrite-confirmation checkboxes entirely, letting it be silently overwritten. Manually-typed paths are now classified the same way as picked-from-list rows: known deployments need the same ack, syncing to the source itself is blocked, and unrecognized paths are treated as genuinely new (no ack needed, matching what POST /sync already does for a target that doesn't exist yet).
… reporting success handleConfirm ran through every target's delete/unlink call plus the source's own delete without checking any result, then unconditionally showed a success message. A failed step now stops the sequence, shows an error instead of a false success, and skips onSuccess/onClose so the modal stays open — except the tree is still refreshed if any earlier step did complete, so the sidebar doesn't show stale badges for targets that were already unlinked/deleted before the failure. setSubmitting(false) moved to a finally so it can't get stuck true.
…e suite serial with teardown - New test: typing an existing deployment's path into SyncToModal's "Add new path" field requires the same overwrite ack as picking it from the list (covers the fix in the previous commit). - New test: deleting a sync source honors independent per-target Keep/Delete choices — the Delete flow had zero coverage in any layer before this (this project has no React component-test convention, so E2E is the only place bugs like the ack-bypass or the swallowed-errors one in DeleteSourceModal could actually be caught). - Wrapped the suite in test.describe.serial and added afterAll teardown that removes the ROOT deployments and resets sync.yaml. Without it, reruns inherit sync state from the previous run (e.g. a target left checked+disabled as already-synced), which is what broke the "saving a source propagates" test while developing the tests above.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/DeleteSourceModal.jsx (1)
11-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEffect keys off
targetsreference, not content.If the parent (
DeploymentTree.jsx) passes a fresh array literal fortargetson each render while the modal stays open, this effect re-fires and silently resets any per-target Keep/Delete selections the user already made.♻️ Possible mitigation
- useEffect(() => { - if (!open) return - const initial = {} - for (const t of targets) initial[t] = 'keep' - setDecisions(initial) - }, [open, targets]) + useEffect(() => { + if (!open) return + const initial = {} + for (const t of targets) initial[t] = 'keep' + setDecisions(initial) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open, JSON.stringify(targets)])🤖 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/components/DeleteSourceModal.jsx` around lines 11 - 16, The initialization effect in DeleteSourceModal is re-running whenever the targets array reference changes, which can wipe user-made Keep/Delete selections while the modal remains open. Update the useEffect tied to open and targets so it only initializes decisions when the modal first opens or when the actual target set changes meaningfully, and avoid resetting state on every new array instance passed from DeploymentTree.
🤖 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/components/DeleteSourceModal.jsx`:
- Around line 18-53: The handleConfirm flow in DeleteSourceModal only handles
non-OK responses, so thrown errors from deleteDeployment or unlinkSync can
escape without user feedback. Add a catch around the existing try/finally in
handleConfirm, show message.error with the exception details for any unexpected
failure, and keep the current finally behavior for setSubmitting and onSuccess
refresh handling.
---
Nitpick comments:
In `@src/components/DeleteSourceModal.jsx`:
- Around line 11-16: The initialization effect in DeleteSourceModal is
re-running whenever the targets array reference changes, which can wipe
user-made Keep/Delete selections while the modal remains open. Update the
useEffect tied to open and targets so it only initializes decisions when the
modal first opens or when the actual target set changes meaningfully, and avoid
resetting state on every new array instance passed from DeploymentTree.
🪄 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: 1e93ac72-eb97-453c-9a3b-16eaceec8cbf
📒 Files selected for processing (9)
server/lib/sync.jsserver/routes/deployments.jsserver/routes/sync.jssrc/components/DeleteSourceModal.jsxsrc/components/SyncToModal.jsxtests/e2e/deployment-sync.spec.jstests/integration/deployments-sync.test.jstests/integration/sync-api.test.jstests/unit/sync.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- server/routes/deployments.js
- server/lib/sync.js
- tests/integration/sync-api.test.js
- server/routes/sync.js
- src/components/SyncToModal.jsx
…rget content not reference
Two follow-ups from CodeRabbit's re-review of the previous commit:
- handleConfirm only handled { ok: false } results — a rejected promise
from deleteDeployment/unlinkSync (e.g. a network failure) would escape
as an unhandled rejection with no feedback shown to the user. Wrapped in
a catch that reports it via message.error.
- The decisions-initializing effect depended on the targets array
reference, so a re-render handing down a fresh-but-equal array while the
modal stayed open would silently wipe out Keep/Delete choices already
made. Keyed on targets.join('|') instead.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/DeleteSourceModal.jsx (1)
15-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a collision-free key for target content.
targets.join('|')can collide for different target sets, e.g.['a|b', 'c']and['a', 'b|c'], leaving stale decisions while the modal is open.♻️ Proposed fix
- const targetsKey = targets.join('|') + const targetsKey = JSON.stringify(targets)🤖 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/components/DeleteSourceModal.jsx` around lines 15 - 22, The modal reset logic in DeleteSourceModal is using a non-unique derived key from targets.join('|'), which can collide for different target arrays and leave stale decisions in useEffect. Replace the targetsKey dependency with a collision-free representation based on the full targets contents, and keep the reset logic in the same effect so it reinitializes decisions correctly whenever the open state or the actual target set changes.
🤖 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/components/DeleteSourceModal.jsx`:
- Around line 54-59: The catch path in DeleteSourceModal’s delete flow is unsafe
when deleteDeployment/unlinkSync rejects with null or undefined, because
accessing err.message can throw inside the catch. Update the catch block in
DeleteSourceModal to defensively derive the error text from err only when it is
truthy, and fall back to the existing generic failure message so message.error
always receives a safe string.
---
Nitpick comments:
In `@src/components/DeleteSourceModal.jsx`:
- Around line 15-22: The modal reset logic in DeleteSourceModal is using a
non-unique derived key from targets.join('|'), which can collide for different
target arrays and leave stale decisions in useEffect. Replace the targetsKey
dependency with a collision-free representation based on the full targets
contents, and keep the reset logic in the same effect so it reinitializes
decisions correctly whenever the open state or the actual target set changes.
🪄 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: b3b59a58-3d3f-4400-830f-fc8a473fbc46
📒 Files selected for processing (1)
src/components/DeleteSourceModal.jsx
…writes, registry-first ordering, generic 500s Addresses four CodeRabbit findings on the sync backend: - readSyncRegistry now only treats a missing sync.yaml as an empty registry; parse/permission/I-O errors surface instead of inviting the next write to silently wipe existing links - withSyncRegistryLock serializes all registry read-modify-write sequences (concurrent POST/DELETE could drop each other's changes); in-process is sufficient since each user gets their own singleuser pod - POST /sync persists the registry before touching target files, with a best-effort content restore + unlink rollback if the copy fails — a failed persist can no longer orphan overwritten target content - 500 responses log the real error server-side and return a generic message instead of fs error text that embeds absolute server paths Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…en Preview guard Addresses the remaining CodeRabbit UI findings: - tree node actions get a visible ⋯ button opening the same menu, so keyboard/touch users aren't locked out of right-click-only actions - deleting the currently selected deployment (directly or via the delete-source flow) clears the selection instead of leaving the editor open on content that no longer exists; DeleteSourceModal now reports which paths were actually removed from disk - Preview on a frozen deployment renders on-disk state directly instead of bouncing a save off the server's read-only guard - DeleteSourceModal's catch tolerates nullish rejection values Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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/pages/AlertUserView.jsx (1)
165-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard both
getSyncSourcecalls against stale responses.handleFolderSelectandonSyncChangeeach awaitgetSyncSource(...)and then setfrozenSourceunconditionally, so a slower earlier request can overwrite the current folder’s sync state. Add a current-folder check before updating state, and handle fetch failures too.🤖 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/pages/AlertUserView.jsx` around lines 165 - 178, The frozen source update in handleFolderSelect and onSyncChange is vulnerable to stale async responses overwriting the current folder’s state. After awaiting getSyncSource(path), verify the selected folder is still the active one before calling setFrozenSource, and apply the same guard in both handlers. Also wrap the getSyncSource call in error handling so failures do not leave the UI in an inconsistent state.
🤖 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/pages/AlertUserView.jsx`:
- Around line 165-178: The frozen source update in handleFolderSelect and
onSyncChange is vulnerable to stale async responses overwriting the current
folder’s state. After awaiting getSyncSource(path), verify the selected folder
is still the active one before calling setFrozenSource, and apply the same guard
in both handlers. Also wrap the getSyncSource call in error handling so failures
do not leave the UI in an inconsistent state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3ce4bc2-60bd-4ab5-82d4-d971ddcb470e
📒 Files selected for processing (8)
server/lib/sync.jsserver/routes/deployments.jsserver/routes/sync.jssrc/components/DeleteSourceModal.jsxsrc/components/DeploymentTree.jsxsrc/pages/AlertUserView.jsxtests/e2e/deployment-sync.spec.jstests/integration/sync-api.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- server/routes/deployments.js
- src/components/DeploymentTree.jsx
readSyncRegistry now (deliberately) treats a corrupt registry as a hard error, which makes a crash mid-write more costly than before — a truncated sync.yaml would take the whole sync API down until manually repaired. Write to a temp file and rename into place so the registry on disk is always either the old or the new complete content. The registry-write-failure integration test switches from a read-only file to a read-only directory, since rename replaces the target regardless of its file permissions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rapid folder switches fire overlapping getSyncSource calls, and a slower earlier response landing last could freeze (or unfreeze) the wrong folder. Both handleFolderSelect and onSyncChange now go through refreshFrozenSource, which only lets the response for the most recently requested folder update state, and tolerates transport failures (failing open is safe — the server enforces read-only on save). Also makes the uppercase-folder e2e save test idempotent: refilling the exact value a previous run already saved doesn't fire React's onChange, so dirty stayed false and Save stayed disabled on any non-fresh workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/AlertUserView.jsx (1)
22-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize
frozenSourcefor restored folder selections
selectedFolderis restored from session state, but the tree only expands ancestors on mount; it does not re-runhandleFolderSelect, sofrozenSourcestaysnulluntil the user reselects the folder or a sync event fires. That leaves a synced deployment looking editable until save time.💡 Proposed fix
+useEffect(() => { + if (selectedFolder) refreshFrozenSource(selectedFolder) +}, [selectedFolder])🤖 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/pages/AlertUserView.jsx` around lines 22 - 27, `AlertUserView` restores `selectedFolder` from session state, but `frozenSource` is never initialized for that restored selection because `handleFolderSelect` only runs on user interaction or sync updates. Update the mount/restore flow in `AlertUserView` so a saved folder selection also initializes `frozenSource` immediately, using the same logic as `handleFolderSelect`/folder sync handling, to keep the deployment state read-only when it should be.
🤖 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/integration/sync-api.test.js`:
- Around line 261-268: The permission-failure case in the sync API test is
brittle under UID 0 and can leave tmpDir read-only if the request throws. Update
the test around request(app).post('/api/v2/sync') to skip or adapt the
chmodSync(tmpDir, 0o555) assertion when running as root, and wrap the read-only
setup/restore in a try/finally so chmodSync(tmpDir, 0o755) always runs even on
failure. Use the existing tmpDir, request(app), and fs.chmodSync calls in this
test to make the cleanup resilient.
---
Outside diff comments:
In `@src/pages/AlertUserView.jsx`:
- Around line 22-27: `AlertUserView` restores `selectedFolder` from session
state, but `frozenSource` is never initialized for that restored selection
because `handleFolderSelect` only runs on user interaction or sync updates.
Update the mount/restore flow in `AlertUserView` so a saved folder selection
also initializes `frozenSource` immediately, using the same logic as
`handleFolderSelect`/folder sync handling, to keep the deployment state
read-only when it should be.
🪄 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: 43baa4a4-9bca-4533-8a68-7dfd0a6f58bb
📒 Files selected for processing (4)
server/lib/sync.jssrc/pages/AlertUserView.jsxtests/e2e/nested-deployment.spec.jstests/integration/sync-api.test.js
…en permission test - Folders restored from session state never pass through handleFolderSelect, so a synced deployment restored on page load looked editable until save time — an effect on selectedFolder now refreshes frozenSource for that path too - The registry-write-failure test skips under root (permissions don't apply to UID 0) and restores directory permissions in a finally so a thrown request can't leave tmpDir unremovable for afterEach Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refreshAll re-inserted expanded-node children in raw expandedKeys order, but antd keeps descendant keys when an ancestor is collapsed and appends the ancestor after them on re-expand — so the array can be child-before- parent. insertChildren against a not-yet-populated parent is a silent no-op, which left expanded nodes empty after any sync/unlink/delete refresh until a full page reload. Sort keys by depth before inserting. E2E covers the collapse → re-expand → sync sequence and asserts the nested deployment stays visible with its source badge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On win32, path.normalize rewrites '/' to '\', so sync.yaml stored non-portable '\'-separated paths — a file that gets committed to the gitops repo and read on Linux — and every registry comparison against '/'-separated tree paths silently missed (no badges, broken auto-unlink, 8 failing sync-api tests on Windows). It also let 'charts\evil' slip past the charts-dir check, which splits on '/', while path.join still resolved it into the charts directory. Fold backslashes into '/' and use path.posix.normalize so both spellings canonicalize identically. Also skip the registry-write-failure test on Windows, where chmod on a directory is a no-op and the write succeeds anyway (same reason it already skips under root). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ches The selectedFolder effect rebuilt the tree down to just the selected folder's ancestor chain on every selection — the loaded children of every other expanded branch were dropped, and antd's internal loadedKeys then blocked loadData from refetching them, so those branches re-expanded empty until a full page reload. Fold the effect into refreshAll: it now takes ensureKeys (ancestors that must end up expanded) and unions them with the user's current expansion set instead of replacing it. The mount/refreshKey effect skips when a folder is selected so two concurrent refreshes can't race, with the loser clobbering the winner's tree with a staler expansion set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unsaved edits live only in React state, and both switching deployments and reloading dropped them silently — with sync in the picture the stakes are higher, since an eagerly-propagated target can't be re-edited to reconstruct what was lost. Folder switches (tree click or new-deployment create) now go through a Discard/Keep-editing confirmation while dirty, and a beforeunload warning covers reload/close. Frozen folders skip the guard: their inputs are disabled, so a lingering dirty flag there can't be real work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rophy
approved these changes
Jul 5, 2026
This was referenced Jul 5, 2026
HahaSula
added a commit
that referenced
this pull request
Jul 6, 2026
Bump version from 1.3.0 to 1.4.0 to trigger a new image build and GitHub Release via CI. Since 1.3.0: deployment sync (#44), alert overview mode with column filters (#37), promtool validation (#41, #43), plus several fixes — enough new features to warrant a minor bump per this project's convention (1.2.0 -> 1.3.0 similarly bumped minor for the Gitea migration).
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
2 participants
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.
Summary
Implements the deployment sync feature specced out in #39: a deployment (target) can follow a source deployment. Saving the source eagerly propagates its content to every registered target, and a synced target is frozen (read-only) until explicitly unlinked.
This closes out the full spec in #39 including the four rounds of design revisions in that issue's comments (role-exclusivity flat tree, 🟢/🔴/🟠/⚫ status system in the sync modals, path validation against
charts/, and the API contract).server/lib/sync.js— registry CRUD (sync.yaml), role-exclusivity enforcement, path safety validationserver/routes/sync.js—GET/POST/DELETE /api/v2/syncper the contract in feat: deployment sync — follow a source deployment with eager propagation #39server/routes/deployments.js— eager sync on save, auto-unlink on delete; also fixes a pre-existing bug where DELETE never removed folder-mode deployments (only a legacy sibling filename)server/routes/folders.js— newGET /folders/deploymentsrecursive listing, used by the sync modals' candidate pickers (kept separate from the lazy-loaded tree)DeploymentTree(the actually-active sidebar —DeploymentSelector.jsxturned out to be unused dead code) — source/synced badges, right-click Sync to/Sync from/Unlink/Delete menu gated by roleSyncToModal/SyncFromModal/DeleteSourceModal— new components implementing the 🟢/🔴/🟠/⚫ status system and Keep/Delete flow from the specAlertUserView/AlertTable/AlertOverviewWorkspace— frozen read-only state with a "Synced from X" bannerNotable fixes found along the way (not sync-specific)
DELETE /:chart/:deploymentonly ever removed a legacy-named sibling file, silently leaving folder-mode deployments (Chart.yaml+values.yamldirectly in the folder) fully intact on diskDeploymentTreewhere tree node handlers frozen at an earlier render would refresh against an outdatedexpandedKeys, wiping out already-loaded children after a mutation — fixed with a refdeployments/, so it can collide withcharts/(this PR only adds the narrower guard needed for sync's own paths)Test plan
npm test— 315/316 passing (the one failure is pre-existing and unrelated: a git-locale string match issue intests/unit/git-lib.test.js)npm run lint— cleannpm run build— succeedsnpm run test:e2e— 52/52 passing, including newtests/e2e/deployment-sync.spec.jscovering Sync to via the UI, frozen read-only state, Unlink sync, and eager propagation🤖 Generated with Claude Code
Summary by CodeRabbit