CS-10630: Reimplement boxel realm remove command#4655
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 845a5fc7fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
so this seems very subtle--its doesn't actually remove the realm, it just unlinks the realm from the user's matrix data. if the user tries to make a new realm with the same name they will get an error that this realm already exists--which is probably confusing since they thought they removed it. is it really the intent that we are just "unlinking" the realm here instead of removing it? if so should we name it something that makes that more clear? |
There was a problem hiding this comment.
Pull request overview
This PR ports the standalone CLI “remove realm” behavior into the monorepo as boxel realm remove <realm-url>, implementing a soft remove that updates the active profile’s app.boxel.realms Matrix account_data list (without deleting server-side realm files).
Changes:
- Added a new
boxel realm removecommand with--dry-runand--yessupport plus a programmaticremoveRealm()API. - Introduced
ProfileManager.removeFromUserRealms()andremoveRealmFromMatrixAccountData()to mirror existing “add realm” helpers. - Added an integration test suite covering removal, dry-run behavior, normalization, and “soft remove” semantics.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/boxel-cli/tests/integration/realm-remove.test.ts | New integration coverage for removing realms from the user list and verifying server realms remain accessible. |
| packages/boxel-cli/src/lib/profile-manager.ts | Adds removeFromUserRealms() wrapper to update Matrix account_data via the auth helper. |
| packages/boxel-cli/src/lib/auth.ts | Adds removeRealmFromMatrixAccountData() to remove a realm URL from app.boxel.realms. |
| packages/boxel-cli/src/commands/realm/remove.ts | New command implementation + programmatic removeRealm() API with confirmation/dry-run flows. |
| packages/boxel-cli/src/commands/realm/index.ts | Registers the new remove subcommand under boxel realm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
That's exactly the issue. The first pass ported the soft-only behavior verbatim from the standalone CLI, which predates the monorepo's Fixed in 8151e40:
|
Addresses Codex P1 + Copilot review feedback on PR #4655. Before: `removeRealmFromMatrixAccountData()` did raw `includes` / `filter` on the stored list, while `removeRealm()` normalized its input via `ensureTrailingSlash`. If `app.boxel.realms` held a legacy entry like `https://host/realm` (no trailing slash) and the user invoked `boxel realm remove https://host/realm/`, the precheck in remove.ts saw a normalized match and proceeded, but the auth helper found no exact match and returned `false`. With the new hard-delete flow this manifested as: server files gone, Matrix list still references the (now-orphan) URL, and the CLI printed "Removed" with no warning. - `removeRealmFromMatrixAccountData()` now compares stored entries via `ensureTrailingSlash` and drops every match, so duplicates like `realm` + `realm/` are both cleaned out in one PUT. - `removeRealm()` computes `nextCount` from the actual normalized match count instead of `previousCount - 1`, and treats a `false` return after a passing precheck as a hard error (concurrent edit race) rather than silently reporting success. - New integration test seeds account_data with both URL shapes for the same realm and asserts both are removed in a single call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Soft-removes a realm URL from the active profile's `app.boxel.realms` Matrix account_data list. Server-side files are untouched. - New `removeRealm()` programmatic API in commands/realm/remove.ts (returns a result object on every code path; never prompts, never calls process.exit). - CLI wrapper with TTY confirmation, `-y/--yes` to skip, and `--dry-run` to preview the change. - `removeRealmFromMatrixAccountData()` helper in lib/auth.ts and `ProfileManager.removeFromUserRealms()` for reuse by other commands. - Integration tests against a real test realm server cover: happy path, not-in-list, dry-run, trailing-slash normalization, soft- remove (server files preserved), and no-active-profile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Hassan's PR feedback that the original soft-only behavior was misleading: a user who "removed" a realm couldn't recreate one with the same name, because server files / index / registry rows were untouched. - `removeRealm()` now calls `DELETE /_delete-realm` first, then unlinks the URL from `app.boxel.realms`. Mirrors the host UI's workspace delete flow (workspace.gts:999-1005) and inverts the `boxel realm create` -> `/_create-realm` symmetry. - 403 from the server (caller does not own the realm) surfaces a clear "you do not own this realm" error; no silent fallback to soft-unlink. - `RemoveRealmResult` extended with `serverDeleted` / `unlinked` so the CLI can report the rare partial-success state (server gone, Matrix list stale). - Integration tests restructured around `createRealm()` so realms are properly registered + namespaced for the namespace check in `_delete-realm`. Adds: name-recyclable-after-removal (the exact Hassan concern), dry-run-does-not-hit-server, and 403-not-owner using a second Synapse user. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Codex P1 + Copilot review feedback on PR #4655. Before: `removeRealmFromMatrixAccountData()` did raw `includes` / `filter` on the stored list, while `removeRealm()` normalized its input via `ensureTrailingSlash`. If `app.boxel.realms` held a legacy entry like `https://host/realm` (no trailing slash) and the user invoked `boxel realm remove https://host/realm/`, the precheck in remove.ts saw a normalized match and proceeded, but the auth helper found no exact match and returned `false`. With the new hard-delete flow this manifested as: server files gone, Matrix list still references the (now-orphan) URL, and the CLI printed "Removed" with no warning. - `removeRealmFromMatrixAccountData()` now compares stored entries via `ensureTrailingSlash` and drops every match, so duplicates like `realm` + `realm/` are both cleaned out in one PUT. - `removeRealm()` computes `nextCount` from the actual normalized match count instead of `previousCount - 1`, and treats a `false` return after a passing precheck as a hard error (concurrent edit race) rather than silently reporting success. - New integration test seeds account_data with both URL shapes for the same realm and asserts both are removed in a single call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FadhlanR
force-pushed
the
cs-10630-reimplement-boxel-realm-remove-command
branch
from
16a4ad6 to
65268ad
Compare
Summary
boxel removefrom the standalonecardstack/boxel-cliintopackages/boxel-cli/src/commands/realm/remove.tsasboxel realm remove <realm-url>.DELETE /_delete-realm(filesystem + indexer + registry), then unlinks the URL from the active profile'sapp.boxel.realmsMatrixaccount_datalist. Mirrors the host UI's workspace delete flow (packages/host/app/components/operator-mode/workspace-chooser/workspace.gts:999-1005) and inverts the existingboxel realm create→/_create-realmpattern.boxel realm create <same-name>now succeeds.Why hard delete
The first iteration of this PR was soft-only (Matrix unlink only). Hassan flagged that as misleading — the user thinks they removed it, but server files / index rows / registry entry remain, and recreating with the same name fails with "already exists".
boxel realm removewas the asymmetric outlier next to the host UI andrealm create. Now it matches.Safety
-y, --yes.--yeserrors out instead of deleting silently.--dry-runpreviewspreviousCount → nextCountwithout sending either request.removeRealm()returns a structured result on every path (neverprocess.exits), withserverDeleted/unlinkedflags so callers can detect the rare partial-success state (server gone, Matrix list still references the URL).Linear
CS-10630 — part of Incorporate Boxel CLI to Monorepo.
Test plan
pnpm --filter @cardstack/boxel-cli lint:typespnpm --filter @cardstack/boxel-cli lint:jspnpm --filter @cardstack/boxel-cli exec vitest run tests/integration/realm-remove.test.ts(7 tests pass: hard-delete, name-recyclable, notInList, dry-run-server-silent, trailing-slash, 403-not-owner, no-active-profile). Requires test-PG (port 55436) and Synapse running locally with realm users registered.boxel realm remove <url>prompts y/N, deletes ony, cancels onN.boxel realm remove <url> --yesskips the prompt.boxel realm remove <url> < /dev/nullerrors out (non-TTY without--yes).boxel realm remove <url> --dry-runprints the count delta and exits without writing.boxel realm create <same-name> "Same Name"succeeds (no "already exists" error).boxel realm remove <url-of-realm-you-do-not-own>fails with a clear 403 message and exit code 1.🤖 Generated with Claude Code