[chore] Split the trigger drawers and management section - #5571
[chore] Split the trigger drawers and management section#5571ardaerzin wants to merge 4 commits into
Conversation
Two drawer roots and the management section become composition roots plus co-located modules. The existing shared primitives (MasterDetailRail, useDraftMasterDetail, DrawerFooter, RunVersionField, EventSourcePicker, SectionRail/RailField, ActiveToggle) were already used correctly, so every extracted module keeps importing them — no new overlapping abstraction. TriggerSubscriptionDrawer.tsx 2089 -> 69 + drawers/subscription/ (11) TriggerScheduleDrawer.tsx 1153 -> 53 + drawers/schedule/ (7) TriggerManagementSection.tsx 1040 -> 377 + triggerManagement/ (6) Two genuine cross-drawer duplicates move to drawers/shared/: RequiredTitle and normalizeJson. Three near-duplicates deliberately stay per-drawer: MAX_DRAFTS/SHOW_LIST_RAIL (same values, different documenting comments), applicationRevisionAdapter (a module singleton — merging would change adapter identity), and extractBoundRevId (the schedule drawer writes the same fallback inline; replacing it would be a rewrite, not a move). Split against current content so #5552 (unblock editing schedule and subscription triggers) survives: extractBoundWorkflowId/isRunVersionBound, extractDefaultBindId, the storedReferences/versionChosen derivation and the hidden Deployed option all land in the module that now owns them. Verified line-by-line against the upstream diff. Public surface unchanged: both drawers keep their default export, and useAgentTriggers / AddTriggerDropdown are re-exported from TriggerManagementSection.tsx.
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
Frontend
refactoring
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Railway Preview Environment
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/AppTriggerProviderGroups.tsx (1)
108-114: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse a connections map instead of a linear
findper row.
providerGroupsalready buildsconnById; hoisting it to its own memo letsconnectionLabelbe O(1) and drops the duplicate map construction.♻️ Suggested change
+ const connById = useMemo(() => new Map(connections.map((c) => [c.id, c] as const)), [ + connections, + ]) const connectionLabel = useCallback( (connectionId?: string) => { - const c = connections.find((conn) => conn.id === connectionId) + const c = connectionId ? connById.get(connectionId) : undefined return c ? c.name || c.slug || c.integration_key : undefined }, - [connections], + [connById], )web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/useAgentTriggers.ts (1)
43-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
workflowMolecule.selectors.artifactNamefor the bound workflow label.
defaultBoundLabelis user-facing, but deriving it fromrevision.nameconflicts with the codebase’s explicit artifact name selector and the “use workflow artifact names for workflow display labels” guideline. Use the workflow molecule’sartifactNameselector here so the drawer binds to the normalized artifact name instead of a raw revision name.Source: Coding guidelines
web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/PillEditor.tsx (1)
86-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing dependency array — this effect re-runs on every render.
insertApi.currentonly needs reassigning wheninsertApi,makePill, orcommitchange.♻️ Proposed change
- }) + }, [insertApi, makePill, commit])web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleDrawerContent.tsx (1)
34-96: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the schedules list fetch when the drawer is not in playground mode.
useTriggerSchedules()is called unconditionally, but the settings path only renders a singleScheduleFormand never needs the full list or draft master-detail state. Add a caller-controlledenabled: !!playgroundEntityIdoption (or equivalent disabled atom) touseTriggerSchedules()so settings-mode create/edit drawer opens avoid fetching the full schedules list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be6b10ad-dd29-4c71-afbd-e6b9bd5769e1
📒 Files selected for processing (29)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/TriggerManagementSection.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/AddTriggerDropdown.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/AppTriggerProviderGroups.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/SubscriptionChildRow.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/SubscriptionRunPopover.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/TriggerRow.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/triggerManagement/useAgentTriggers.tsweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerScheduleDrawer.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerSubscriptionDrawer.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/MessageComposer.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/RunInPlaygroundButton.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleDrawerContent.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleForm.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/SchedulesList.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/WindowField.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/constants.tsweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/RequiredTitle.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/normalizeJson.tsweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/InputsMappingField.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/MappingSection.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/PillEditor.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/RunSubscriptionButton.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/SourceBrowsePage.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/SourceField.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/SubscriptionDrawerContent.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/SubscriptionForm.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/SubscriptionsList.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/constants.tsweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/subscription/helpers.ts
| useEffect(() => { | ||
| if (!isEdit || !schedule) return | ||
| setName(schedule.name ?? "") | ||
| setCron(schedule.data?.schedule ?? DEFAULT_CRON) | ||
| setStartTime(schedule.data?.start_time ?? null) | ||
| setEndTime(schedule.data?.end_time ?? null) | ||
| setEnabled(isEntityActive(schedule)) | ||
| const refs = schedule.data?.references | ||
| const envRef = refs?.environment | ||
| if (envRef) { | ||
| setBindMode("environment") | ||
| setEnvironmentSlug(envRef.slug ?? null) | ||
| setAppSlug(refs?.application?.slug ?? null) | ||
| } else { | ||
| setWorkflowRevId(extractBoundWorkflowId(refs)) | ||
| // Label is resolved from the revision id below, not stored as the raw id. | ||
| } | ||
| setInputsText(JSON.stringify(schedule.data?.inputs_fields ?? {}, null, 2)) | ||
| }, [isEdit, schedule]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate ScheduleForm and schedule hook references =="
fd -a 'ScheduleForm\.tsx$|Schedule\.tsx$|.*schedule.*\.tsx$' web/packages/agenta-entity-ui 2>/dev/null | sed 's#^\./##'
echo
echo "== Repository files containing useTriggerSchedule/trigger schedule =="
rg -n "useTriggerSchedule|TriggerSchedule|scheduleId|refetchOnWindowFocus|staleTime|queryOptions" web -S --glob '*.ts' --glob '*.tsx' | head -200Repository: Agenta-AI/agenta
Length of output: 27597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ScheduleForm relevant sections =="
sed -n '1,120p' web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleForm.tsx
sed -n '130,210p' web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleForm.tsx
sed -n '210,430p' web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleForm.tsx
echo
echo "== Trigger schedule query files =="
rg -n "useTriggerSchedule|queryTriggerSchedule|fetchTriggerSchedule|refetch|staleTime|enabled|schedules|TriggerSchedule" web/packages/agenta-entities/src/gatewayTrigger web/packages/agenta-sdk -S --glob '*.ts' --glob '*.tsx'
echo
echo "== Candidate query hook files =="
fd -a '\.ts$' web/packages/agento-entities web/packages/agenta-sdk 2>/dev/null | rg 'gatewayTrigger|trigger|schedule|hooks' | sed -n '1,120p'
fd -a '\.ts$' web/packages/agenta-sdk/src web/packages/agenta-sdk 2>/dev/null | sed -n '1,160p'Repository: Agenta-AI/agenta
Length of output: 35970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Exact hook implementation =="
sed -n '1,110p' web/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerSchedule.ts
echo
echo "== Trigger schedule schemas and response fields =="
rg -n "triggerScheduleSchema|triggerScheduleResponseSchema|updated_at|updated|created_at|fetched|schedule_id|TriggerScheduleResponse" web/packages/agenta-entities/src/gatewayTrigger web/packages/agenta-api-client/src/generated -S
echo
echo "== Trigger schedule schema source =="
fd -a '\.ts$' web/packages/agenta-entities/src/gatewayTrigger | xargs rg -n "triggerScheduleSchema|triggerResponse|TriggerScheduleResponse" -S
sed -n '1,140p' web/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerSchedule.tsRepository: Agenta-AI/agenta
Length of output: 50372
Guard the edit-mode prefill to run at most once per schedule id.
useTriggerSchedule disables window-focus refetch and uses a 30s stale time, but save/other mutations can still make this query stale and refetch in the background. Since this effect’s dependency array is [isEdit, schedule], any fresh schedule object will reset inputsText and the other form fields the user is currently editing.
🛡️ Proposed fix: prefill once per schedule id
+ const prefilledIdRef = useRef<string | undefined>(undefined)
useEffect(() => {
if (!isEdit || !schedule) return
+ if (prefilledIdRef.current === schedule.id) return
+ prefilledIdRef.current = schedule.id ?? undefined
setName(schedule.name ?? "")
setCron(schedule.data?.schedule ?? DEFAULT_CRON)
...
}, [isEdit, schedule])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!isEdit || !schedule) return | |
| setName(schedule.name ?? "") | |
| setCron(schedule.data?.schedule ?? DEFAULT_CRON) | |
| setStartTime(schedule.data?.start_time ?? null) | |
| setEndTime(schedule.data?.end_time ?? null) | |
| setEnabled(isEntityActive(schedule)) | |
| const refs = schedule.data?.references | |
| const envRef = refs?.environment | |
| if (envRef) { | |
| setBindMode("environment") | |
| setEnvironmentSlug(envRef.slug ?? null) | |
| setAppSlug(refs?.application?.slug ?? null) | |
| } else { | |
| setWorkflowRevId(extractBoundWorkflowId(refs)) | |
| // Label is resolved from the revision id below, not stored as the raw id. | |
| } | |
| setInputsText(JSON.stringify(schedule.data?.inputs_fields ?? {}, null, 2)) | |
| }, [isEdit, schedule]) | |
| const prefilledIdRef = useRef<string | undefined>(undefined) | |
| useEffect(() => { | |
| if (!isEdit || !schedule) return | |
| if (prefilledIdRef.current === schedule.id) return | |
| prefilledIdRef.current = schedule.id ?? undefined | |
| setName(schedule.name ?? "") | |
| setCron(schedule.data?.schedule ?? DEFAULT_CRON) | |
| setStartTime(schedule.data?.start_time ?? null) | |
| setEndTime(schedule.data?.end_time ?? null) | |
| setEnabled(isEntityActive(schedule)) | |
| const refs = schedule.data?.references | |
| const envRef = refs?.environment | |
| if (envRef) { | |
| setBindMode("environment") | |
| setEnvironmentSlug(envRef.slug ?? null) | |
| setAppSlug(refs?.application?.slug ?? null) | |
| } else { | |
| setWorkflowRevId(extractBoundWorkflowId(refs)) | |
| // Label is resolved from the revision id below, not stored as the raw id. | |
| } | |
| setInputsText(JSON.stringify(schedule.data?.inputs_fields ?? {}, null, 2)) | |
| }, [isEdit, schedule]) |
| useEffect(() => { | ||
| onErrorChange(parseError) | ||
| }, [parseError, onErrorChange]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Duplicate error ownership: parent overwrites this field's message.
In the non-agent path, MappingSection also validates value and calls onErrorChange (MappingSection.tsx Lines 85-96). Child effects flush before the parent's, so the parent wins: the detailed parser message is replaced by "Invalid JSON", and "Mapping must be a JSON object or a selector string" is cleared to null (parent's JSON.parse succeeds for arrays/strings). Let one owner validate — e.g. skip the parent's JSON effect when InputsMappingField is rendered.
| return ( | ||
| <div className="relative"> | ||
| <div | ||
| ref={ref} | ||
| contentEditable | ||
| suppressContentEditableWarning | ||
| role="textbox" | ||
| aria-multiline="true" | ||
| onInput={commit} | ||
| onKeyDown={onKeyDown} | ||
| className="box-border max-h-[280px] min-h-[120px] w-full overflow-y-auto whitespace-pre-wrap break-words rounded-lg border border-solid border-[var(--ag-colorBorder)] bg-[var(--ag-colorBgContainer)] px-3 py-2 text-xs leading-relaxed outline-none focus:border-[var(--ag-colorPrimary)]" | ||
| /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Force plain-text paste into the contenteditable.
Without an onPaste handler, pasted rich HTML is inserted verbatim into the editor: serialize then produces junk (nested elements collapse to "\n" + …), and pasted markup such as <img onerror=...> executes in-page (self-XSS).
🛡️ Proposed fix
+ const onPaste = useCallback(
+ (e: React.ClipboardEvent) => {
+ e.preventDefault()
+ const text = e.clipboardData.getData("text/plain")
+ document.execCommand("insertText", false, text)
+ commit()
+ },
+ [commit],
+ )
+
return (
<div className="relative">
<div
ref={ref}
contentEditable
suppressContentEditableWarning
role="textbox"
aria-multiline="true"
onInput={commit}
+ onPaste={onPaste}
onKeyDown={onKeyDown}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div className="relative"> | |
| <div | |
| ref={ref} | |
| contentEditable | |
| suppressContentEditableWarning | |
| role="textbox" | |
| aria-multiline="true" | |
| onInput={commit} | |
| onKeyDown={onKeyDown} | |
| className="box-border max-h-[280px] min-h-[120px] w-full overflow-y-auto whitespace-pre-wrap break-words rounded-lg border border-solid border-[var(--ag-colorBorder)] bg-[var(--ag-colorBgContainer)] px-3 py-2 text-xs leading-relaxed outline-none focus:border-[var(--ag-colorPrimary)]" | |
| /> | |
| const onPaste = useCallback( | |
| (e: React.ClipboardEvent) => { | |
| e.preventDefault() | |
| const text = e.clipboardData.getData("text/plain") | |
| document.execCommand("insertText", false, text) | |
| commit() | |
| }, | |
| [commit], | |
| ) | |
| return ( | |
| <div className="relative"> | |
| <div | |
| ref={ref} | |
| contentEditable | |
| suppressContentEditableWarning | |
| role="textbox" | |
| aria-multiline="true" | |
| onInput={commit} | |
| onPaste={onPaste} | |
| onKeyDown={onKeyDown} | |
| className="box-border max-h-[280px] min-h-[120px] w-full overflow-y-auto whitespace-pre-wrap break-words rounded-lg border border-solid border-[var(--ag-colorBorder)] bg-[var(--ag-colorBgContainer)] px-3 py-2 text-xs leading-relaxed outline-none focus:border-[var(--ag-colorPrimary)]" | |
| /> |
| const isDirty = useMemo( | ||
| () => | ||
| baselineSnapshot !== | ||
| JSON.stringify({ | ||
| name, | ||
| connectionId: connectionId ?? null, | ||
| eventKey, | ||
| enabled, | ||
| bindMode, | ||
| environmentSlug: environmentSlug ?? null, | ||
| workflowRevId: workflowRevId ?? null, | ||
| inputs: normalizeJson(inputsText), | ||
| }), | ||
| [ | ||
| baselineSnapshot, | ||
| name, | ||
| connectionId, | ||
| eventKey, | ||
| enabled, | ||
| bindMode, | ||
| environmentSlug, | ||
| workflowRevId, | ||
| inputsText, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Create may be unsaveable when the source is pre-seeded.
canSave={isDirty && !alreadySubscribed} and the create-mode baseline already includes state?.connectionId, state?.eventKey and the default binding. When the drawer opens with a preselected connection/event (add-menu flow), the snapshot matches the initial state, so isDirty is false and Create stays disabled until the user edits an unrelated field. Consider canSave={(!isEdit || isDirty) && !alreadySubscribed} (create mode already validates required fields in buildData).
🐛 Proposed fix
- canSave={isDirty && !alreadySubscribed}
+ canSave={(!isEdit || isDirty) && !alreadySubscribed}Also applies to: 761-761
|
@coderabbitai review |
✅ Action performedReview finished.
|
Four defects the split surfaced, all pre-existing: - Create was unreachable when the source is pre-seeded. The catalog opens the subscription drawer with connection + event + default binding, which is exactly the create baseline, so `isDirty` stayed false and Create never enabled. Create now gates on completeness (source + version chosen); Save still gates on dirty. Same change in the schedule form, which had the identical defect. - The mapping field had two error owners. `MappingSection`'s JSON effect flushes after `InputsMappingField`'s, so it replaced the parser's message with "Invalid JSON" and cleared "Mapping must be a JSON object or a selector string" outright (its `JSON.parse` succeeds for arrays). The parent now defers to the child on the non-agent path. - Edit-mode prefill reset the form on refetch. Every trigger mutation invalidates the detail query by prefix, so a background refetch re-ran the prefill and overwrote edits in progress. Both forms now hydrate once per loaded id; the subscription's `trigger_config` prefill folds into that same hydration. - The pill composer accepted rich paste. Pasted markup landed in the contenteditable verbatim, where `serialize` collapses nested elements to "\n". Paste is now plain text inserted at the caret; the Enter handler reuses it. Plus: bind the default label to the workflow artifact name instead of `revision.name`, hoist the settings/playground split to the drawer roots so settings stops mounting the list query and master-detail state, and hoist the connections map in the provider groups.
1 participant
Context
Three files in the agent triggers feature had grown past the point where anyone could scan them:
TriggerSubscriptionDrawer.tsxat 2089 lines,TriggerScheduleDrawer.tsxat 1153, andTriggerManagementSection.tsxat 1040. Together they held 29 inline components.Changes
Each file becomes a composition root plus co-located modules.
The area already had a good set of shared primitives (
MasterDetailRail,useDraftMasterDetail,DrawerFooter,RunVersionField,EventSourcePicker,SectionRail,RailField,ActiveToggle) and all three files were already consuming them correctly. There were no duplicated re-implementations to collapse, so every extracted module keeps importing the same primitives. No new overlapping abstraction was added.Two genuine cross-drawer duplicates moved to
drawers/shared/:RequiredTitleandnormalizeJson.Three near-duplicates deliberately stayed per-drawer, because sharing them would cost more than it saves:
MAX_DRAFTSandSHOW_LIST_RAILhold the same values but carry different documenting comments. Merging them would delete one.applicationRevisionAdapteris a module-level singleton. Merging the two would change adapter identity from two instances to one, which is a behaviour change with no payoff.extractBoundRevIdexists in the subscription drawer, while the schedule drawer writes the same three-key fallback inline. Replacing the inline version would be a rewrite, not a move.Public surface is unchanged. Both drawers keep their default export, and
useAgentTriggersandAddTriggerDropdownare re-exported fromTriggerManagementSection.tsx, so no importer changed.Tests
pnpm --filter @agenta/entity-ui exec tsc --noEmitpasses, as do@agenta/ossand@agenta/ee.pnpm lint-fixacross the repo makes no changes. Prettier clean.release/v0.106.1rather than rebased. [fix] Unblock editing schedule and subscription triggers #5552 changed the binding logic inside the regions this refactor deletes, so a naive replay would have dropped that fix. Verified line by line: of 86 lines [fix] Unblock editing schedule and subscription triggers #5552 added, 84 are present verbatim. The two that differ are the same import closer, which correctly became"../shared/RunVersionField"one directory deeper.extractBoundWorkflowId,isRunVersionBound,extractDefaultBindId, thestoredReferences/versionChosenderivation andhideEnvironmentall land in the module that now owns them.Not runtime verified. The gates above are static.
What to QA
subscription/constants.tsand are read by two modules.