Skip to content

Let nested Factory automations poll, save settings safely, and show investigated Activity - #3383

Open
enzoames wants to merge 6 commits into
mainfrom
enzoames/changes-1028
Open

Let nested Factory automations poll, save settings safely, and show investigated Activity#3383
enzoames wants to merge 6 commits into
mainfrom
enzoames/changes-1028

Conversation

@enzoames

@enzoames enzoames commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Nested Factory Slack, GitHub, and Sentry automations can poll and run, including via the generic run-now path for jobs nested under jobs/.
  • Create and Settings share the same source cards and a sticky Save/Discard bar; Save disables the form and does not overwrite edits made while the request is pending.
  • Activity shows investigated items (held, dispatched, failed) with a collapsed Trace, including failed Slack dispatch from factory_runs.
  • Migration 24 reconciles leftover default-factory rows, clears every extra factory sharing a Slack channel, then creates the unique index through the migration SQL executor.

Test plan

  • Create or open a non-default factory, enable Slack polling, and click Run now on factory-slack-feedback.
  • Confirm poll-slack-channel is no longer rejected with “not invoked by a governed Factory automation.”
  • Change a Settings field and confirm the sticky Save/Discard bar appears; save and confirm the Automations list stays visible while a run is in progress.
  • Switch factories with unsaved Settings edits and confirm Factory B does not inherit Factory A values.
  • Open Activity after a poll: investigated items instead of raw check counts; held items show summaries; a failed Slack dispatch shows the error.
  • Two factories on the same Slack channel: boot should clear the extra assignment instead of failing unique-index creation.

Per-factory jobs live under jobs/factories/<id>/, so Run now and the governed poll check now treat the path as the automation name and the leaf as the role. Create and Settings share source cards with a sticky unsaved bar, and Activity keeps running automations visible under their display names.

Co-authored-by: Cursor <cursoragent@cursor.com>
@enzoames
enzoames requested review from a team, NKoech123 and steve8708 and removed request for a team August 21, 2026 23:26
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

builder-io-integration[bot]

This comment was marked as outdated.

enzoames and others added 2 commits August 21, 2026 19:58
…sions.

Skip rewriting a legacy factory_config id when the scoped row already exists, merge source settings and poll cursors before deleting leftovers, restore the local self-dispatch fallback port to 3000, and share Slack member-id validation on create.

Co-authored-by: Cursor <cursoragent@cursor.com>
@enzoames

Copy link
Copy Markdown
Contributor Author

Addressed all three open Builder review items in ae5170d (also merged origin/main).

  • Required factory_config reconciliation: collision-safe id rewrite in migration 21; merge source settings and poll cursors before deleting leftovers in 25.
  • Required self-dispatch: restored the local fallback port to 3000; still uses inbound Host when present.
  • Optional create-factory Slack member id: same schema as Settings.

No items skipped. PR not merged.

builder-io-integration[bot]

This comment was marked as outdated.

Migration 24 now merges leftover factory_config rows before creating the Slack-channel unique index, and Settings/Automations reset per factory so unsaved values and queued run polling cannot leak across factories.

Co-authored-by: Cursor <cursoragent@cursor.com>
@netlify

This comment has been minimized.

@enzoames

Copy link
Copy Markdown
Contributor Author

Addressed all four open Builder review items in 300059e.

Required — migration 24 reconciles duplicate factory_config rows before creating the Slack-channel unique index; 25 still reconciles for workspaces that already ran the old index-only 24.

Optional — Settings remounts/resets hydration on factory switch; Automations clears queuedRuns on factory switch; Settings save baselines every trimmed field.

No items skipped. Branch is 1 commit behind main (unrelated Clips/Slides sessionStorage fix); no merge conflicts. PR not merged.

builder-io-integration[bot]

This comment was marked as outdated.

…ueness.

Activity lists held, dispatched, and failed items instead of raw checks. Migration 24 clears every duplicate Slack channel and creates the unique index through the migration SQL executor, Settings no longer clobber in-flight edits, and the generic run-now action accepts a nested jobs path.

Co-authored-by: Cursor <cursoragent@cursor.com>
@enzoames enzoames changed the title Let nested Factory automations poll and keep settings easy to save Let nested Factory automations poll, save settings safely, and show investigated Activity Aug 22, 2026
builder-io-integration[bot]

This comment was marked as outdated.

@enzoames

Copy link
Copy Markdown
Contributor Author

Addressed all five open Builder review items in 4237cf9, and landed outcome-first Activity in the same commit.

Required — migration 24 clears every duplicate Slack-channel assignment (not only leftover/product-feedback) before creating the unique index.

Optional — unique-index DDL goes through migration sql; Slack channel+checkpoints merge as one identity; Settings disables controls while Save is pending and does not apply the snapshot over a newer edit; generic run-now accepts a nested path.

Activity now lists investigated items (held/dispatched/failed) with a collapsed Trace, including failed Slack dispatch from factory_runs.

No items skipped. Branch is 1 commit behind main (unrelated Clips/Slides); no merge conflicts. PR not merged.

The audit item-id predicate now returns a boolean so Factory typecheck can pass. Config merge takes slack_workspace from the same row that supplies the channel, so leftover secondary channels are not polled as primary.

Co-authored-by: Cursor <cursoragent@cursor.com>
@enzoames

Copy link
Copy Markdown
Contributor Author

Addressed both open Builder review items in b359136.

RequiredeventItemIds type predicate now returns a boolean (value.length > 0). This was the CI Typecheck failure (TS2322); Factory typecheck passes locally.

Optionalslack_workspace is taken from the same row that supplies the adopted Slack channel, so leftover secondary channels are not polled as primary.

No items skipped. Branch is 6 commits behind main (deploy/docs/desktop/plugin work, merge-tree clean, no Factory overlap); did not merge main. PR not merged.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Incremental Code Review Summary

The latest revision fixes both previously open issues: the audit report’s item-ID predicate now returns an explicit boolean, and Slack workspace selection stays coupled to the adopted channel during reconciliation. I resolved those stale review threads before this review.

The new Activity projection and audit reporting are otherwise well structured, with factory/org scoping, investigated-item aggregation, provider failure visibility, and focused regression coverage. One failure-state gap remains in the new reporting path. This remains high risk because the change touches shared automation execution and operational failure reporting.

New Findings

  • 🟡 MEDIUM — Activity only treats failed as a failed run, hiding persisted timed_out and reconciliation_required outcomes as successful/held activity without the stored error.
  • 🟡 LOW — Collapsed scan traces emit a raw English Loaded … item(s). string instead of localized copy.

The migration ordering/conflict fixes, nested run-now support, and Settings state protections remain covered by the updated tests. 🧪 Browser testing: Will run after this review (PR touches UI code).

Comment on lines +200 to +207
function readDispatchError(
dispatch: FactoryAuditEventRecord | null,
run: FactoryAuditRunSnapshot | undefined,
): string | null {
if (dispatch?.status === "error") {
return dispatch.summary.trim() || run?.error || "Dispatch failed.";
}
if (run?.status === "failed" && run.error) return run.error;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Show all terminal run failures in Activity

triageRuns can persist timed_out and reconciliation_required states with an error, but this projection only emits a dispatch error when status is exactly failed. Activity therefore presents callback/provider failures as successful or held outcomes with no error; treat all error-bearing terminal states as failed, including timed_out and reconciliation_required.

Fix in Builder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant