feat(console): mark immutable resource fields as read-only in edit forms#6
Open
lexfrei wants to merge 6 commits into
Open
feat(console): mark immutable resource fields as read-only in edit forms#6lexfrei wants to merge 6 commits into
lexfrei wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds Kubernetes-style immutability enforcement to the console UI. It discovers immutable schema fields via ChangesImmutability Discovery and Overlay
SchemaForm Immutability UI
Update Preparation
Schema Sanitization
Edit Page Integration
Documentation and Configuration
Sequence Diagram(s)sequenceDiagram
participant User
participant SchemaForm
participant findImmutablePaths
participant overlayImmutable
participant ApplicationOrderPage
participant API
User->>ApplicationOrderPage: Click edit
ApplicationOrderPage->>SchemaForm: Set immutableMode="enforce"
ApplicationOrderPage->>ApplicationOrderPage: Capture initialSpec
SchemaForm->>findImmutablePaths: Parse schema for immutable paths
findImmutablePaths-->>SchemaForm: List of immutable paths
SchemaForm->>SchemaForm: Disable UI nodes on immutable paths
User->>SchemaForm: Edit non-immutable fields
User->>ApplicationOrderPage: Click save
ApplicationOrderPage->>overlayImmutable: Apply overlay with captured initial spec
overlayImmutable-->>ApplicationOrderPage: Spec with immutable fields restored
ApplicationOrderPage->>API: PUT request with prepared spec
API-->>ApplicationOrderPage: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements immutability enforcement in the SchemaForm component by parsing x-kubernetes-validations and applying ui:disabled and ui:help to immutable paths. It introduces a prepareUpdateSpec utility to ensure immutable values are preserved during updates and adds Vitest for testing. The review feedback highlights improvements for the array overlay logic to prevent potential null-value issues, suggests consolidating utility functions like isPlainObject and deepClone for better consistency, and recommends cleaning up redundant code in the update preparation logic.
Comment on lines
+120
to
+131
| const sourceArr = Array.isArray(source) ? source : [] | ||
| const targetArr = Array.isArray(target) ? target : [] | ||
| const len = Math.max(sourceArr.length, targetArr.length) | ||
| const out: unknown[] = [] | ||
| for (let i = 0; i < len; i++) { | ||
| if (i >= sourceArr.length) { | ||
| out[i] = deepClone(targetArr[i]) | ||
| } else { | ||
| out[i] = overlayPath(targetArr[i], sourceArr[i], path, depth + 1) | ||
| } | ||
| } | ||
| return out |
There was a problem hiding this comment.
The current array overlay logic can produce arrays with undefined elements (holes) when the source array is longer than the target array (e.g., when an item is deleted). When serialized to JSON, these holes become null, which may violate the schema or cause unexpected behavior on the API server. Using targetArr.map is a safer and more concise way to handle positional overlays while correctly respecting deletions and additions.
const sourceArr = Array.isArray(source) ? source : []
const targetArr = Array.isArray(target) ? target : []
return targetArr.map((item, i) =>
i < sourceArr.length
? overlayPath(item, sourceArr[i], path, depth + 1)
: deepClone(item),
)
Comment on lines
+19
to
+20
| const isPlainObject = (v: unknown): v is Record<string, unknown> => | ||
| v !== null && typeof v === "object" && !Array.isArray(v) |
There was a problem hiding this comment.
Export isPlainObject so it can be reused in SchemaForm.tsx and deepClone for better consistency and code reuse.
Suggested change
| const isPlainObject = (v: unknown): v is Record<string, unknown> => | |
| v !== null && typeof v === "object" && !Array.isArray(v) | |
| export const isPlainObject = (v: unknown): v is Record<string, unknown> => | |
| v !== null && typeof v === "object" && !Array.isArray(v) |
Comment on lines
+143
to
+154
| function deepClone<T>(value: T): T { | ||
| if (value === null || value === undefined) return value | ||
| if (Array.isArray(value)) return value.map(deepClone) as unknown as T | ||
| if (typeof value === "object") { | ||
| const out: Record<string, unknown> = {} | ||
| for (const [k, v] of Object.entries(value as Record<string, unknown>)) { | ||
| out[k] = deepClone(v) | ||
| } | ||
| return out as unknown as T | ||
| } | ||
| return value | ||
| } |
There was a problem hiding this comment.
Export deepClone to allow its reuse in prepare-update.ts, and use the isPlainObject helper for a cleaner and more consistent implementation.
Suggested change
| function deepClone<T>(value: T): T { | |
| if (value === null || value === undefined) return value | |
| if (Array.isArray(value)) return value.map(deepClone) as unknown as T | |
| if (typeof value === "object") { | |
| const out: Record<string, unknown> = {} | |
| for (const [k, v] of Object.entries(value as Record<string, unknown>)) { | |
| out[k] = deepClone(v) | |
| } | |
| return out as unknown as T | |
| } | |
| return value | |
| } | |
| export function deepClone<T>(value: T): T { | |
| if (value === null || value === undefined) return value | |
| if (Array.isArray(value)) return value.map(deepClone) as unknown as T | |
| if (isPlainObject(value)) { | |
| const out: Record<string, unknown> = {} | |
| for (const [k, v] of Object.entries(value)) { | |
| out[k] = deepClone(v) | |
| } | |
| return out as unknown as T | |
| } | |
| return value | |
| } |
Comment on lines
+1
to
+5
| import { | ||
| findImmutablePaths, | ||
| overlayImmutable, | ||
| type ImmutablePath, | ||
| } from "./immutable-paths.ts" |
There was a problem hiding this comment.
Add deepClone to the imports from ./immutable-paths.ts to replace the redundant cloneShallowAsDeep function.
Suggested change
| import { | |
| findImmutablePaths, | |
| overlayImmutable, | |
| type ImmutablePath, | |
| } from "./immutable-paths.ts" | |
| import { | |
| findImmutablePaths, | |
| overlayImmutable, | |
| deepClone, | |
| type ImmutablePath, | |
| } from "./immutable-paths.ts" |
| openAPISchema: string, | ||
| ): T { | ||
| if (original === undefined || original === null) { | ||
| return cloneShallowAsDeep(submitted) |
There was a problem hiding this comment.
Use the deepClone utility instead of cloneShallowAsDeep.
Suggested change
| return cloneShallowAsDeep(submitted) | |
| return deepClone(submitted) |
Comment on lines
+46
to
+49
| function cloneShallowAsDeep<T>(value: T): T { | ||
| if (value === null || value === undefined) return value | ||
| return JSON.parse(JSON.stringify(value)) as T | ||
| } |
There was a problem hiding this comment.
This function is redundant as deepClone is already implemented in immutable-paths.ts. Additionally, the name is misleading (it performs a deep clone) and the implementation using JSON.stringify is less efficient and can strip undefined values from objects.
Comment on lines
+6
to
+10
| import { | ||
| IMMUTABLE_HELP_TEXT, | ||
| findImmutablePaths, | ||
| type ImmutablePath, | ||
| } from "../lib/immutable-paths.ts" |
There was a problem hiding this comment.
Add isPlainObject to the imports from ../lib/immutable-paths.ts.
Suggested change
| import { | |
| IMMUTABLE_HELP_TEXT, | |
| findImmutablePaths, | |
| type ImmutablePath, | |
| } from "../lib/immutable-paths.ts" | |
| import { | |
| IMMUTABLE_HELP_TEXT, | |
| findImmutablePaths, | |
| isPlainObject, | |
| type ImmutablePath, | |
| } from "../lib/immutable-paths.ts" |
| key: string, | ||
| ): Record<string, unknown> { | ||
| const existing = uiNode[key] | ||
| if (existing && typeof existing === "object" && !Array.isArray(existing)) { |
There was a problem hiding this comment.
Use the isPlainObject helper for better readability and consistency.
Suggested change
| if (existing && typeof existing === "object" && !Array.isArray(existing)) { | |
| if (isPlainObject(existing)) { |
lexfrei
force-pushed
the
claude/magical-meitner-7db476
branch
from
5727ac1 to
4416ec3
Compare
Introduce pure helpers used to recognise and enforce Kubernetes-style
immutability rules on resource schemas.
findImmutablePaths(schema) walks an OpenAPI/JSON-schema tree and
returns every path whose subschema carries an
x-kubernetes-validations entry with rule "self == oldSelf". Object
properties contribute named segments; array items and
additionalProperties contribute the wildcard segment "*". oneOf,
anyOf and allOf branches are considered when determining whether a
node is immutable.
overlayImmutable(submitted, original, paths) deep-clones the submitted
value, then for each path copies the value from original. Wildcard
semantics dispatch on the runtime value shape:
- *-last on an array: replace with a clone of source (whole-array
immutable).
- *-last on an object map: replace with a clone of source (whole-map
immutable).
- *-not-last on an array: iterate the user's array, restore the
immutable nested subfield from source at shared indices, keep
user-added entries verbatim. Length shrinks are detected and the
overlay is skipped with a console.warn so admission stays the
authoritative enforcer (index-aligned overlay would otherwise
silently swap which element the user deleted).
- *-not-last on an object map: freeze the whole map. The UI marks
such fields ui:disabled (Add/Remove hidden); the overlay matches
so YAML-editor and devtools bypasses can't add or rename keys.
A root-level immutable path or a top-level wildcard immutable path
(both pathological schema shapes) replaces submitted wholesale with a
clone of original and logs a warning.
IMMUTABLE_HELP_TEXT centralises the user-facing string consumed by
later UI wiring.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
AJV has no CEL evaluator, so the rule does nothing for JSON-Schema validation; the UI consults the raw schema once to harvest immutability paths and then drops the extension so RJSF traversal stays small. Also add characterization tests for keysOrderToUiSchema and sanitizeSchema so existing behaviour (int-or-string coercion, preserve-unknown-fields → additionalProperties, "Chart Values" → "Parameters") is locked in alongside the new strip. Widen keysOrderToUiSchema's parameter to ReadonlyArray<ReadonlyArray<string>> so call sites can pass immutable-path-style arrays without casts. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
claude/magical-meitner-7db476
branch
2 times, most recently
from
b41f2aa to
55c8e1e
Compare
Add an immutableMode prop ("enforce" | "off", default "off"). When
"enforce", every immutable schema path (as reported by
findImmutablePaths) receives ui:disabled=true plus a ui:help string
sourced from IMMUTABLE_HELP_TEXT. RJSF then renders the corresponding
inputs greyed out and surfaces the helper text underneath. The default
keeps every field editable so create flows are unaffected.
Wildcard segments map to RJSF's "items" key for arrays; for object
maps (additionalProperties) the field itself is marked disabled and
the existing AdditionalPropertiesField renderer propagates that to
the nested form, hiding Add/Remove controls.
Parse the raw schema once and derive both the sanitised RJSFSchema
and the immutable-path set from the shared parsed object so the JSON
string isn't decoded twice per render.
Add component-level tests covering:
- enforce / off / omitted prop branches on scalar fields,
- per-element-nested-immutable fields inside array items,
- whole-array immutable shape,
- the deliberate UI/overlay asymmetry on additionalProperties
object maps (the whole map is frozen in both UI and overlay).
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
ApplicationOrderPage and BackupResourceEditPage now opt their SchemaForm into immutableMode="enforce" when in edit flow so the relevant fields render greyed out with helper text. On submit, prepareUpdateSpec walks the schema for immutable paths and copies the original values from the persisted spec into the outgoing body. The API server therefore observes no delta on those paths and the CEL immutability check is not triggered, even if the user managed to bypass the read-only UI (YAML editor, devtools, RJSF bug). Both pages anchor the overlay source to a mount-time snapshot stored in a ref so a background React-Query refetch can't sneak a different immutable value into the PUT. The snapshot capture lives inside useEffect to keep render side-effect-free. prepareUpdateSpec is a thin pure wrapper around findImmutablePaths + overlayImmutable. It short-circuits to a deep clone when the persisted spec is null/undefined (avoids blanking immutable fields with undefined) and logs a console.warn when the schema string fails to parse instead of silently disabling enforcement. BackupResourceEditPage.handleSubmit also refuses to save when the CRD schema hasn't loaded yet, mirroring the existing !resource guard. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The workspace already configures vitest + jsdom + @testing-library in apps/console; the README mentioned only install/dev/build. Add a one-line Test section pointing at the workspace-wide "pnpm test". Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
pnpm/action-setup@v4 fails when the workspace doesn't pin a pnpm version anywhere. Declare it in the standard packageManager field on the root package.json so both the Test workflow and any local contributor's corepack pick up the same version we develop against. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
claude/magical-meitner-7db476
branch
from
55c8e1e to
d51f984
Compare
lexfrei
added a commit
to cozystack/cozystack
that referenced
this pull request
May 15, 2026
Annotate storageClass as immutable across every stateful app whose chart exposes one — clickhouse, foundationdb, harbor, http-cache, kafka (Kafka + ZooKeeper), kubernetes (top-level + nodeGroups[]), mariadb, mongodb, nats, openbao, opensearch, postgres, qdrant, rabbitmq, redis, vm-disk. Without coverage across the catalogue, the UI would render storageClass read-only on some apps and editable on others — a worse mental model than uniform behaviour. The annotation lands at the schema layer (CEL XValidation rule 'self == oldSelf' on values.schema.json / types.go / embedded openAPISchema in cozyrds/<app>.yaml). The cozystack aggregated apiserver does not yet evaluate CEL on application-backed resources; that is tracked at #2657. The UI (cozystack/cozystack-ui#6) consumes the marker today. The PVC-pinning argument is identical for every chart: Kubernetes fixes a PVC's storageClassName at creation time, so editing this field on an existing resource never migrated data. The schema annotation makes that contract explicit. A refactor in pkg/registry/apps/application/rest.go extracts the inline schema-build path into buildSpecSchema so it can be unit-tested. Three regression tests in rest_spec_schema_test.go pin the contract that x-kubernetes-validations survives the v1→internal→structural conversion (loaded from the actual MariaDB ApplicationDefinition fixture on disk, not a hand-edited mock). Requires cozystack/cozyvalues-gen with @immutable support — bump the pre-commit pin in .github/workflows/pre-commit.yml once the release exists. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
added a commit
to cozystack/cozystack
that referenced
this pull request
May 15, 2026
Annotate storageClass as immutable across every stateful app whose chart exposes one — clickhouse, foundationdb, harbor, http-cache, kafka (Kafka + ZooKeeper), kubernetes (top-level + nodeGroups[]), mariadb, mongodb, nats, openbao, opensearch, postgres, qdrant, rabbitmq, redis, vm-disk. Without coverage across the catalogue, the UI would render storageClass read-only on some apps and editable on others — a worse mental model than uniform behaviour. The annotation lands at the schema layer (CEL XValidation rule 'self == oldSelf' on values.schema.json / types.go / embedded openAPISchema in cozyrds/<app>.yaml). The cozystack aggregated apiserver does not yet evaluate CEL on application-backed resources; that is tracked at #2657. The UI (cozystack/cozystack-ui#6) consumes the marker today. The PVC-pinning argument is identical for every chart: Kubernetes fixes a PVC's storageClassName at creation time, so editing this field on an existing resource never migrated data. The schema annotation makes that contract explicit. A refactor in pkg/registry/apps/application/rest.go extracts the inline schema-build path into buildSpecSchema so it can be unit-tested. Three regression tests in rest_spec_schema_test.go pin the contract that x-kubernetes-validations survives the v1→internal→structural conversion (loaded from the actual MariaDB ApplicationDefinition fixture on disk, not a hand-edited mock). Requires cozystack/cozyvalues-gen with @immutable support — bump the pre-commit pin in .github/workflows/pre-commit.yml once the release exists. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
added a commit
to cozystack/cozystack
that referenced
this pull request
May 15, 2026
Annotate storageClass as immutable across every stateful app whose chart exposes one — clickhouse, foundationdb, harbor, http-cache, kafka (Kafka + ZooKeeper), kubernetes (top-level + nodeGroups[]), mariadb, mongodb, nats, openbao, opensearch, postgres, qdrant, rabbitmq, redis, vm-disk. Without coverage across the catalogue, the UI would render storageClass read-only on some apps and editable on others — a worse mental model than uniform behaviour. The annotation lands at the schema layer (CEL XValidation rule 'self == oldSelf' on values.schema.json / types.go / embedded openAPISchema in cozyrds/<app>.yaml). The cozystack aggregated apiserver does not yet evaluate CEL on application-backed resources; that is tracked at #2657. The UI (cozystack/cozystack-ui#6) consumes the marker today. The PVC-pinning argument is identical for every chart: Kubernetes fixes a PVC's storageClassName at creation time, so editing this field on an existing resource never migrated data. The schema annotation makes that contract explicit. A refactor in pkg/registry/apps/application/rest.go extracts the inline schema-build path into buildSpecSchema so it can be unit-tested. Three regression tests in rest_spec_schema_test.go pin the contract that x-kubernetes-validations survives the v1→internal→structural conversion (loaded from the actual MariaDB ApplicationDefinition fixture on disk, not a hand-edited mock). Requires cozystack/cozyvalues-gen with @immutable support — bump the pre-commit pin in .github/workflows/pre-commit.yml once the release exists. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
added a commit
to cozystack/cozystack
that referenced
this pull request
May 15, 2026
Annotate storageClass as immutable across every stateful app whose chart exposes one — clickhouse, foundationdb, harbor, http-cache, kafka (Kafka + ZooKeeper), kubernetes (top-level only), mariadb, mongodb, nats, openbao, opensearch, postgres, qdrant, rabbitmq, redis, vm-disk. Without coverage across the catalogue the UI would render storageClass read-only on some apps and editable on others — a worse mental model than uniform behaviour. The annotation lands at the schema layer (CEL XValidation rule 'self == oldSelf' on values.schema.json / types.go / embedded openAPISchema in cozyrds/<app>.yaml). The cozystack aggregated apiserver does not yet evaluate CEL on application-backed resources; that is tracked at #2657. The UI (cozystack/cozystack-ui#6) consumes the marker today. The PVC-pinning argument is identical for every chart: Kubernetes fixes a PVC's storageClassName at creation time, so editing this field on an existing resource never migrated data. The schema annotation makes that contract explicit. kubernetes.nodeGroups[name].storageClass is intentionally NOT annotated: the field is optional and undefaulted, so a strict 'self == oldSelf' rule would block any future attempt to set it on an existing node group. A richer ratcheting form is needed there — see docs/storage-immutability.md. A refactor in pkg/registry/apps/application/rest.go extracts the inline schema-build path into buildSpecSchema so it can be unit-tested. Three regression tests in rest_spec_schema_test.go pin the contract that x-kubernetes-validations survives the v1->internal->structural conversion (loaded from the actual MariaDB ApplicationDefinition fixture on disk, not a hand-edited mock). Each per-app README gains a one-line blockquote pointing to a new docs/storage-immutability.md that explains the contract and which consumers enforce it today. Requires cozystack/cozyvalues-gen with @immutable support — bump the pre-commit pin in .github/workflows/pre-commit.yml once the release exists. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/console/src/lib/immutable-paths.ts`:
- Around line 123-147: wildcardArrayLengthChanged currently returns when it sees
the first "*" segment and therefore misses shrink events deeper in the path;
update wildcardArrayLengthChanged so that when you encounter a "*" that is not
the last segment you iterate over the array elements (using subCur and origCur)
up to Math.min(subCur.length, origCur.length) and for each element
recursively/iteratively continue checking the remaining path segments instead of
returning early; if any element-check detects shrink (or if subCur and origCur
themselves are arrays and subCur.length < origCur.length at the current non-last
"*" level) return true, otherwise continue; keep the existing behavior for the
last "*" segment (isLast -> false).
In `@apps/console/src/routes/ApplicationOrderPage.tsx`:
- Around line 50-56: initialSpecRef currently holds the same object reference as
editMode.initialSpec, so in useEffect where you set initialSpecRef.current
(guarded by initialSpecCapturedRef), deep-clone the snapshot before storing it
to prevent in-place nested edits from mutating the captured "original"; update
the useEffect that checks editMode and initialSpecCapturedRef to assign a cloned
copy (use structuredClone(editMode.initialSpec) when available, falling back to
a JSON deep-copy) to initialSpecRef.current so the original snapshot remains
immutable.
In `@apps/console/src/routes/BackupResourceEditPage.tsx`:
- Around line 67-71: The current initialization assigns the same object to both
form state and the immutable snapshot (resource.spec -> setFormData and
initialSpecRef.current), causing shared-mutability issues; fix it by creating a
deep copy when setting formData and initialSpecRef (e.g., clone resource.spec
before assigning) so setFormData(resource.spec) and initialSpecRef.current =
resource.spec are replaced with cloned copies, ensuring initializedRef.current
logic remains the same but both form state and the immutable snapshot reference
independent objects.
🪄 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
Run ID: 3a5e802b-fecd-4b84-a881-0cdf6f2294f0
📒 Files selected for processing (13)
README.mdapps/console/src/components/AdditionalPropertiesField.test.tsxapps/console/src/components/SchemaForm.test.tsxapps/console/src/components/SchemaForm.tsxapps/console/src/lib/immutable-paths.test.tsapps/console/src/lib/immutable-paths.tsapps/console/src/lib/keys-order.test.tsapps/console/src/lib/keys-order.tsapps/console/src/lib/prepare-update.test.tsapps/console/src/lib/prepare-update.tsapps/console/src/routes/ApplicationOrderPage.tsxapps/console/src/routes/BackupResourceEditPage.tsxpackage.json
Comment on lines
+123
to
+147
| function wildcardArrayLengthChanged( | ||
| submitted: unknown, | ||
| original: unknown, | ||
| path: ImmutablePath, | ||
| ): boolean { | ||
| let subCur: unknown = submitted | ||
| let origCur: unknown = original | ||
| for (let i = 0; i < path.length; i++) { | ||
| const seg = path[i] | ||
| if (seg === "*") { | ||
| // Whole-array immutability (*-last) is fine to overlay even when the | ||
| // user changed the length — the semantics is "freeze, replace from | ||
| // source", and that's exactly what the *-last branch does. | ||
| const isLast = i === path.length - 1 | ||
| if (isLast) return false | ||
| // Per-element-nested immutability (*-not-last) is the case that can | ||
| // silently corrupt on SHRINK (we'd re-anchor source values onto the | ||
| // user's surviving indices, which may belong to a different element | ||
| // than the user thought they kept). On grow the shared indices stay | ||
| // put and the new entries past source.length keep the user's values, | ||
| // so growth is safe to overlay. | ||
| if (Array.isArray(subCur) && Array.isArray(origCur)) { | ||
| return subCur.length < origCur.length | ||
| } | ||
| return false |
There was a problem hiding this comment.
Handle nested wildcard shrink detection before overlaying immutable array-item paths.
wildcardArrayLengthChanged returns on the first "*" segment, so it can miss array shrinkage on deeper wildcard segments. For nested array paths, this can still re-anchor immutable values to wrong indices.
💡 Suggested fix
function wildcardArrayLengthChanged(
submitted: unknown,
original: unknown,
path: ImmutablePath,
): boolean {
- let subCur: unknown = submitted
- let origCur: unknown = original
- for (let i = 0; i < path.length; i++) {
- const seg = path[i]
- if (seg === "*") {
- // Whole-array immutability (*-last) is fine to overlay even when the
- // user changed the length — the semantics is "freeze, replace from
- // source", and that's exactly what the *-last branch does.
- const isLast = i === path.length - 1
- if (isLast) return false
- // Per-element-nested immutability (*-not-last) is the case that can
- // silently corrupt on SHRINK (we'd re-anchor source values onto the
- // user's surviving indices, which may belong to a different element
- // than the user thought they kept). On grow the shared indices stay
- // put and the new entries past source.length keep the user's values,
- // so growth is safe to overlay.
- if (Array.isArray(subCur) && Array.isArray(origCur)) {
- return subCur.length < origCur.length
- }
- return false
- }
- if (!isPlainObject(subCur) || !isPlainObject(origCur)) return false
- subCur = (subCur as Record<string, unknown>)[seg]
- origCur = (origCur as Record<string, unknown>)[seg]
- }
- return false
+ const visit = (subCur: unknown, origCur: unknown, depth: number): boolean => {
+ if (depth >= path.length) return false
+ const seg = path[depth]
+
+ if (seg === "*") {
+ const isLast = depth === path.length - 1
+ if (isLast) return false
+ if (!Array.isArray(subCur) || !Array.isArray(origCur)) return false
+ if (subCur.length < origCur.length) return true
+
+ const shared = Math.min(subCur.length, origCur.length)
+ for (let i = 0; i < shared; i++) {
+ if (visit(subCur[i], origCur[i], depth + 1)) return true
+ }
+ return false
+ }
+
+ if (!isPlainObject(subCur) || !isPlainObject(origCur)) return false
+ return visit(
+ (subCur as Record<string, unknown>)[seg],
+ (origCur as Record<string, unknown>)[seg],
+ depth + 1,
+ )
+ }
+
+ return visit(submitted, original, 0)
}🤖 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 `@apps/console/src/lib/immutable-paths.ts` around lines 123 - 147,
wildcardArrayLengthChanged currently returns when it sees the first "*" segment
and therefore misses shrink events deeper in the path; update
wildcardArrayLengthChanged so that when you encounter a "*" that is not the last
segment you iterate over the array elements (using subCur and origCur) up to
Math.min(subCur.length, origCur.length) and for each element
recursively/iteratively continue checking the remaining path segments instead of
returning early; if any element-check detects shrink (or if subCur and origCur
themselves are arrays and subCur.length < origCur.length at the current non-last
"*" level) return true, otherwise continue; keep the existing behavior for the
last "*" segment (isLast -> false).
Comment on lines
+50
to
+56
| const initialSpecRef = useRef<unknown>(editMode?.initialSpec) | ||
| const initialSpecCapturedRef = useRef(false) | ||
| useEffect(() => { | ||
| if (editMode && !initialSpecCapturedRef.current) { | ||
| initialSpecCapturedRef.current = true | ||
| initialSpecRef.current = editMode.initialSpec | ||
| } |
There was a problem hiding this comment.
Clone the persisted snapshot before storing it in initialSpecRef.
initialSpecRef currently keeps the same object reference as editable data. If nested edits mutate in place, the “original” snapshot drifts and immutable overlay protection can be bypassed.
Suggested fix
- const initialSpecRef = useRef<unknown>(editMode?.initialSpec)
+ const initialSpecRef = useRef<unknown>(
+ editMode?.initialSpec === undefined
+ ? undefined
+ : JSON.parse(JSON.stringify(editMode.initialSpec)),
+ )
@@
- initialSpecRef.current = editMode.initialSpec
+ initialSpecRef.current = JSON.parse(JSON.stringify(editMode.initialSpec))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const initialSpecRef = useRef<unknown>(editMode?.initialSpec) | |
| const initialSpecCapturedRef = useRef(false) | |
| useEffect(() => { | |
| if (editMode && !initialSpecCapturedRef.current) { | |
| initialSpecCapturedRef.current = true | |
| initialSpecRef.current = editMode.initialSpec | |
| } | |
| const initialSpecRef = useRef<unknown>( | |
| editMode?.initialSpec === undefined | |
| ? undefined | |
| : JSON.parse(JSON.stringify(editMode.initialSpec)), | |
| ) | |
| const initialSpecCapturedRef = useRef(false) | |
| useEffect(() => { | |
| if (editMode && !initialSpecCapturedRef.current) { | |
| initialSpecCapturedRef.current = true | |
| initialSpecRef.current = JSON.parse(JSON.stringify(editMode.initialSpec)) | |
| } |
🤖 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 `@apps/console/src/routes/ApplicationOrderPage.tsx` around lines 50 - 56,
initialSpecRef currently holds the same object reference as
editMode.initialSpec, so in useEffect where you set initialSpecRef.current
(guarded by initialSpecCapturedRef), deep-clone the snapshot before storing it
to prevent in-place nested edits from mutating the captured "original"; update
the useEffect that checks editMode and initialSpecCapturedRef to assign a cloned
copy (use structuredClone(editMode.initialSpec) when available, falling back to
a JSON deep-copy) to initialSpecRef.current so the original snapshot remains
immutable.
Comment on lines
67
to
71
| if (resource?.spec && !initializedRef.current) { | ||
| initializedRef.current = true | ||
| setFormData(resource.spec) | ||
| initialSpecRef.current = resource.spec | ||
| } |
There was a problem hiding this comment.
Avoid sharing the same resource.spec object between form state and immutable snapshot.
formData and initialSpecRef currently point to the same value. Any in-place mutation can invalidate the immutable overlay source used during submit.
Suggested fix
- setFormData(resource.spec)
- initialSpecRef.current = resource.spec
+ const formSpec = JSON.parse(JSON.stringify(resource.spec))
+ const initialSpec = JSON.parse(JSON.stringify(resource.spec))
+ setFormData(formSpec)
+ initialSpecRef.current = initialSpec📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (resource?.spec && !initializedRef.current) { | |
| initializedRef.current = true | |
| setFormData(resource.spec) | |
| initialSpecRef.current = resource.spec | |
| } | |
| if (resource?.spec && !initializedRef.current) { | |
| initializedRef.current = true | |
| const formSpec = JSON.parse(JSON.stringify(resource.spec)) | |
| const initialSpec = JSON.parse(JSON.stringify(resource.spec)) | |
| setFormData(formSpec) | |
| initialSpecRef.current = initialSpec | |
| } |
🤖 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 `@apps/console/src/routes/BackupResourceEditPage.tsx` around lines 67 - 71, The
current initialization assigns the same object to both form state and the
immutable snapshot (resource.spec -> setFormData and initialSpecRef.current),
causing shared-mutability issues; fix it by creating a deep copy when setting
formData and initialSpecRef (e.g., clone resource.spec before assigning) so
setFormData(resource.spec) and initialSpecRef.current = resource.spec are
replaced with cloned copies, ensuring initializedRef.current logic remains the
same but both form state and the immutable snapshot reference independent
objects.
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
1 participant
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
Resource fields declared immutable in the schema via
x-kubernetes-validations: [{rule: "self == oldSelf"}]are now:Create flows are untouched.
Acceptance criterion
The companion cozystack/cozystack#2639 marks MariaDB
storageClassimmutable (itself dependent on cozystack/cozyvalues-gen#24). With all three PRs in place, opening an existing MariaDB instance for edit grays outstorageClass, shows the helper text, keeps the rest editable, and the outgoing PUT carries the originalstorageClassvalue.What's in this PR
apps/console/src/lib/immutable-paths.ts:findImmutablePathswalks the schema tree and returns every path carrying the CEL rule.overlayImmutablecopies the original spec's values along those paths into the submitted body. Wildcard semantics: whole-array/whole-map (*-last) → clone from source; per-element nested (*-not-last) → overlay shared indices/keys, keep user adds, respect deletes. Length shrink with a nested-immutable path bails out with a console.warn (admission stays authoritative). Top-level wildcard paths are handled like root-immutable: warn + clone source.apps/console/src/lib/prepare-update.ts— pure wrapper; warns on malformed schema; short-circuits to a clone when persisted spec is null/undefined.SchemaFormacceptsimmutableMode(enforce | off, default off). When enforcing, walks the schema and emitsui:disabled+ui:helpper immutable path. For object maps (additionalProperties) the field itself is marked disabled — per-entry-value semantics is deliberately deferred.sanitizeSchemastripsx-kubernetes-validationsonce it has been harvested. AJV has no CEL evaluator, so the rule is pure noise to the form validator.ApplicationOrderPageandBackupResourceEditPageopt in to enforcement on edit and anchor the overlay source to a mount-time snapshot so a background React-Query refetch can't sneak a different immutable value into the PUT.Known limitations (pinned with FIXME tests + tracking issues)
propertiesbeneathoneOf/anyOf/allOfbranches.Tests
The console workspace already had vitest + jsdom + @testing-library/react on main; this PR adds 7 new test files (immutable-paths walker, overlay edges, prepare-update, SchemaForm immutable rendering, AdditionalPropertiesField asymmetry, keys-order characterization) totalling 85 passing tests.
pnpm testruns them all.Tested manually against a hand-edited MariaDB
values.schema.jsoncarrying the rule onstorageClass:No-op until cozystack ships the schema annotation
When no schema carries the rule,
findImmutablePathsreturns[]everywhere, all walkers are no-ops, the overlay is identity, edit pages behave exactly as today. The UI lights up resource-by-resource as upstream annotates immutable fields.Summary by CodeRabbit
Release Notes
New Features
Documentation
pnpm testTests
Chores