Repository navigation
Filter and sort queue jobs before pagination - #1516
hartikainen wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces server-side sorting and filtering capabilities for running and pending jobs, allowing users to filter by author and status, and sort by author, status, or priority. The changes span the API definitions, frontend UI components, backend services, and both Postgres and Spanner storage layers. The code review feedback highlights two high-severity issues: an incorrect status sorting mapping in the Postgres query that causes inconsistency with Spanner, and a potential nil pointer dereference panic in the filter parsing logic due to missing nil checks on the restriction's left-hand and right-hand sides.
c9766d3 to
99f43dd
Compare
99f43dd to
1e69167
Compare
There was a problem hiding this comment.
Sorry it took so long for the review, this PR kept getting lost in the cracks of my review pipeline.
Preserve author escapes after AIP parsing and compose only the requested queue predicates and ordering. Restore the default query shapes and use bytewise author ordering in PostgreSQL. Strengthen UI request and page-reset assertions, reuse status constants, and clarify the ROD documentation. Validated all 71 affected backend targets, 46 focused UI tests, nine Go binaries, the UI build, coverage, and applicable static checks. Review: #1516
Preserve author escapes after AIP parsing and compose only the requested queue predicates and ordering. Restore the default query shapes and use bytewise author ordering in PostgreSQL. Strengthen UI request and page-reset assertions, reuse status constants, and clarify the ROD documentation. Validated all 71 affected backend targets, 46 focused UI tests, nine Go binaries, the UI build, coverage, and applicable static checks. Review: #1516
bc06314 to
4503fc9
Compare
Apply exact author and active-status filters, matching counts, and sorts by author, status, or priority in both storage backends. Preserve the default scheduling display order when no sort is selected and use unique work-unit keys to break ties. Reset pagination when controls change and keep reported queue positions independent of display order. Add controls to the queue table and update its resource dashboard screenshot baselines. Closes #1514.
Preserve author escapes after AIP parsing and compose only the requested queue predicates and ordering. Restore the default query shapes and use bytewise author ordering in PostgreSQL. Strengthen UI request and page-reset assertions, reuse status constants, and clarify the ROD documentation. Validated all 71 affected backend targets, 46 focused UI tests, nine Go binaries, the UI build, coverage, and applicable static checks. Review: #1516
4503fc9 to
0940cbb
Compare
| t.Run("filtered pagination", func(t *testing.T) { | ||
| token := "" | ||
| for page := 0; page < 3; page++ { | ||
| result, err := repo.FetchActiveWorkUnitsByQueues(ctx, []string{"test-queue"}, 1, token, storage.ActiveWorkUnitQuery{OrderBy: "priority desc", Filter: storage.ActiveWorkUnitFilter{Author: "Bob@example.com", HasAuthor: true}}) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if result.TotalQueueSize != 3 || len(result.WorkUnits) != 1 { | ||
| t.Fatalf("filtered page = %+v", result) | ||
| } | ||
| token = result.NextPageToken | ||
| if (token != "") != (page < 2) { | ||
| t.Fatalf("page %d token = %q", page, token) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
[Priority: Clarity] internal/storage/postgres/workunit/queries_test.go:381-396
Observation: This is the only integration case that combines a filter with a non-default OrderBy (priority desc plus the author filter). It checks the count, the page length and whether a next-page token exists, but never which rows come back or in what order. If the ordering stopped applying when a filter is present, this subtest would still pass. The SQL-shape tests can't catch that either, because they pair filters only with the default ordering. The Spanner twin at fetch_active_work_units_test.go:249-264 has the same gap.
Risk / Rule: Test fidelity: an assertion must be able to fail when the behaviour it names regresses (.agents/rules/core_engineering.md §5). The Spanner and PostgreSQL implementations must behave the same (internal/storage/AGENTS.md §3).
Suggestion: Collect the names across the three pages and compare them with Bob's work units in descending priority, as the status-only subtest already does. Please mirror this in the Spanner test.
| t.Run("filtered pagination", func(t *testing.T) { | |
| token := "" | |
| for page := 0; page < 3; page++ { | |
| result, err := repo.FetchActiveWorkUnitsByQueues(ctx, []string{"test-queue"}, 1, token, storage.ActiveWorkUnitQuery{OrderBy: "priority desc", Filter: storage.ActiveWorkUnitFilter{Author: "Bob@example.com", HasAuthor: true}}) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if result.TotalQueueSize != 3 || len(result.WorkUnits) != 1 { | |
| t.Fatalf("filtered page = %+v", result) | |
| } | |
| token = result.NextPageToken | |
| if (token != "") != (page < 2) { | |
| t.Fatalf("page %d token = %q", page, token) | |
| } | |
| } | |
| }) | |
| t.Run("filtered pagination", func(t *testing.T) { | |
| var names []string | |
| token := "" | |
| for page := 0; page < 3; page++ { | |
| result, err := repo.FetchActiveWorkUnitsByQueues(ctx, []string{"test-queue"}, 1, token, storage.ActiveWorkUnitQuery{OrderBy: "priority desc", Filter: storage.ActiveWorkUnitFilter{Author: "Bob@example.com", HasAuthor: true}}) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if result.TotalQueueSize != 3 || len(result.WorkUnits) != 1 { | |
| t.Fatalf("filtered page = %+v", result) | |
| } | |
| names = append(names, result.WorkUnits[0].WorkUnit.GetName()) | |
| token = result.NextPageToken | |
| if (token != "") != (page < 2) { | |
| t.Fatalf("page %d token = %q", page, token) | |
| } | |
| } | |
| want := []string{"experiments/200/workUnits/3", "experiments/200/workUnits/2", "experiments/200/workUnits/1"} | |
| if diff := cmp.Diff(want, names); diff != "" { | |
| t.Errorf("filtered order (-want +got):\n%s", diff) | |
| } | |
| }) |
| } | ||
|
|
||
| async filterStatus(status: string): Promise<void> { | ||
| const select = await this.locatorFor(MatSelectHarness)(); |
There was a problem hiding this comment.
[Priority: Clarity] cmd/ui/app/side_panel/contents/queue_jobs_table/queue_jobs_table_harness.ts:90
Observation: locatorFor(MatSelectHarness) returns the first mat-select in the component. The template renders two: the Status filter (queue_jobs_table.ng.html:41) and the paginator's page-size select (queue_jobs_table.ng.html:162-167, pageSizeOptions at queue_jobs_table.ts:169). The helper works today only because the filter row comes first in the DOM, so a layout change would quietly make it drive the page-size select.
Risk / Rule: Harnesses should shield tests from template structure (.agents/rules/languages/typescript_javascript.md, Component Harness standards). The repo already scopes filter harnesses with an ancestor, e.g. work_units_table_harness.ts:41.
Suggestion: Scope the locator to the filter row that this PR adds (.job-filters).
| const select = await this.locatorFor(MatSelectHarness)(); | |
| const select = await this.locatorFor(MatSelectHarness.with({ ancestor: '.job-filters' < 8BD0 span class="pl-kos x">}))(); |
Apply exact author and active-status filters, matching counts, and sorts by author, status, or priority in both storage backends. Preserve the default scheduling display order when no sort is selected and use unique work-unit keys to break ties.
Reset pagination when controls change and keep reported queue positions independent of display order. Add controls to the queue table and update its resource dashboard screenshot baselines.
Tested locally. Here's what the filters look like:
Closes #1514.