Added graph editing for automations#27878
Conversation
ref https://linear.app/tryghost/issue/NY-1229/ The automations edit endpoint needs to sync the frontend-authored graph through the fake sqlite repository while preserving backend validation before the MySQL/Bookshelf implementation lands.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR expands automation editing from status-only updates to full graph replacement. The frontend sends an EditAutomationPayload (id, status, actions, edges) via a new useEditAutomation PUT hook. The endpoint forwards the optional-chained payload to automationsApi.edit. The service layer adds Zod schemas and graph validators that parse unknown input, enforce action/edge shapes and a single linear path, and throw ValidationError on invalid payloads. The fake repository replaces action/edge graphs with revision tracking. E2E tests cover positive replacements and many invalid payload scenarios. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
ghost/core/test/e2e-api/admin/automations.test.js (3)
305-758: ⚡ Quick winSignificant scaffolding duplication across the negative-case tests.
Almost every rejection test repeats the same 5-step pattern: browse, read-before, PUT(422 + cacheInvalidateHeaderNotSet), read-after,
deepEqual(after, before). Extracting one helper would shrink each test to its essence (the invalid body) and make it harder to forget the state-preservation assertion.♻️ Sketch of a helper
async function expectEditRejectionPreservesState(agent, automationId, body) { const {body: beforeBody} = await agent .get(`automations/${automationId}`) .expectStatus(200); await agent .put(`automations/${automationId}`) .body(body) .expectStatus(422) .expect(cacheInvalidateHeaderNotSet()); const {body: afterBody} = await agent .get(`automations/${automationId}`) .expectStatus(200); assert.deepEqual(afterBody, beforeBody); return beforeBody; }Then each test becomes roughly:
const {body: browseBody} = await agent.get('automations').expectStatus(200); const automationId = browseBody.automations[0].id; await expectEditRejectionPreservesState(agent, automationId, { automations: [{status: 'inactive', actions: [...], edges: [...]}] });🤖 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 `@ghost/core/test/e2e-api/admin/automations.test.js` around lines 305 - 758, Many negative-case tests duplicate the same browse → read-before → PUT(expect 422 + cacheInvalidateHeaderNotSet) → read-after → assert.deepEqual pattern; extract a helper (e.g. expectEditRejectionPreservesState) to encapsulate that flow. Implement a function named expectEditRejectionPreservesState(agent, automationId, body) that GETs the automation to capture beforeBody, performs the PUT with the provided body and asserts 422 and cacheInvalidateHeaderNotSet, then GETs the automation again and asserts deepEqual(afterBody, beforeBody), returning beforeBody for callers; replace the repeated blocks in each test with a single call to this helper, keeping existing use of agent and cacheInvalidateHeaderNotSet unchanged.
519-520: 💤 Low valueImplicit fixture-shape dependency.
beforeBody.automations[0].actions[0]and[1]here come frombrowseBody.automations[1](the second automation in the fixture). Most other tests in this file pull fromautomations[0](whichmatchAutomation()documents as having 4 actions), butautomations[1]'s action count isn't documented anywhere in this file. If a future fixture change dropsautomations[1]to a single action, this test will start failing withCannot read properties of undefined.A short assert (e.g.
assert.ok(beforeBody.automations[0].actions.length >= 2)) up front would surface a clearer failure if the fixture drifts.🤖 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 `@ghost/core/test/e2e-api/admin/automations.test.js` around lines 519 - 520, The test assumes at least two actions exist but accesses beforeBody.automations[0].actions[0] and [1] without checking; add an upfront assertion (e.g. assert.ok(beforeBody.automations[0].actions.length >= 2)) before creating keptAction and deletedAction to fail clearly if the fixture drifts, and keep using the existing keptAction/deletedAction assignments (references: beforeBody, keptAction, deletedAction).
508-558: 💤 Low valueTest name doesn't match the validation path actually exercised.
The second PUT submits
actions: editedAutomation.actions(which contains onlykeptAction) and an edge whosetarget_action_idisdeletedAction.id— an id that isn't in the submittedactionsarray. That trips the "edges must reference actions in the submitted graph" check invalidateGraph(automations-api.ts line 148-150), not any soft-deleted-specific logic. The same 422 would occur ifdeletedAction.idhad never existed.If the intent is to test that soft-deleted actions truly aren't reusable, consider also asserting the case where
deletedActionis added back intoactions(which should hitloadActionOwnerand return the "conflictingAutomationActionId" message). Otherwise renaming this case to something likerejects an edge whose target is not in the submitted actionswould be more accurate.🤖 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 `@ghost/core/test/e2e-api/admin/automations.test.js` around lines 508 - 558, The test "rejects an edge that references a soft-deleted action" is actually exercising the validateGraph check that edges must reference actions present in the submitted graph (because the second PUT uses actions: editedAutomation.actions which only contains keptAction), so either rename the test to reflect that (e.g., "rejects an edge whose target is not in the submitted actions") or change the second PUT to include deletedAction in the submitted actions so the request reaches the soft-delete path in automations-api.ts (validateGraph -> loadActionOwner) and asserts the "conflictingAutomationActionId" response; locate references to validateGraph, loadActionOwner and the conflictingAutomationActionId message to implement the corresponding change.ghost/core/core/server/services/automations/fake-database-automations-repository.ts (2)
251-301: 💤 Low valueReprepare-on-each-call is fine here, but each
insertActionRevisiondoes two queries per submitted action.
getNextRevisionCreatedAtperforms aMAX(created_at)lookup per action, theninsertActionRevisionissues the insert. For the small N enforced by the linear-graph constraint this is fine, but if revisions become a hot path (or the TODO NY-1283 dedup work lands), consider preparing eachdatabase.prepare(...)once at module/function scope and reusing the statement, or computing the next timestamp from data already in memory.Worth deferring until you move off the fake repository.
🤖 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 `@ghost/core/core/server/services/automations/fake-database-automations-repository.ts` around lines 251 - 301, insertActionRevision calls getNextRevisionCreatedAt which issues a SELECT MAX(created_at) and then insertActionRevision runs an INSERT, causing two DB prepares per action; to fix, prepare and reuse the statements (the SELECT MAX(...) and the INSERT into automation_action_revisions) at module or outer function scope instead of calling database.prepare inside getNextRevisionCreatedAt and insertActionRevision each time, or alternatively compute the next revision timestamp from in-memory data before calling insertActionRevision; update insertActionRevision, getNextRevisionCreatedAt, and any callers to use the cached prepared statements (or pass the in-memory latest timestamp) so only one DB operation is performed per submitted action.
214-222: 💤 Low valueReturn type can be simplified given how the value is consumed.
loadActionOwnerreturnsautomation_id(ornull) but every caller only treats it as truthy. Returning a boolean and usingEXISTSmakes the contract clearer and avoids reading the column. Optional cleanup.🤖 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 `@ghost/core/core/server/services/automations/fake-database-automations-repository.ts` around lines 214 - 222, loadActionOwner currently returns the automation_id or null but callers only treat it as truthy; change its contract to return a boolean by updating the signature of loadActionOwner(DatabaseSync, actionId: string): boolean, replace the SELECT of automation_id with an EXISTS query against automation_actions (e.g. SELECT EXISTS(...) WHERE id = ?), and return the boolean result; also update any callers that expect a string/null to use the boolean return (use the loadActionOwner name to find usages) so we avoid reading the column and make the intent explicit.ghost/core/core/server/services/automations/automations-api.ts (1)
41-49: 💤 Low value
email_lexicalJSON-parse check accepts non-object payloads.
z.string().refine(JSON.parse)accepts any valid JSON literal —"42","null","\"text\"", etc. — even though downstream code (and the test payloads in this PR) assume a lexical document shape with arootobject. A trivially tighter guard is to also require the parsed value to be a plain object. Worth doing only if you want the API surface to reject obviously invalid inputs eagerly; otherwise leave for a follow-up.♻️ Tighter refine
- email_lexical: z.string().refine((value) => { - try { - JSON.parse(value); - return true; - } catch { - return false; - } - }), + email_lexical: z.string().refine((value) => { + try { + const parsed = JSON.parse(value); + return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed); + } catch { + return false; + } + }),🤖 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 `@ghost/core/core/server/services/automations/automations-api.ts` around lines 41 - 49, The current email_lexical z.string().refine only checks that the string is valid JSON but allows primitives; update the refine for email_lexical to parse the JSON and ensure the parsed value is a non-null plain object (e.g., typeof result === 'object' && result !== null && !Array.isArray(result')) and — if you want the stricter guard mentioned in tests — also validate that the object has the expected lexical shape (for example a root property) before returning true; adjust the refine error message accordingly so failures are descriptive.
🤖 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 `@ghost/core/core/server/services/automations/automations-api.ts`:
- Around line 113-126: validateEditData currently maps any Zod validation
failure not tied to 'status' to a generic messages.invalidAutomationPayload,
hiding which field or issue failed; update validateEditData to inspect
result.error.issues and, when failures exist outside 'status', build a clearer
error (e.g., include the top offending issue.path and message or a sanitized
list of issue summaries) and pass that into throwValidationError instead of
messages.invalidAutomationPayload; keep the existing special-case for status
(checking issue.path[0] === 'status'), but for other issues include references
to actions/edges or the specific issue messages from result.error.issues so
callers can see if the failure was an unknown action.type, malformed
email_lexical, or missing edge.target_action_id, then continue to call
validateGraph(result.data.actions, result.data.edges) only after returning the
parsed result.data.
---
Nitpick comments:
In `@ghost/core/core/server/services/automations/automations-api.ts`:
- Around line 41-49: The current email_lexical z.string().refine only checks
that the string is valid JSON but allows primitives; update the refine for
email_lexical to parse the JSON and ensure the parsed value is a non-null plain
object (e.g., typeof result === 'object' && result !== null &&
!Array.isArray(result')) and — if you want the stricter guard mentioned in tests
— also validate that the object has the expected lexical shape (for example a
root property) before returning true; adjust the refine error message
accordingly so failures are descriptive.
In
`@ghost/core/core/server/services/automations/fake-database-automations-repository.ts`:
- Around line 251-301: insertActionRevision calls getNextRevisionCreatedAt which
issues a SELECT MAX(created_at) and then insertActionRevision runs an INSERT,
causing two DB prepares per action; to fix, prepare and reuse the statements
(the SELECT MAX(...) and the INSERT into automation_action_revisions) at module
or outer function scope instead of calling database.prepare inside
getNextRevisionCreatedAt and insertActionRevision each time, or alternatively
compute the next revision timestamp from in-memory data before calling
insertActionRevision; update insertActionRevision, getNextRevisionCreatedAt, and
any callers to use the cached prepared statements (or pass the in-memory latest
timestamp) so only one DB operation is performed per submitted action.
- Around line 214-222: loadActionOwner currently returns the automation_id or
null but callers only treat it as truthy; change its contract to return a
boolean by updating the signature of loadActionOwner(DatabaseSync, actionId:
string): boolean, replace the SELECT of automation_id with an EXISTS query
against automation_actions (e.g. SELECT EXISTS(...) WHERE id = ?), and return
the boolean result; also update any callers that expect a string/null to use the
boolean return (use the loadActionOwner name to find usages) so we avoid reading
the column and make the intent explicit.
In `@ghost/core/test/e2e-api/admin/automations.test.js`:
- Around line 305-758: Many negative-case tests duplicate the same browse →
read-before → PUT(expect 422 + cacheInvalidateHeaderNotSet) → read-after →
assert.deepEqual pattern; extract a helper (e.g.
expectEditRejectionPreservesState) to encapsulate that flow. Implement a
function named expectEditRejectionPreservesState(agent, automationId, body) that
GETs the automation to capture beforeBody, performs the PUT with the provided
body and asserts 422 and cacheInvalidateHeaderNotSet, then GETs the automation
again and asserts deepEqual(afterBody, beforeBody), returning beforeBody for
callers; replace the repeated blocks in each test with a single call to this
helper, keeping existing use of agent and cacheInvalidateHeaderNotSet unchanged.
- Around line 519-520: The test assumes at least two actions exist but accesses
beforeBody.automations[0].actions[0] and [1] without checking; add an upfront
assertion (e.g. assert.ok(beforeBody.automations[0].actions.length >= 2)) before
creating keptAction and deletedAction to fail clearly if the fixture drifts, and
keep using the existing keptAction/deletedAction assignments (references:
beforeBody, keptAction, deletedAction).
- Around line 508-558: The test "rejects an edge that references a soft-deleted
action" is actually exercising the validateGraph check that edges must reference
actions present in the submitted graph (because the second PUT uses actions:
editedAutomation.actions which only contains keptAction), so either rename the
test to reflect that (e.g., "rejects an edge whose target is not in the
submitted actions") or change the second PUT to include deletedAction in the
submitted actions so the request reaches the soft-delete path in
automations-api.ts (validateGraph -> loadActionOwner) and asserts the
"conflictingAutomationActionId" response; locate references to validateGraph,
loadActionOwner and the conflictingAutomationActionId message to implement the
corresponding change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2ff9512-c4fd-45fd-ba2b-fa3036af7c3e
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/automations.test.js.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/admin-x-framework/src/api/automations.tsghost/core/core/server/api/endpoints/automations.jsghost/core/core/server/services/automations/automations-api.tsghost/core/core/server/services/automations/automations-repository.tsghost/core/core/server/services/automations/fake-database-automations-repository.tsghost/core/package.jsonghost/core/test/e2e-api/admin/automations.test.js
ref https://linear.app/tryghost/issue/NY-1229/ The automations e2e tests mutate the temporary fake database while later graph validation tests assume the original fixture graph. Resetting the test database after each case keeps those tests independent.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/automations/automations-api.ts (1)
226-229: ⚡ Quick winClose the cached test database before resetting it.
If
createTemporaryFakeAutomationsDatabase()returns a freshDatabaseSyncper test, settingtestDatabase = nullleaves the sqlite handle alive until GC. Closing it here avoids accumulating open databases across repeated E2E resets.Proposed fix
function _resetTestDatabase() { if (process.env.NODE_ENV?.startsWith('testing')) { + testDatabase?.close(); testDatabase = null; } }🤖 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 `@ghost/core/core/server/services/automations/automations-api.ts` around lines 226 - 229, The _resetTestDatabase function currently just nulls testDatabase, which leaves the sqlite handle open; update _resetTestDatabase to first close the cached testDatabase returned by createTemporaryFakeAutomationsDatabase() (call its close/shutdown method—e.g., testDatabase.close() or await testDatabase.close() if it returns a promise—guarded by a null check) before setting testDatabase = null, and keep this behavior only under the testing environment check.
🤖 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.
Nitpick comments:
In `@ghost/core/core/server/services/automations/automations-api.ts`:
- Around line 226-229: The _resetTestDatabase function currently just nulls
testDatabase, which leaves the sqlite handle open; update _resetTestDatabase to
first close the cached testDatabase returned by
createTemporaryFakeAutomationsDatabase() (call its close/shutdown method—e.g.,
testDatabase.close() or await testDatabase.close() if it returns a
promise—guarded by a null check) before setting testDatabase = null, and keep
this behavior only under the testing environment check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c50edc6b-494b-4229-ae34-2ae5c7c1a7d1
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/automations.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
ghost/core/core/server/services/automations/automations-api.tsghost/core/test/e2e-api/admin/automations.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/e2e-api/admin/automations.test.js
ref https://linear.app/tryghost/issue/NY-1229/ The automations edit endpoint was collapsing non-status schema failures into a generic payload error. Including the Zod path and message keeps invalid action and edge payloads debuggable without changing the public API error shape.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #27878 +/- ##
==========================================
+ Coverage 73.73% 73.78% +0.04%
==========================================
Files 1515 1519 +4
Lines 127273 128104 +831
Branches 15220 15369 +149
==========================================
+ Hits 93850 94525 +675
- Misses 32497 32647 +150
- Partials 926 932 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EvanHahn
deleted the
chris-ny-1229-create-endpoint-for-editing-automations
branch
2 participants
Summary
PUT /automations/:idto accept full graph edits: status, actions, and edgesTesting
pnpm --filter ghost lint:testpnpm --filter ghost lint:typespnpm --filter @tryghost/admin-x-framework lintpnpm --filter @tryghost/admin-x-framework test:typesgit diff --check