From d33c039a17f0c8563f3c64e66bc1e726eb41b3f8 Mon Sep 17 00:00:00 2001 From: hsiegeln <37154749+hsiegeln@users.noreply.github.com> Date: Wed, 22 Apr 2026 23:29:01 +0200 Subject: [PATCH] =?UTF-8?q?fix(deploy):=20address=20final=20review=20?= =?UTF-8?q?=E2=80=94=20sensitiveKeys=20snapshot,=20dirty=20scrubbing,=20tr?= =?UTF-8?q?ansition=20race,=20refetch=20invalidations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Issue 1: add List sensitiveKeys as 4th field to DeploymentConfigSnapshot; populate from agentConfig.getSensitiveKeys() in DeploymentExecutor; handleRestore hydrates from snap.sensitiveKeys directly; Deployment type in apps.ts gains sensitiveKeys field - Issue 2: after createApp succeeds, refetchQueries(['apps', envSlug]) before navigate so the new app is in cache before the router renders the deployed view (eliminates transient Save- disabled flash) - Issue 3: useDeploymentPageState useEffect now uses prevServerStateRef to detect local edits; background refetches only overwrite form when no local changes are present - Issue 5: handleRedeploy invalidates dirty-state + versions queries after createDeployment resolves; handleSave invalidates dirty-state after staged save - Issue 10: DirtyStateCalculator strips volatile agentConfig keys (version, updatedAt, updatedBy, environment, application) before JSON comparison via scrubAgentConfig(); adds versionBumpDoesNotMarkDirty test Co-Authored-By: Claude Sonnet 4.6 --- .../app/runtime/DeploymentExecutor.java | 4 ++- .../PostgresDeploymentRepositoryIT.java | 6 ++-- .../runtime/DeploymentConfigSnapshot.java | 4 ++- .../core/runtime/DirtyStateCalculator.java | 18 ++++++++++-- .../runtime/DirtyStateCalculatorTest.java | 29 ++++++++++++++----- ui/src/api/queries/admin/apps.ts | 1 + .../hooks/useDeploymentPageState.ts | 13 +++++++-- .../pages/AppsTab/AppDeploymentPage/index.tsx | 23 +++++++++++---- 8 files changed, 78 insertions(+), 20 deletions(-) diff --git a/cameleer-server-app/src/main/java/com/cameleer/server/app/runtime/DeploymentExecutor.java b/cameleer-server-app/src/main/java/com/cameleer/server/app/runtime/DeploymentExecutor.java index ad7fb1cf..7ddfd02a 100644 --- a/cameleer-server-app/src/main/java/com/cameleer/server/app/runtime/DeploymentExecutor.java +++ b/cameleer-server-app/src/main/java/com/cameleer/server/app/runtime/DeploymentExecutor.java @@ -261,10 +261,12 @@ public class DeploymentExecutor { ApplicationConfig agentConfig = applicationConfigRepository .findByApplicationAndEnvironment(app.slug(), env.slug()) .orElse(null); + List snapshotSensitiveKeys = agentConfig != null ? agentConfig.getSensitiveKeys() : null; DeploymentConfigSnapshot snapshot = new DeploymentConfigSnapshot( deployment.appVersionId(), agentConfig, - app.containerConfig() + app.containerConfig(), + snapshotSensitiveKeys ); pgDeployRepo.saveDeployedConfigSnapshot(deployment.id(), snapshot); diff --git a/cameleer-server-app/src/test/java/com/cameleer/server/app/storage/PostgresDeploymentRepositoryIT.java b/cameleer-server-app/src/test/java/com/cameleer/server/app/storage/PostgresDeploymentRepositoryIT.java index 03336f9b..6e122e8c 100644 --- a/cameleer-server-app/src/test/java/com/cameleer/server/app/storage/PostgresDeploymentRepositoryIT.java +++ b/cameleer-server-app/src/test/java/com/cameleer/server/app/storage/PostgresDeploymentRepositoryIT.java @@ -61,7 +61,8 @@ class PostgresDeploymentRepositoryIT extends AbstractPostgresIT { DeploymentConfigSnapshot snapshot = new DeploymentConfigSnapshot( jarVersionId, agentConfig, - Map.of("memoryLimitMb", 1024, "replicas", 2) + Map.of("memoryLimitMb", 1024, "replicas", 2), + null ); UUID deploymentId = repository.create(appId, appVersionId, envId, "test-container"); @@ -92,7 +93,8 @@ class PostgresDeploymentRepositoryIT extends AbstractPostgresIT { DeploymentConfigSnapshot snapshot = new DeploymentConfigSnapshot( jarVersionId, new ApplicationConfig(), - Map.of() + Map.of(), + null ); UUID deploymentId = repository.create(appId, appVersionId, envId, "test-container-clear"); diff --git a/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DeploymentConfigSnapshot.java b/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DeploymentConfigSnapshot.java index 947bb93c..348c5d7f 100644 --- a/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DeploymentConfigSnapshot.java +++ b/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DeploymentConfigSnapshot.java @@ -2,6 +2,7 @@ package com.cameleer.server.core.runtime; import com.cameleer.common.model.ApplicationConfig; +import java.util.List; import java.util.Map; import java.util.UUID; @@ -15,6 +16,7 @@ import java.util.UUID; public record DeploymentConfigSnapshot( UUID jarVersionId, ApplicationConfig agentConfig, - Map containerConfig + Map containerConfig, + List sensitiveKeys ) { } diff --git a/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DirtyStateCalculator.java b/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DirtyStateCalculator.java index 5187721f..5239922f 100644 --- a/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DirtyStateCalculator.java +++ b/cameleer-server-core/src/main/java/com/cameleer/server/core/runtime/DirtyStateCalculator.java @@ -9,6 +9,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.TreeSet; import java.util.UUID; @@ -22,12 +23,23 @@ import java.util.UUID; */ public class DirtyStateCalculator { + private static final Set AGENT_CONFIG_IGNORED_KEYS = Set.of( + "version", "updatedAt", "updatedBy", "environment", "application" + ); + private final ObjectMapper mapper; public DirtyStateCalculator(ObjectMapper mapper) { this.mapper = mapper; } + private JsonNode scrubAgentConfig(JsonNode node) { + if (!(node instanceof ObjectNode obj)) return node; + ObjectNode copy = obj.deepCopy(); + for (String k : AGENT_CONFIG_IGNORED_KEYS) copy.remove(k); + return copy; + } + public DirtyStateResult compute(UUID desiredJarVersionId, ApplicationConfig desiredAgentConfig, Map desiredContainerConfig, @@ -44,8 +56,10 @@ public class DirtyStateCalculator { String.valueOf(desiredJarVersionId), String.valueOf(snapshot.jarVersionId()))); } - compareJson("agentConfig", mapper.valueToTree(desiredAgentConfig), - mapper.valueToTree(snapshot.agentConfig()), diffs); + compareJson("agentConfig", + scrubAgentConfig(mapper.valueToTree(desiredAgentConfig)), + scrubAgentConfig(mapper.valueToTree(snapshot.agentConfig())), + diffs); compareJson("containerConfig", mapper.valueToTree(desiredContainerConfig), mapper.valueToTree(snapshot.containerConfig()), diffs); diff --git a/cameleer-server-core/src/test/java/com/cameleer/server/core/runtime/DirtyStateCalculatorTest.java b/cameleer-server-core/src/test/java/com/cameleer/server/core/runtime/DirtyStateCalculatorTest.java index 986e38d6..600e5a40 100644 --- a/cameleer-server-core/src/test/java/com/cameleer/server/core/runtime/DirtyStateCalculatorTest.java +++ b/cameleer-server-core/src/test/java/com/cameleer/server/core/runtime/DirtyStateCalculatorTest.java @@ -39,7 +39,7 @@ class DirtyStateCalculatorTest { Map container = Map.of("memoryLimitMb", 512); UUID jarId = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, cfg, container); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, cfg, container, null); DirtyStateResult result = calc.compute(jarId, cfg, container, snap); assertThat(result.dirty()).isFalse(); @@ -53,7 +53,7 @@ class DirtyStateCalculatorTest { Map container = Map.of(); UUID v1 = UUID.randomUUID(); UUID v2 = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(v1, cfg, container); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(v1, cfg, container, null); DirtyStateResult result = calc.compute(v2, cfg, container, snap); @@ -71,7 +71,7 @@ class DirtyStateCalculatorTest { ApplicationConfig desiredCfg = new ApplicationConfig(); desiredCfg.setSamplingRate(1.0); UUID jarId = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployedCfg, Map.of()); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployedCfg, Map.of(), null); DirtyStateResult result = calc.compute(jarId, desiredCfg, Map.of(), snap); @@ -85,7 +85,7 @@ class DirtyStateCalculatorTest { DirtyStateCalculator calc = CALC; ApplicationConfig cfg = new ApplicationConfig(); UUID jarId = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, cfg, Map.of("memoryLimitMb", 512)); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, cfg, Map.of("memoryLimitMb", 512), null); DirtyStateResult result = calc.compute(jarId, cfg, Map.of("memoryLimitMb", 1024), snap); @@ -100,7 +100,7 @@ class DirtyStateCalculatorTest { ApplicationConfig desired = new ApplicationConfig(); desired.setSamplingRate(1.0); UUID jarId = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, null, Map.of()); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, null, Map.of(), null); DirtyStateResult result = calc.compute(jarId, desired, Map.of(), snap); @@ -118,7 +118,7 @@ class DirtyStateCalculatorTest { ApplicationConfig desired = new ApplicationConfig(); desired.setTracedProcessors(Map.of("proc-1", "TRACE")); UUID jarId = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployed, Map.of()); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployed, Map.of(), null); DirtyStateResult result = calc.compute(jarId, desired, Map.of(), snap); @@ -136,7 +136,7 @@ class DirtyStateCalculatorTest { ApplicationConfig desired = new ApplicationConfig(); desired.setApplicationLogLevel("DEBUG"); UUID jarId = UUID.randomUUID(); - DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployed, Map.of()); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployed, Map.of(), null); DirtyStateResult result = calc.compute(jarId, desired, Map.of(), snap); @@ -146,4 +146,19 @@ class DirtyStateCalculatorTest { assertThat(diff.staged()).isEqualTo("DEBUG"); assertThat(diff.deployed()).isEqualTo("INFO"); } + + @Test + void versionBumpDoesNotMarkDirty() { + ApplicationConfig deployedCfg = new ApplicationConfig(); + deployedCfg.setSamplingRate(0.5); + deployedCfg.setVersion(1); + ApplicationConfig desiredCfg = new ApplicationConfig(); + desiredCfg.setSamplingRate(0.5); + desiredCfg.setVersion(2); // bumped by save + UUID jarId = UUID.randomUUID(); + DeploymentConfigSnapshot snap = new DeploymentConfigSnapshot(jarId, deployedCfg, Map.of(), null); + + DirtyStateResult result = CALC.compute(jarId, desiredCfg, Map.of(), snap); + assertThat(result.dirty()).isFalse(); + } } diff --git a/ui/src/api/queries/admin/apps.ts b/ui/src/api/queries/admin/apps.ts index 12b0fa25..79479876 100644 --- a/ui/src/api/queries/admin/apps.ts +++ b/ui/src/api/queries/admin/apps.ts @@ -45,6 +45,7 @@ export interface Deployment { jarVersionId: string; agentConfig: Record | null; containerConfig: Record; + sensitiveKeys: string[] | null; } | null; } diff --git a/ui/src/pages/AppsTab/AppDeploymentPage/hooks/useDeploymentPageState.ts b/ui/src/pages/AppsTab/AppDeploymentPage/hooks/useDeploymentPageState.ts index 485812b1..f6b0f6ee 100644 --- a/ui/src/pages/AppsTab/AppDeploymentPage/hooks/useDeploymentPageState.ts +++ b/ui/src/pages/AppsTab/AppDeploymentPage/hooks/useDeploymentPageState.ts @@ -1,5 +1,5 @@ // ui/src/pages/AppsTab/AppDeploymentPage/hooks/useDeploymentPageState.ts -import { useState, useEffect, useMemo } from 'react'; +import { useState, useEffect, useMemo, useRef } from 'react'; import type { ApplicationConfig } from '../../../../api/queries/commands'; import type { App } from '../../../../api/queries/admin/apps'; @@ -115,9 +115,18 @@ export function useDeploymentPageState( }, [app, agentConfig, envDefaults]); const [form, setForm] = useState(serverState); + const prevServerStateRef = useRef(serverState); useEffect(() => { - setForm(serverState); + // Only overwrite form if the current form value still matches the previous + // server state (i.e., the user has no local edits). Otherwise preserve + // user edits through background refetches. + setForm((current) => { + const hadLocalEdits = + JSON.stringify(current) !== JSON.stringify(prevServerStateRef.current); + prevServerStateRef.current = serverState; + return hadLocalEdits ? current : serverState; + }); }, [serverState]); return { form, setForm, reset: () => setForm(serverState), serverState }; diff --git a/ui/src/pages/AppsTab/AppDeploymentPage/index.tsx b/ui/src/pages/AppsTab/AppDeploymentPage/index.tsx index cc39b5d8..d4b612d3 100644 --- a/ui/src/pages/AppsTab/AppDeploymentPage/index.tsx +++ b/ui/src/pages/AppsTab/AppDeploymentPage/index.tsx @@ -1,5 +1,6 @@ import { useState, useEffect, useRef } from 'react'; import { useParams, useLocation, useNavigate } from 'react-router'; +import { useQueryClient } from '@tanstack/react-query'; import { AlertDialog, Button, Tabs, useToast } from '@cameleer/design-system'; import { useEnvironmentStore } from '../../../api/environment-store'; import { useEnvironments } from '../../../api/queries/admin/environments'; @@ -45,6 +46,7 @@ export default function AppDeploymentPage() { const location = useLocation(); const navigate = useNavigate(); const { toast } = useToast(); + const queryClient = useQueryClient(); const selectedEnv = useEnvironmentStore((s) => s.environment); const { data: environments = [], isLoading: envLoading } = useEnvironments(); const { data: apps = [], isLoading: appsLoading } = useApps(selectedEnv); @@ -218,8 +220,14 @@ export default function AppDeploymentPage() { setStagedJar(null); + // Invalidate dirty-state so the button reflects the new saved state + await queryClient.invalidateQueries({ queryKey: ['apps', envSlug, targetApp.slug, 'dirty-state'] }); + if (!app) { - // Transition to the existing-app view + // Transition to the existing-app view — refetch apps first so the new app + // is in the cache before the router renders the deployed view (prevents + // the transient Save-disabled flash while useApps is loading). + await queryClient.refetchQueries({ queryKey: ['apps', envSlug] }); navigate(`/apps/${targetApp.slug}`); } } catch (e) { @@ -242,7 +250,6 @@ export default function AppDeploymentPage() { if (stagedJar) { const newVersion = await uploadJar.mutateAsync({ envSlug, appSlug: app.slug, file: stagedJar }); versionId = newVersion.id; - setStagedJar(null); } else { if (!currentVersion) { toast({ @@ -257,6 +264,10 @@ export default function AppDeploymentPage() { } await createDeployment.mutateAsync({ envSlug, appSlug: app.slug, appVersionId: versionId }); + setStagedJar(null); + // Invalidate dirty-state and versions so button recomputes after deploy + queryClient.invalidateQueries({ queryKey: ['apps', envSlug, app.slug, 'dirty-state'] }); + queryClient.invalidateQueries({ queryKey: ['apps', envSlug, app.slug, 'versions'] }); } catch (e) { toast({ title: 'Redeploy failed', @@ -344,9 +355,11 @@ export default function AppDeploymentPage() { : prev.variables.envVars, }, sensitiveKeys: { - sensitiveKeys: Array.isArray(a.sensitiveKeys) - ? (a.sensitiveKeys as string[]) - : prev.sensitiveKeys.sensitiveKeys, + sensitiveKeys: Array.isArray(snap.sensitiveKeys) + ? snap.sensitiveKeys + : Array.isArray(a.sensitiveKeys) + ? (a.sensitiveKeys as string[]) + : prev.sensitiveKeys.sensitiveKeys, }, }; });