feat(delete): optionally remove resources associated with a zone or domain (PPT-1203) - #307
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The cascade flow needs to fail closed before this is safe to merge.
- [P1] Domain lookup failures can delete another domain's org zone: converting
queryDomainsfailures to an empty list makes an unavailable or partially paginated ownership list look like no other domain references the zone. - [P1] Failed system deletion does not prevent deleting its zone:
runCascadecontinues after errors, while domain plans append the org-zone deletion after system removals, so a failed system can be orphaned. - [P1] Failed plan resolution silently falls back to ordinary deletion: when details resolution rejects, the selected option has no plan, the cascade branch is skipped, and
actions.removestill runs. Please make plan resolution fail closed, stop dependent deletion steps after a failure, and block confirmation unless every selected option has a valid plan.
…omain (PPT-1203) Deleting a zone has never removed the systems inside it. `Zone#destroy` cascades child zones, trigger instances, metadata, settings and group links, but `sys.zones` is a text array with no foreign key, so the zone id was simply stripped and any system left with no zones became an orphan (PROJ-845 — the dev install currently carries six such systems). The confirmation copy already claimed otherwise: "Deleting this zone will immediately remove systems without another zone". The delete confirmation now offers "Also delete associated resources", **off by default** — deleting an item without touching what hangs off it stays the default behaviour. Switching it on resolves a plan first and shows exactly what would go before anything is confirmed. Zones remove the systems whose every zone falls inside the subtree, so a system shared with a zone outside it is kept, as it is today. The backend takes their modules, triggers, metadata and settings. Domains remove their OAuth applications (`oauth_applications.owner_id` has no foreign key, so these orphan today) and their staff API tenant, matched on domain name the same way the admin screen does. Zones are only reachable from a domain through the `authority.config.org_zone` convention, which is not exclusive — three domains share one org zone on the dev install — so the zone tree is only included when no other domain references it, and the dialog names the domains that caused it to be skipped. Because the removals are destructive, the system index (Elasticsearch, which can lag) only nominates candidates; each one is re-read through `showSystem` and re-checked against the database before it makes the list. Also here: - `ConfirmModalComponent` gains optional checkboxes with lazily resolved detail, reported back on the confirmation event. Existing callers pass no options and are unaffected. - The zone mock filtered on a `parent` query param the API has never sent; it now honours `parent_id`, including comma separated lists and `root`. Mock zones gained the `parent_id` hierarchy their `zones` arrays imply. - `config/proxy.conf.js` accepts `PLACEOS_DOMAIN` so the dev server can point at a local stack. - `ZONES.DELETE_MSG` now describes what actually happens. The stale translation was dropped from the non-English locales so they fall back to the corrected source string rather than repeating the old claim. Verified against a local PlaceOS stack: an org > building > two levels tree with three systems (two enclosed, one straddling an outside zone) and their modules. With the option on, the tree, both enclosed systems and both modules were removed while the straddling system survived holding only its outside zone; with it off, the zone went and the system was left orphaned exactly as before. A domain delete removed its org zone tree, orphaned system, OAuth application and staff API tenant; a domain sharing its org zone with another left the zone, its system and the other domain untouched.
The org zone line already reads "and everything beneath it", so appending the nested zone plan's own scope line stacked two "Scope:" sentences on top of each other in the dialog.
The checkbox label and the empty box outline both rendered near-black on the dark background. The app already styles `mat-checkbox`, but those rules target `.mdc-checkbox__background` and `.mdc-label`, which never win against Angular Material's own selectors — so the colours were still coming from the prebuilt `indigo-pink` (light) palette. Nothing hit this before because no other `<mat-checkbox>` in the app has label content, and the faint outline was easy to miss. Set at the token layer instead, where Material actually reads them, using `currentColor` so they follow whichever theme is active. Measured on the delete confirmation: label outline dark 1.21:1 -> 10.84:1 (WCAG wants 3:1 for controls) label text dark 1.18:1 -> 10.84:1 (WCAG wants 4.5:1 for body text) Light theme is unchanged at 20.12:1. This affects every checkbox on the dark theme, all of which had the same invisible outline. The cascade warning line used `text-warning` — a yellow that only reads on a dark background (1.39:1 on the light theme). It now tints the block and keeps the inherited text colour, matching how warnings are done elsewhere. Secondary text nudged from 60% to 70% opacity for the same reason. Every line in the dialog now passes WCAG AA in both themes.
A toast saying "Removed 6 associated resources" is not something you can act
on afterwards. When a cascade runs, the confirmation now becomes a receipt
listing every resource that went — type, name and id — in the order they were
removed, ending with the item itself. A "Copy list" button puts it on the
clipboard as tab separated rows for pasting into a ticket.
Cascade steps carry a `CascadeResource` ({type, id, name}) instead of a
pre-rendered progress string, and `CascadeOutcome.removed` is that list rather
than a count, so the receipt reports what actually happened rather than what
was planned.
Partial failures now show the same receipt with the failures called out
separately, and the item is deliberately left in place — previously this was a
notification that named only the first failure and left the operator guessing
which of the rest had gone.
Existing delete flows are untouched: the receipt only appears when a cascade
actually ran, so every other resource type still closes the dialog and shows
its usual notification. There is an e2e test asserting exactly that.
`receiptToTsv` is a pure function so the clipboard formatting is tested
without DI. It also keeps `@angular/cdk/clipboard` out of the spec, which
upsets vitest's `vi.mock` hoisting analysis and made it warn about mocks it
had previously accepted.
Verified against the local stack: deleting a domain with two OAuth apps, a
staff tenant, an org zone tree and three enclosed systems produced an 8 row
receipt, and every id on it was confirmed absent from the API afterwards
while the four intended survivors remained.
Captured from the local stack for the pull request. 512KB total — say the word if the team would rather not carry them in tree and I will move them out.
Addresses Alex's review. The theme across all three points was the same:
an error anywhere in the flow degraded to "proceed", when the only safe
default for a destructive action is "stop".
Plan resolution no longer swallows failures. `queryDomains`,
`queryApplications` and `querySystems` used to fall back to an empty
list, which reads as "nothing found" rather than "we could not look".
The domain case was the dangerous one: the org-zone ownership check is
the only thing preventing this flow from deleting a zone tree that
another domain still points at, and an unavailable list made a shared
zone look unshared. `collectPages` now raises instead of returning a
truncated list for the same reason — a short answer and a complete one
were indistinguishable, and a sharing domain could sit on any unread
page.
`runCascade` stops at the first failure. Steps are ordered by
dependency — `planDomainCascade` appends the org zone after the systems
inside it — so continuing past a failed system removal deleted the zone
out from under it. Everything after the failure is now reported as
skipped rather than silently not attempted, and the receipt shows those
rows. The existing copy ("so nothing further was deleted") already
described this behaviour; now it is true.
Confirmation is withheld while a selected option has no usable
breakdown. Previously a rejected `details()` left the plan null, the
cascade branch was skipped, and `actions.remove` still ran — deleting
the parent on its own and orphaning everything the cascade existed to
remove. The modal now disables confirm in that state, and `item.service`
refuses the plain removal as a second line of defence. Options that
start enabled resolve their breakdown on init, so they cannot sit
selected-with-no-details and hold the button disabled for good.
Two existing tests asserted the old behaviour and have been inverted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8347cf3 to
d77aaad
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/placeos?upgradeToPro=build-rate-limit |
|
Thanks @MrYuion — all three were real, and the shared root was exactly as you put it: every failure path degraded to "proceed". Fixed in 1. Domain lookup failures could delete another domain's org zoneAgreed, and this was the one that could actually destroy someone else's data. The ownership check is the only thing standing between this flow and another domain's zone tree, and
Your "partially paginated" wording sent me to 2. Failed system deletion did not prevent deleting its zoneAgreed. Worth noting the user-facing copy already said "so nothing further was deleted". The string was describing behaviour the code didn't have. It does now. 3. Failed plan resolution silently fell back to ordinary deletionAgreed, and this is the one that would have quietly orphaned things in normal use, since it needs nothing more than a flaky lookup. Done in both places you asked for:
While doing this I found an adjacent bug: an option declared TestsTwo existing tests asserted the old behaviour and have been inverted — Backoffice suite: 900 passed, lint clean, production build green. One thing I did not change
|
…l review found A multi-lens review of this PR found 26 substantiated defects, several of the same class Alex raised — the previous commit fixed his three findings without fixing their cause. Plan resolution, remaining fail-open paths: - `domainTenants` still swallowed every staff-API failure. That listing crosses a service boundary and is admin gated, so it fails for reasons unrelated to the domain having no tenant. The plan then rendered "Nothing else to remove" — a positive claim of absence — and the tenant survived the domain that owned it, still holding its encrypted calendar credentials. - `zoneSubtreeIds` truncated silently at MAX_ZONES, the same mistake `collectPages` had. It now raises. Over-deletion: - The org-zone sharing check compared ids, but deleting the zone destroys everything beneath it. A domain whose own org zone was a *child* of this one was invisible to the check and destroyed with it. The check now tests membership of the whole subtree, reusing that walk for the plan rather than repeating it. - `inside()` used `zones.every(...)`, and `[].every()` is true, so a system with no zones counted as "entirely inside" and was deleted. A system whose zones were just cleared, still listed by the lagging search index, is exactly that case. Half-deleted states with no record: - The dialog stayed dismissable while the cascade ran, and the footer is hidden during loading, so Escape was the only thing to press. That nulled the component the progress callback writes to, threw outside `runCascade`'s try, and abandoned an irreversible run with no receipt and no notification. It now disables close for the duration, guards the derefs, and the chain has a catch so nothing can fail silently. - A cascade that fully succeeded followed by a failed delete of the item itself discarded the receipt entirely — the only record of what had already gone — and re-rendered a confirmation whose breakdown was no longer true. It now shows the partial receipt. Modal: - `resolving()` is scoped to selected options, so unticking releases the confirm button instead of holding it until an abandoned request settles. - `_resolveDetails` guards against a second in-flight resolution, so a stale rejection cannot overwrite a good breakdown. - Failures render via `describeError` rather than as "[object Response]", which is what ts-client's raw `Response` rejection produced. - Dropped the dead `CASCADE.SUCCESS` keys and the spec that pinned them. Every fix is mutation-tested: reverting each one individually fails a test, including the receipt and TSV rendering of skipped rows, which the review showed could be deleted wholesale with the suite still green. 910 tests pass, lint clean, production build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`log()` takes an array of loggable values, not `unknown`. The previous commit passed the raw rejection and only unit tests and lint were run before pushing, both of which use a looser type-check than the AOT production build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e zone-walk cap Two findings from the re-review, neither blocking but both worth having. `CASCADE.FAILED` fell back to the resource *name* when the rejection had no `.message` — and ts-client rejects with a raw `Response`, which never has one, so the toast read "Error: Level 31 Boardroom" for a 403. The status is the one thing an admin needs to know when an irreversible run has stopped part-way, and the receipt already names the resource. Uses `describeError`, which this commit's predecessor added for exactly this and wired into the modal but not here. `zoneSubtreeIds` raising instead of silently truncating at MAX_ZONES was not covered — reverting it left the suite green. Now pinned. 911 tests, lint clean, production build green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile SummaryThe PR adds opt-in, frontend-orchestrated cascade deletion for zones and domains, including preflight plans, progress reporting, and deletion receipts.
Confidence Score: 4/5The stale destructive plan can delete systems or zone trees whose ownership changed after the preview, so that race should be fixed before merging. Cascade eligibility is checked while rendering the preview, but confirmation later runs captured deletion closures without repeating the system-zone and cross-domain sharing checks; the clipboard export also needs formula-safe cell encoding. Files Needing Attention: src/app/common/item.service.ts, src/app/common/cascade-delete.ts, src/app/overlays/confirm-modal.component.ts
|
| Filename | Overview |
|---|---|
| src/app/common/cascade-delete.ts | Implements cascade discovery and sequential deletion, but destructive steps execute plan-time ownership decisions without revalidation. |
| src/app/common/item.service.ts | Integrates the opt-in cascade into item deletion and receipts while retaining the resolved plan across an unbounded confirmation interval. |
| src/app/overlays/confirm-modal.component.ts | Adds asynchronous option details and receipts; TSV export should neutralize formula-leading cells. |
| src/app/common/actions.ts | Registers zone and domain cascade planners with their existing item actions. |
| src/app/mocks/backend/zones.mock.ts | Updates mock parent filtering to match parent_id, comma-list, and root query behavior. |
Sequence Diagram
sequenceDiagram
actor Admin
participant Modal
participant Planner
participant API
Admin->>Modal: Enable cascade
Modal->>Planner: Resolve cascade plan
Planner->>API: Read zones, systems, domains, applications, tenants
API-->>Planner: Current ownership snapshot
Planner-->>Modal: Scope, summary, warnings, captured delete steps
Note over Admin,API: Ownership may change while confirmation remains open
Admin->>Modal: Confirm deletion
Modal->>Planner: Run cached plan
Planner->>API: DELETE captured resources
Planner->>API: DELETE zone/domain
API-->>Modal: Results and partial failures
Modal-->>Admin: Deletion receipt
Reviews (1): Last reviewed commit: "fix(delete): report the actual error whe..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| details: async () => { | ||
| plan = await cascade.plan(item); | ||
| const { scope, summary, warnings } = plan; | ||
| return { scope, summary, warnings }; |
There was a problem hiding this comment.
If system-zone assignments or domain org-zone references change after the preview is resolved, confirmation executes the cached deletion plan without revalidating ownership, causing a system assigned outside the subtree or another domain’s newly shared zone tree to be deleted.
| 'FAILED', | ||
| ]), | ||
| ...(result.skipped || []).map((item) => [ | ||
| item.type, | ||
| item.name, | ||
| item.id, | ||
| 'SKIPPED', | ||
| ]), | ||
| ]; | ||
| return rows.map((row) => row.join('\t')).join('\n'); | ||
| } | ||
|
|
||
| export const CONFIRM_METADATA = { | ||
| height: 'auto', | ||
| }; | ||
|
|
||
| export interface ConfirmRepsonse { |
There was a problem hiding this comment.
A resource name beginning with =, +, -, or @ is copied verbatim into a TSV cell, so pasting the receipt into formula-capable spreadsheet software evaluates the name as a formula instead of literal text.
How this was verified: receiptToTsv directly joins resource names into tab-separated rows, and copyAll copies that output to the clipboard.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Merging this now. Summary of what changed since the last review, for the record. @MrYuion — your three findings were correct, and fixing them was not enough. Before merging I ran an adversarial review over the whole diff (six lenses, each finding then handed to a separate agent whose only job was to refute it). It surfaced 26 substantiated defects, including a fourth instance of the exact fail-open pattern you flagged — The ones that mattered:
Plus the modal fixes ( Every fix is mutation-tested — each one reverted individually, confirming a specific test goes red. That includes the receipt and TSV rendering of skipped rows, which the review showed could be deleted wholesale with the suite still green. A second adversarial pass over the fixed code returned zero blockers (36 findings refuted, 4 non-blocking, of which the two worth having are in the final commit). 911 tests, lint clean, production build green. Happy to walk through any of it, and if you'd rather something came out, say so and I'll revert it. |
2 participants
Closes PPT-1203. Also addresses the behaviour reported in PROJ-845.
The problem
Deleting a zone has never removed the systems inside it.
Zone#destroycascades child zones, trigger instances, metadata, settings and group links, butsys.zonesis a text array with no foreign key — soZone#remove_array_referencesjust strips the id and any system left with no zones is orphaned.The confirmation copy already claimed otherwise:
It never did. That is exactly the complaint in PROJ-845, and
placeos-dev.aca.imcurrently carries 6 systems withzones = {}as a result, plus one pointing at azone-123that does not exist.Tearing down a domain is worse — it is the manual, one-at-a-time process described in PPT-1203, and OAuth applications orphan silently because
oauth_applications.owner_idhas no foreign key back toauthority.What this adds
An opt-in checkbox on the delete confirmation. It is off by default — deleting an item without touching what hangs off it stays the default behaviour, unchanged.
Ticking it resolves a plan first and shows exactly what would go, before anything is confirmed:
Afterwards the dialog becomes a receipt of what actually went — type, name and id, in removal order, ending with the item itself. "Copy list" puts it on the clipboard as TSV for pasting into a ticket.
The rules it applies
Zones — removes systems whose every zone falls inside the subtree, i.e. exactly those that would be left with no zone. A system shared with a zone outside it is kept, as today, and the dialog says so. The backend then takes their modules, triggers, metadata and settings; the zone delete takes the rest.
Domains — removes the domain's OAuth applications and its staff API tenant (matched on domain name, the same rule
staff-api.component.tsalready uses to list them). Zones are only reachable from a domain through theauthority.config.org_zoneconvention.That convention is not exclusive, and this is the important bit. On dev:
Three domains share one org zone and three have none at all. Cascading zones off a domain unconditionally would destroy another customer's estate, so the org zone tree is only included when no other domain references it, and the dialog names the domains that caused it to be skipped:
Drivers and repositories are deliberately out of scope — they are install-wide and shared across domains, and deleting one destroys every module using it everywhere.
Notes for review
Nothing existing changes. The receipt and the cascade only engage when the option is ticked. Users, drivers, modules, triggers, and zones/domains with the box left off all behave exactly as before — there are e2e tests asserting both (
shows no receipt for a delete without the option,leaves systems alone when the option is left off).Destructive reads go to the database, not the index.
GET /systems?zone_id=is Elasticsearch-backed and can lag. Since a stalezonesarray would mean deleting a system that still belongs somewhere, the index only nominates candidates — each is re-read throughshowSystemand re-checked before it makes the list.It is frontend-orchestrated, so a large teardown is N delete requests rather than one. That was a deliberate call: it works against every deployed backend today with no coordinated release, and gives per-item progress and precise partial-failure reporting. A rest-api
DELETE /zones/:id?recursive=truecould replace the inner loop later without changing anything the user sees — worth a follow-up ticket.Partial failures show the same receipt with the failures called out, and the item is deliberately left in place rather than deleted on top of a broken cascade.
Two drive-by fixes, both in
styles.cssand both affecting checkboxes app-wide on the dark theme: the label and the empty box outline were rendering near-black (1.18:1 and 1.21:1) because the app'smat-checkboxrules target.mdc-checkbox__background/.mdc-labeland never win against Material's own selectors, so the colours still came from the prebuilt light palette. Set at the token layer instead. Nothing hit this before because no other<mat-checkbox>in the app has label content.Also here: the zones mock filtered on a
parentquery param the API has never sent (nowparent_id, including comma lists androot), mock zones gained theparent_idhierarchy theirzonesarrays already implied, andconfig/proxy.conf.jsacceptsPLACEOS_DOMAINso the dev server can point at a local stack.Screenshots live in
docs/ppt-1203/(512KB). Happy to move them out of tree if you would rather not carry them.Testing
cascade-delete.spec.ts(24),cascade-locale.spec.ts(14, pins singular/plural against the real locale file), plus 23 on the confirm modal.e2e/src/cascade-delete.spec.ts, 9 tests. Full chromium suite 84 passing, 1 skipped.org > building > 2 levels, 3 enclosed systems + 1 straddling an outside zone, option onA seeded sandbox that reproduces all four is in
tasks/PPT-1203/demo(not part of this branch).