feat(frontend): One reader for a trigger's bound agent and honest cron names - #5766
feat(frontend): One reader for a trigger's bound agent and honest cron names#5766ardaerzin wants to merge 1 commit into
Conversation
dosubot
Bot
added
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ 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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add shared workflow binding resolution and nullable delivery session data. The schedule UI uses the shared resolver. Cron descriptions now cover additional hourly and weekday formats, with tests for the new descriptions. ChangesGateway trigger updates
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 (2)
web/packages/agenta-entities/src/gatewayTrigger/core/types.ts (1)
466-475: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused tests for binding precedence.
triggerBoundAgentIdnow defines the shared precedence used by the schedule table. Add tests for each reference type, precedence when multiple IDs exist, null IDs, and null or undefined references. This protects the contract when another trigger surface adopts the helper.web/packages/agenta-entities/tests/unit/gatewayTriggerCron.test.ts (1)
119-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for weekday restrictions and lists.
The stepped-hour test uses an unrestricted weekday field, so it does not catch the missing
dowguard inweb/packages/agenta-entities/src/gatewayTrigger/core/cron.tsLine 104. The weekday test covers1-5, but not comma-separated values. Add both cases.Proposed tests
it("describes an hour step", () => { expect(describeCron("30 */2 * * *")).toBe("Every 2 hours at :30 (UTC)") + expect(describeCron("30 */2 * * 1")).toBe("30 */2 * * 1 (UTC)") }) it("describes a day range", () => { expect(describeCron("0 9 * * 1-5")).toBe( "Monday, Tuesday, Wednesday, Thursday, Friday at 09:00 UTC", ) + expect(describeCron("0 9 * * 1,3")).toBe("Monday, Wednesday at 09:00 UTC") })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f97fabe3-7b25-422a-8964-e4f2537f16a4
📒 Files selected for processing (5)
web/oss/src/components/pages/settings/Triggers/components/GatewaySchedulesSection.tsxweb/packages/agenta-entities/src/gatewayTrigger/core/cron.tsweb/packages/agenta-entities/src/gatewayTrigger/core/types.tsweb/packages/agenta-entities/src/gatewayTrigger/index.tsweb/packages/agenta-entities/tests/unit/gatewayTriggerCron.test.ts
| // "5 * * * *" — every hour, on the 5th minute. Left undescribed it fell through to the raw | ||
| // expression, which is what a schedule list shows as its own name. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the explanatory comment to one short line.
Line 98 and Line 99 use two long comment lines for this branch. Replace them with one concise line, or remove the comment if the condition is clear.
As per coding guidelines: “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
Railway Preview Environment
|
|
@coderabbitai review |
|
…t cron names triggerBoundAgentId reads the bound workflow out of a trigger's references in one place, and the cron describer names a schedule by what it does. GatewaySchedulesSection adopts both.
5886859 to
167dc42
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai review |
|
|
Landed in |
2 participants
Context
Several surfaces need to answer two questions about a gateway trigger: which agent does it run, and when does it fire next. The settings schedules table derived both from raw trigger references inline, and the upcoming agent Home and overview surfaces were about to grow their own copies.
Changes
triggerBoundAgentId(references)reads the bound workflow id out of a trigger's references in one place, and the cron describer now names a schedule by what it does instead of repeating its cadence.GatewaySchedulesSectionadopts both, so the settings table and the new agent surfaces resolve a trigger's agent the same way.Tests / notes
gatewayTriggerCronunit tests cover the new naming.mainandfeat/mobile-parity-and-consolidation, so this lands onmainindependently. The sessions/agents UX stack (based onfeat/mobile-parity-and-consolidation) importstriggerBoundAgentIdfrom itsoss/home-overviewlane up, so this PR should merge (and reach that base) first.