fix(triggers): test-event 500, single-click test, dismiss drawer on create#5259
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughBackend trigger testing now handles active subscription conflicts by reusing existing subscriptions and filtering deliveries. Frontend trigger controls support automatic event capture and distinguish create versus edit drawer behavior. ChangesBackend subscription reuse
Frontend trigger capture
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TriggerTest
participant TriggersService
participant TriggersDAO
participant PostgreSQL
participant DeliveryStore
TriggerTest->>TriggersService: test subscription
TriggersService->>TriggersDAO: create subscription
TriggersDAO->>PostgreSQL: persist active subscription
PostgreSQL-->>TriggersDAO: uniqueness conflict
TriggersDAO-->>TriggersService: EntityCreationConflict
TriggersService->>TriggersDAO: fetch existing subscription
TriggersService->>DeliveryStore: poll deliveries after baseline
DeliveryStore-->>TriggersService: newly captured delivery
TriggersService-->>TriggerTest: return delivery
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
dosubot
Bot
added
Bug Report
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/EventSourcePicker.tsx (1)
66-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnhandled promise rejection in
wait()whenonWaitForEventthrows.
wait()has atry/finallywith nocatch, so ifonWaitForEvent()rejects, the error propagates. InhandleOpenChange(Line 86),void wait()discards the promise, turning the rejection into an unhandled promise rejection. The button-click path (Line 95,onClick={wait}) has the same issue since React event handlers don't catch async exceptions.Add a
catchblock to handle errors gracefully:🛡️ Proposed fix
const wait = async () => { if (!onWaitForEvent || waiting) return setWaiting(true) try { const event = await onWaitForEvent() if (event && !settledRef.current) { settledRef.current = true setOpen(false) onPick(event) } + } catch { + // Swallow — callers of onWaitForEvent are expected to surface + // their own error messages (e.g. message.error) before rejecting. } finally { setWaiting(false) } }
🧹 Nitpick comments (1)
api/oss/src/core/triggers/service.py (1)
1196-1208: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
trigger_idfrom the conflict for a precise, safe lookup.
EntityCreationConflictalready carriestrigger_idin itsconflictdict (DAO line 91). Usingget_project_and_subscription_by_trigger_id— which filters bydeleted_at.is_(None)— is more precise than the currentconnection_id+event_keyquery and avoids the soft-delete risk flagged above.♻️ Suggested refactor: trigger_id-based reuse lookup
except EntityCreationConflict as conflict: - existing = await self.dao.query_subscriptions( - project_id=project_id, - subscription=TriggerSubscriptionQuery( - connection_id=subscription.connection_id, - event_key=subscription.data.event_key, - ), - windowing=Windowing(limit=1), - ) - if not existing: + trigger_id = ( + conflict.conflict.get("trigger_id") + if conflict.conflict + else None + ) + found = None + if trigger_id: + result = ( + await self.dao.get_project_and_subscription_by_trigger_id( + trigger_id=trigger_id, + ) + ) + if result: + found = result[1] + if found is None: raise - created = existing[0] + created = found owns_created = False
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7116b35a-3fbd-476d-ac0c-5a61e42d4a26
📒 Files selected for processing (7)
api/oss/src/core/triggers/service.pyapi/oss/src/dbs/postgres/triggers/dao.pyapi/oss/tests/pytest/unit/triggers/test_triggers_subscription_test_mode.pyweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/TriggerManagementSection.tsxweb/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/shared/EventSourcePicker.tsx
…tion and late resolve
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/oss/src/dbs/postgres/triggers/dao.py (1)
75-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClassify the uniqueness violation from the underlying constraint metadata. Matching
str(e.orig)is brittle here and can let duplicate active subscriptions fall back to a generic 500. Re-raise unrelatedIntegrityErrors unchanged.
🧹 Nitpick comments (1)
api/oss/src/dbs/postgres/triggers/dao.py (1)
75-93: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd persistence-level regression coverage for the new contract.
The current service test mocks both DAO methods, so it will not catch incorrect index detection, cross-project matches, or deleted subscriptions being returned. Add tests covering active lookup, deleted/cross-project exclusion, and re-raising unrelated
IntegrityErrorinstances.Also applies to: 234-258
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ddb3a045-fd09-4ea5-a915-faaa3adce272
📒 Files selected for processing (5)
api/oss/src/core/triggers/interfaces.pyapi/oss/src/core/triggers/service.pyapi/oss/src/dbs/postgres/triggers/dao.pyapi/oss/tests/pytest/unit/triggers/test_triggers_subscription_test_mode.pyweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/EventSourcePicker.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- api/oss/tests/pytest/unit/triggers/test_triggers_subscription_test_mode.py
- web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/EventSourcePicker.tsx
- api/oss/src/core/triggers/service.py
…, not message text
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Context
Three trigger issues. Testing an app trigger failed with "an unexpected error occurred" whenever a live subscription already covered the same connection and event. The provider upsert returns the same trigger id, the partial-unique index rejects a second active row, and that database error was flattened to a generic 500. The test UI also took two clicks (open the popover, then "Wait for a new event"). And creating a trigger left the drawer open.
Changes
Backend: the DAO translates the unique-index violation into a typed conflict, so the API returns a clean 409 instead of a 500.
test_subscriptiondetects an existing live subscription for the same connection and event and captures against it instead of minting a duplicate. A freshly minted test subscription is still created and torn down as before.Frontend: the app-trigger test flask starts the wait on a single click (opt-in
autoWaitOnOpenon the shared picker; the other pickers keep their two-step flow). Creating a trigger, subscription or schedule, now dismisses the drawer. The triggers list refreshes from the create mutation's own cache invalidation.Tests / notes
What to QA