Filter workflow runs #294
Filter workflow runs #294
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant return-dispatch.ts
participant api.ts
participant GitHubAPI
Caller->>return-dispatch.ts: getRunIdAndUrl(workflowId, branch, startTime)
return-dispatch.ts->>api.ts: fetchWorkflowRunIds(workflowId, branch, startTimeISO)
api.ts->>GitHubAPI: listWorkflowRuns(created>=startTimeISO, event='workflow_dispatch', branch?)
GitHubAPI-->>api.ts: Workflow runs data
api.ts-->>return-dispatch.ts: Filtered workflow run IDs
return-dispatch.ts-->>Caller: Run ID and URL
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/api.ts (2)
161-172: Prefer passing aDateobject instead of a raw epoch number
startTimeis accepted as anumber, converted back toDate, and immediately transformed into ISO.
Exposing the parameter as aDate:-export async function fetchWorkflowRunIds( - workflowId: number, - branch: BranchNameResult, - startTime: number, +export async function fetchWorkflowRunIds( + workflowId: number, + branch: BranchNameResult, + startTime: Date,would make the intent explicit, remove the extra
setTimeindirection, and avoid accidental misuse with non-epoch numbers.-const startTimeDate = new Date(); -startTimeDate.setTime(startTime); -const startTimeISO = startTimeDate.toISOString(); +const startTimeISO = startTime.toISOString();
169-172: Redundant object creation
new Date(); startTimeDate.setTime(startTime)costs an allocation and does nothing if the caller already ensures a valid epoch.
If you keep the numeric parameter, replace the block with:const startTimeISO = new Date(startTime).toISOString();src/return-dispatch.ts (1)
152-156: Variable name shadows the imported function, hampers readabilityconst fetchWorkflowRunIds = await api.retryOrTimeout(/* … */);The local constant now shares the exact name of the imported
api.fetchWorkflowRunIds, forcing readers to parse scopes to understand which one is being used.
Consider renaming to something likeworkflowRunIdsResult(or simplyrunIdsResult) to avoid shadowing.-const fetchWorkflowRunIds = await api.retryOrTimeout( +const runIdsResult = await api.retryOrTimeout( () => api.fetchWorkflowRunIds(workflowId, branch, startTime), retryTimeout, ); -if (!fetchWorkflowRunIds.success) { +if (!runIdsResult.success) {src/api.spec.ts (1)
360-363: Use a fixed timestamp in tests for determinism & clearer intent
Date.now()is evaluated at call-time, so each expectation uses a slightly different value.
It isn’t an issue today because the mock ignores the parameter, but if you later assert oncreatedor snapshot requests, millisecond drift will break tests.-const now = Date.now(); -await expect(fetchWorkflowRunIds(0, branch, Date.now())).resolves… +const now = 1_689_000_000_000; // arbitrary, fixed +await expect(fetchWorkflowRunIds(0, branch, now)).resolves…Apply once in a
beforeEachand reuse the constant across assertions.Also applies to: 391-394, 419-422, 455-457, 492-495, 529-532
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/index.mjsis excluded by!**/dist/**
📒 Files selected for processing (3)
src/api.spec.ts(6 hunks)src/api.ts(1 hunks)src/return-dispatch.ts(1 hunks)
🔇 Additional comments (1)
src/api.ts (1)
178-180: Verify the format expected by the OctokitcreatedfilterThe code sends
created: '>' + startTimeISO.
Octokit/GitHub currently accept exact timestamps (e.g.2024-06-01T12:00:00Z) without comparison operators, and the use of a leading>is only documented for the Search API, not for the Actions list-runs endpoint.Please double-check that the produced request actually filters anything and doesn’t 400 / silently ignore the parameter.
Fixes Codex-/return-dispatch#264
carlobeltrame
force-pushed
the
filter-workflow-runs
branch
from
d49bf80 to
6b7c495
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/api.ts (2)
169-170: Minor micro-optimisation: avoid intermediate string concatenation
'>'can be prefixed in the template literal itself for marginally clearer intent.-const afterStartTime = ">" + new Date(startTime).toISOString(); +const afterStartTime = `>${new Date(startTime).toISOString()}`;
172-178: Use the workflow-scoped endpoint to reduce payload and remove unused parameter
octokit.rest.actions.listWorkflowRunsis a repository-wide endpoint.
Becauseworkflow_idis supplied, the more specificlistWorkflowRunsForWorkflowendpoint is a better fit; it is documented to acceptevent,created, etc., and avoids returning runs from other workflows in edge cases where the repo-wide endpoint silently ignoresworkflow_id.-const response = await octokit.rest.actions.listWorkflowRuns({ +const response = await octokit.rest.actions.listWorkflowRunsForWorkflow({ owner: config.owner, repo: config.repo, workflow_id: workflowId, created: afterStartTime, event: "workflow_dispatch",This change tightens the query scope and may further cut API traffic.
Please verify other call-sites compile after the rename.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/api.spec.ts(6 hunks)src/api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api.spec.ts
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
+ Coverage 96.88% 96.91% +0.03%
==========================================
Files 6 6
Lines 577 583 +6
Branches 112 112
==========================================
+ Hits 559 565 +6
Misses 17 17
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Fixes #264
Also adds a filter
event=workflow_dispatchto the workflow runs query. This should be safe and not a breaking change, since this action is only ever interested in the workflow_dispatched runs which it itself triggered.This should reduce the required API calls by a lot, if there are a lot of workflow runs in the target repository.
Summary by CodeRabbit
New Features
Bug Fixes