Public: anyone can read this; the crew can change it.

Absorb: deleting an app leaves its feature rows and settings behind #9643

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

`deleteApp` now deletes the app's rows from `app_feature` and `app_feature_setting` along with `app_vital` and the app itself, all in one transaction (`src/core/apps.ts`). Un-absorbing still keeps the settings; only deleting the app removes them.

shipped production is serving it deploy f8d173e

opened this

deleteApp now deletes the app's rows from app_feature and app_feature_setting along with app_vital and the app itself, all in one transaction (src/core/apps.ts). Un-absorbing still keeps the settings; only deleting the app removes them.

I added a test in test/features.test.ts. It checks that the app has a feature row and settings, calls deleteApp, then checks that neither table holds a row for the app's id. I did not run it without the fix, so I haven't seen it fail.

bun x tsc --noEmit is clean and test/features.test.ts passes (15 tests). I ran bun scripts/gate.ts but only looked at the last lines of its output, which were empty. I didn't run the rest of the suite, which the forge runs on push.

I added no foreign keys or migration. The cleanup is in code, as the issue asked.

🤖 Generated with Claude Code

Closes #9542

commented

deleteApp now deletes the app's rows from app_feature and app_feature_setting along with app_vital and the app itself, all in one transaction (src/core/apps.ts). Un-absorbing still keeps the settings; only deleting the app removes them.

I added a test in test/features.test.ts. It checks that the app has a feature row and settings, calls deleteApp, then checks that neither table holds a row for the app's id. I did not run it without the fix, so I haven't seen it fail.

bun x tsc --noEmit is clean and test/features.test.ts passes (15 tests). I ran bun scripts/gate.ts but only looked at the last lines of its output, which were empty. I didn't run the rest of the suite, which the forge runs on push.

I added no foreign keys or migration. The cleanup is in code, as the issue asked.

🤖 Generated with Claude Code

commented

deleteApp now removes an app's app_feature and app_feature_setting rows along with its vitals and the app itself, all in one transaction. The earlier failure was a bug in a test, not in that change.

Cause of the failing check: in test/apps-measure.test.ts, the "use an index" test ran EXPLAIN QUERY PLAN through db.query(...).all(). That left a cached statement open on the shared connection. The next test in the file calls deleteApp, and its transaction then failed to commit with "SQL statements in progress". Run on its own, that test passed.

Fix: the "use an index" test now prepares the explain statement, reads it, and finalizes it. src/core/apps.ts is unchanged.

Checked here: test/apps-measure.test.ts and test/features.test.ts pass (31 tests), and bun x tsc --noEmit is clean. I did not run scripts/gate.ts or the full suite; the forge's check runs the full suite on this push.

Not done: I didn't change deleteApp to tolerate open statements on the connection. The transaction is meant to be atomic, and an open statement elsewhere is a bug in the code that left it open.

To try it: bun test test/apps-measure.test.ts test/features.test.ts.

Approved — before the latest push

deleteApp now removes app_feature and app_feature_setting rows in one transaction with the app. The test covers both tables. The apps-measure change finalizes the EXPLAIN statement, which the new transaction needs. No security, dependency or both-doors concern: this is a core cleanup with no new behaviour.

commented

I merged main into the branch and pushed it. The only conflict was in test/features.test.ts, and I kept both sides' tests there.

main added two tests at the end of the "absorbing an app" block: one on the declaration cache and one on the menu with 20 more absorbed apps. I kept both. My test, "deleting the app takes its feature row and settings with it", now sits after them. It deletes the app, so putting it last stops it from affecting the others.

test/features.test.ts passes (17 tests) and bun x tsc --noEmit is clean. I did not run scripts/gate.ts or the rest of the suite; the forge's check runs the full suite on this push.

deleteApp now removes an app's app_feature and app_feature_setting rows along with its vitals and the app itself, all in one transaction. The earlier check failure came from a test bug: an unfinalized EXPLAIN statement in test/apps-measure.test.ts. I fixed it by finalizing the statement.

Approved

deleteApp now clears app_feature and app_feature_setting with app_vital and the app in one transaction. It is the only app delete path in src, and the new test would fail without the fix. No security, dependency or both-doors concern; no README change.

Two things I would do next:

  1. Fix the indentation in test/apps-measure.test.ts. The plan helper is now a braced block inside an arrow, with odd nesting; make it a plain function body.
  2. deleteApp now throws if another statement is open on the connection, which is what broke the earlier check. Keep it that way, but consider a test that pins it.
Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs