feat: queue items pagination #1585
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces a new paginated API endpoint for retrieving queue items ( Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Server
participant QueueStore as Queue Store
participant ProcStore as Proc Store
Client->>API: GET /queues/{name}/items?type=queued&page=1&perPage=20
activate API
alt type == "queued"
API->>QueueStore: ListPaginated(ctx, name, paginator)
activate QueueStore
QueueStore-->>API: PaginatedResult[QueuedItemData]
deactivate QueueStore
else type == "running"
API->>ProcStore: Load running DAG runs
activate ProcStore
ProcStore-->>API: Running items
deactivate ProcStore
end
API->>API: Convert items to DAGRunSummary
API-->>Client: 200 QueueItemsResponse {items, pagination}
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/features/queues/components/QueueCard.tsx (1)
90-116: Clear Queue only dequeues the current page.
queuedItemsis limited to the active page, but the button/tooltip/modal claim “all queued DAG runs.” With pagination, this leaves items behind and is misleading. Either iterate through all pages (or add a server-side clear endpoint) or update the copy/counts to reflect page-scoped clearing.📝 Minimal copy fix (page-scoped clear)
- <span className="ml-1 text-xs">Clear</span> + <span className="ml-1 text-xs">Clear page</span> ... - <p>Remove all queued DAG runs</p> + <p>Remove queued DAG runs on this page</p> ... - <p className="text-sm"> - Remove all queued DAG runs from "{queue.name}"? - </p> + <p className="text-sm"> + Remove queued DAG runs on this page from "{queue.name}"? + </p> - <p className="text-xs text-muted-foreground"> - {queue.queuedCount || 0} DAG runs will be removed. This cannot be undone. - </p> + <p className="text-xs text-muted-foreground"> + {queuedItems.length} DAG runs on this page will be removed. This cannot be + undone. + </p>Also applies to: 290-316, 384-390
🤖 Fix all issues with AI agents
In `@api/v2/api.gen.go`:
- Around line 180-184: Add explicit enum validation for ListQueueItemsParamsType
by implementing a Validate() (or IsValid) function that returns an error for any
value not in the allowed set {"queued","running"}, and call this validator
wherever request params are parsed/handled instead of silently defaulting to
"queued" (replace the fallback logic in queues.go that currently forces unknown
types to "queued" with a rejection path). Also update the api.gen.go pagination
comments to reflect the actual behavior (page is clamped to >=1 and perPage
defaults to 50 and is capped at 200) so the docs match runtime enforcement.
In `@internal/core/exec/queue.go`:
- Around line 27-28: Add the missing ListPaginated method to the mockQueueStore
type used in collector_test; implement the method with the exact signature
ListPaginated(ctx context.Context, name string, pg Paginator)
(PaginatedResult[QueuedItemData], error) and return an appropriate zero-value
PaginatedResult[QueuedItemData] (or use any existing mock fields to drive
results) and nil error (or configurable error if the mock supports it) so
mockQueueStore satisfies the QueueStore interface; locate the mockQueueStore
definition in the tests and add this method there using the same receiver type.
In `@internal/service/frontend/api/v2/queues.go`:
- Around line 92-104: In the Len pass loop, getOrCreateQueue is being called
with dag == nil which causes DAG-based queues with only queued items to keep
maxConcurrency = 0; change the logic so before calling getOrCreateQueue you
detect DAG-based, non-global queues and load DAG metadata from the first queued
item (using the queue store read used for queued items in this service) to
extract MaxActiveRuns and pass that DAG into getOrCreateQueue; mirror the
approach used by updateQueueMaxConcurrency in queue_processor.go (load DAG from
first queued item, read MaxActiveRuns, then call getOrCreateQueue(queueMap,
queueName, a.config, dag)) so these queues contribute correctly to totalCapacity
and totalQueued.
In `@ui/src/features/queues/components/QueueCard.tsx`:
- Around line 143-167: The DAGRunRow tr is clickable but not
keyboard-accessible; update the DAGRunRow component to make rows focusable and
operable via keyboard by adding role="button" and tabIndex={0} to the <tr>, and
implement an onKeyDown handler that calls onDAGRunClick(dagRun) when Enter or
Space is pressed (also ensure existing onClick still uses onDAGRunClick). Keep
accessibility semantics by preserving the StatusChip and other cells and
optionally add an aria-label or aria-describedby if needed to describe the row
action.
In `@ui/src/pages/queues/index.tsx`:
- Around line 244-247: Guard the filtered-count render by first checking that
data?.queues exists before comparing lengths: update the conditional that uses
filteredQueues and data?.queues (the JSX around filteredQueues.length !==
data?.queues?.length) to require data?.queues (or data?.queues?.length != null)
first so the span is only rendered when queue data is loaded.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1585 +/- ##
==========================================
- Coverage 64.79% 64.71% -0.09%
==========================================
Files 259 259
Lines 28871 28899 +28
==========================================
- Hits 18707 18701 -6
- Misses 8489 8522 +33
- Partials 1675 1676 +1
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.