From 7b134e84c50dbaac6c1ac56dc0c39ee8ab9ee557 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 12:36:37 +0200 Subject: [PATCH 1/9] Add PLAN --- PLAN.md | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 00000000..8eda2705 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,118 @@ +# Paginate the Merge Review candidate list (STIT-786) + +## Context + +`GET /api/v1/oil-gas-fields/merge-candidates` returns every candidate of every status as a bare array. The Merge Review page renders all of them, and each queue row fetches the full detail of every resource in its candidate just to show a name. After one link-all pass on production data (about 750 candidates), page load means one large list response plus about 1,500 detail requests, all rendered without virtualization. + +**Jira:** **STIT-786 "Add paging to Merge Review page"** already exists (Backlog, reporter jhoffart, no description). We'll use it. Related tickets: +- **STIT-795** (sort control, Jason, Selected for Development). This plan covers its API and UI for *date* sorts. Alphabetical sort is deferred to a follow-up (see below). The user is coordinating with Jason. +- **STIT-745** (search, filter, sort, "virtualize or paginate"). This plan covers part of it: pagination and the status filter. + +**Done when:** +1. The page fetches one page of candidates at a time, filtered to PENDING by default. +2. Queue rows render names with no per-row detail requests. +3. The header still shows whole-queue Pending, Reviewed and Total counts. +4. Status, page, page size and sort live in the URL. +5. Entity-linkage de-dupe still sees every candidate. +6. `make check` passes. + +## Decisions (agreed) + +- **Paging:** page-number paging using the existing `PaginationParams` / `PaginatedResponse` and the `Pagination` component. +- **Status filter:** a server-side, repeatable `status` param. No param returns all statuses. The frontend sends `status=PENDING` by default. The "Show approved" checkbox is replaced by a multi-select `FilterDropdown` (PENDING / APPROVED / DENIED), like the resource filters. +- **Counts:** add `status_counts` to the response. It's computed over the whole queue, ignoring the status filter, so the header numbers keep their current meaning. +- **Names:** the server computes `name` per candidate in this change. Name *sort* goes to a follow-up ticket. +- **Sort:** `sort_by ∈ {created, reviewed_at}` and `sort_order ∈ {asc, desc}`, default `created desc`, always with an `id` tiebreak. Today's order has no tiebreak, so bulk-created rows could shift between pages. + +## API: `deployments/api` + +1. **`entities.py`** + - Add `MergeCandidateQueryParams(PaginationParams, …)` with `status: list[MergeCandidateStatus] | None`, `sort_by: Literal["created", "reviewed_at"] = "created"` and `sort_order = "desc"`, following the `OGFieldQueryParams` pattern (entities.py:101-131). + - Add `name: str | None` to `MergeCandidateView`. `MergeCandidateDetailView` inherits it. + - Add `MergeCandidatePage(PaginatedResponse[MergeCandidateView])` with `status_counts: dict[MergeCandidateStatus, int]`. +2. **`db/merge_candidate_actions.py`: `list_merge_candidates`** (line 203) + - Accept params and `licensed_sources`. Apply the `status IN (…)` filter, then count, then `ORDER BY , id ` with `.limit/.offset`, following `og_field_resource_actions.py:57-90`. + - `reviewed_at` is null for pending rows; put nulls last. + - Run one `GROUP BY status` count query, unfiltered. + - Fill in names in one batch: collect the page's resource ids and call `coalesce_resources(session, ids, licensed_sources)` (`db/utils.py:198`). Per candidate, pick the name whose provenance source ranks best in `SOURCE_PRIORITY`, with ties going to item position. This mirrors `pickCandidateName` (frontend `utils/mergeCandidateName.js:12`). + - Have the detail view use the same helper so names agree. + - Keep the `named_query` labels (`merge_candidates.list`, plus `.count` and `.status_counts`). +3. **`routers/oil_gas_fields.py:102`** + - Set `response_model=MergeCandidatePage` and bind `Annotated[MergeCandidateQueryParams, Query()]`. + - Pass `licensed_sources(claims)` through, the same way the resource list does. +4. **No migration in this change.** At about 750 rows, scanning on `status` and `created` is fine. If `merge_candidates.list` timings say otherwise, add an index on `(status, created, id)` later. +5. **`API_REFERENCE.md:89-97`:** update the response shape and params. + +## Client and entity-linkage (same PR; the response shape changes) + +- **`packages/stitch-client/.../async_client.py:430`** + - Replace `list_merge_candidates` with `list_merge_candidates_page(page, page_size, status=None)`, which returns a dict. + - Add `iter_merge_candidates(...)`, mirroring `iter_oil_gas_fields` (lines 170+) and `_validate_page_params`. +- **`deployments/entity-linkage/.../client.py:78` and `matching.py:182` `_existing_fingerprints`:** iterate every page with no status filter, at `MAX_PAGE_SIZE`, so de-dupe still covers reviewed candidates. + +## Frontend: `deployments/stitch-frontend/src` + +1. **`queries/api.js:192` `getMergeCandidates`:** accept `{page, pageSize, status[], sortBy, sortOrder}` and serialize `status` as repeated params, the way the resource filters do. +2. **`queries/resources.js:44-50, 122-132`** + - Include the params in the list query key, keeping the `[endpoint, "merge-candidates", …]` prefix so review invalidation still works. + - Add `placeholderData: keepPreviousData`, matching the resource list. +3. **`hooks/useResources.js:107` and mocks at 326:** pass the params through. Update the mock to return the paged shape. +4. **URL state:** add a merge-review-specific parse/serialize pair (page, page_size, status, sort_by, sort_order), styled after `config/listParams.js`. + - It's a separate schema because `useListState` / `listParams` is tied to resource filters and columns. Generalizing it would be a refactor nobody asked for. + - Default view is `status=PENDING`, page 1, page size 50, created desc, with defaults left out of the URL. +5. **`pages/MergeCandidateReviewPage.jsx`** + - **Queue rendering** + - Render `data.items`. + - Replace the `showApproved` checkbox and the in-browser `visibleCandidates` filter (430-435) with a `FilterDropdown` for status. + - Add a sort control (created / reviewed, asc / desc). + - Put `` at the bottom of the sticky `QueuePanel`. It must fit the `md:max-h` flex column, so the list scrolls and the pager stays visible. + - **Header and names** + - Header counts come from `status_counts` (459-463, 517-536). + - `CandidateQueueItem` uses `candidate.name` and drops `useMergeCandidateName` / per-row `useMergeSourceDetails`. + - Check whether `useMergedResourceDetail` per row is still needed for merged rows. If so, keep it; it's only for approved rows, and those are filtered out by default. + - **Selection** + - Default selection stays "first PENDING on this page, else first item." + - After a review, choose the next pending item from the current page's items. + - If the page empties after refetch and `page > total_pages`, step back to the last page. + - Clear the selection when the filter, sort or page changes and the selected id isn't in the new page. + - **`hasHiddenApproved` (575-577):** replace with an empty-state message when the filter returns nothing, for example "No candidates match these statuses." +6. **Cleanup:** remove `useMergeCandidateName`, and `pickCandidateName` with its test, only if nothing else uses them after the change. The detail panel's source comparison still uses `useMergeSourceDetails`. + +## Tests + +- **API unit** (`tests/test_merge_candidate_actions.py`): status filter, the id tiebreak in ordering, `status_counts` ignoring the filter, name selection by source priority, and a null name when nothing is licensed. +- **API integration** (`tests/routers/test_merge_candidates_integration.py`, reusing `_create_candidate`): + - paging across pages with identical `created` (no duplicates or gaps) + - repeated `status` params + - 422 on a bad `status`, `sort_by` or `page_size`, following `test_query_param_validation.py` +- **Update existing tests:** `test_route_permissions.py:139` (expects `[]`), `observability/test_query_name_actions.py:372`, `test_openapi.py`. +- **Client and linkage:** `stitch-client/tests/test_async_client.py` (iterator ends on `total_pages`), `entity-linkage/tests/test_matching.py` (fingerprints span multiple pages). +- **Frontend:** + - `MergeCandidateReviewPage.test.jsx`: + - adapt the fixtures to the paged shape + - header counts come from `status_counts` + - the status dropdown changes the query params + - paging and URL round-trips + - next-pending after review on a page boundary + - names render with no `getResourceDetail` calls from the queue + - Also update `queries/resources.test.js` (key prefix) and `queries/api.test.js` (param serialization). + +## Suggested PR split (small, reviewable) + +1. **API, client, entity-linkage, and a minimal frontend adaptation.** The frontend reads `.items` and sends `status=PENDING`, replacing the in-browser filter. The shape change breaks old callers, so these ship together. +2. **Frontend UI:** status dropdown, sort control, pager, URL state, server names in rows. + +## Follow-up ticket to draft + +- **"Sort Merge Review candidates by name"** (covers STIT-795's alphabetical requirement). It needs a SQL ranking of coalesced names across all candidates for each user's licensed sources. + +## Verification + +- `make check` from the repo root. +- `make dev-docker`, seed with `004-merge-demo.json`, and run entity-linkage `link_all` to create candidates. On `/merge-candidate-review`: + - Only pending candidates show by default, and the header counts match the database. + - The network panel shows one list request and no per-row `/detail` requests. + - Paging, status and sort changes update the URL, and Back restores the previous view. + - Approving the last pending item on a page moves the selection correctly. +- Re-run `link_all` and confirm it creates no duplicate candidates, so de-dupe still sees every page. +- For scale, set a small `page_size` (for example 2) to exercise paging with the demo data. From 2af7b6cb53fc06884ab12cac64a06b952bc7d5df Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 12:45:27 +0200 Subject: [PATCH 2/9] feat(api): paginate the merge candidate list (STIT-786) GET /oil-gas-fields/merge-candidates returned every candidate of every status as a bare array, so the Merge Review page loaded the whole queue (~750 candidates after one link-all pass) and every row fetched each of its resources' details just to show a name. The endpoint now returns a page: - page / page_size, using the shared PaginationParams - a repeatable `status` filter; omitted, every status is returned - sort_by (created | reviewed_at) and sort_order, always with an id tiebreak so rows stay on one page when candidates share a timestamp - status_counts over the whole queue, ignoring the filter, so the header keeps reporting the full workload - a server-computed `name` per candidate: the merged resource's name once approved, otherwise the best-ranked source's name across its resources (the rule the frontend applied). Only list and detail responses carry it. Callers updated for the new shape: - stitch-client: list_merge_candidates() is replaced by list_merge_candidates_page() and iter_merge_candidates(); the now-unused _expect_list helper is removed - entity-linkage: still gathers every candidate of every status, page by page, so de-duplication keeps seeing reviewed candidates - frontend: reads the paged response, applies the "Show approved" filter on the server, and takes header counts from status_counts. It fetches a single page of 200 until the queue gets paging controls. Co-Authored-By: Claude Opus 5.5 --- deployments/api/API_REFERENCE.md | 8 +- .../stitch/api/db/merge_candidate_actions.py | 145 +++++++++++++-- deployments/api/src/stitch/api/entities.py | 45 ++++- .../src/stitch/api/routers/oil_gas_fields.py | 18 +- .../observability/test_query_name_actions.py | 13 +- .../test_merge_candidates_integration.py | 170 +++++++++++++++++- .../tests/routers/test_route_permissions.py | 13 +- .../api/tests/test_merge_candidate_actions.py | 94 +++++++++- .../src/stitch/entity_linkage/client.py | 6 +- .../entity-linkage/tests/test_client.py | 36 ++++ .../stitch-frontend/src/hooks/useResources.js | 28 ++- .../src/pages/MergeCandidateReviewPage.jsx | 44 +++-- .../pages/MergeCandidateReviewPage.test.jsx | 80 ++++----- .../stitch-frontend/src/queries/api.js | 11 +- .../stitch-frontend/src/queries/api.test.js | 45 +++++ .../stitch-frontend/src/queries/resources.js | 17 +- .../src/stitch/client/async_client.py | 71 ++++++-- .../stitch-client/tests/test_async_client.py | 57 ++++-- 18 files changed, 774 insertions(+), 127 deletions(-) diff --git a/deployments/api/API_REFERENCE.md b/deployments/api/API_REFERENCE.md index ee975323..fb1b8683 100644 --- a/deployments/api/API_REFERENCE.md +++ b/deployments/api/API_REFERENCE.md @@ -92,7 +92,12 @@ **Response:** `200` -- array[MergeCandidateView] +- `items`: array[MergeCandidateListItemView] +- `total_count`: integer +- `page`: integer +- `page_size`: integer +- `status_counts`: object[string, integer] +- `total_pages`: integer --- @@ -137,6 +142,7 @@ - `last_updated_by_id`: integer - `reviewed_at`: string | null - `reviewed_by_id`: integer | null +- `name`: string | null - `compare`: array[FieldComparisonView] --- diff --git a/deployments/api/src/stitch/api/db/merge_candidate_actions.py b/deployments/api/src/stitch/api/db/merge_candidate_actions.py index b93a712c..7e4337e8 100644 --- a/deployments/api/src/stitch/api/db/merge_candidate_actions.py +++ b/deployments/api/src/stitch/api/db/merge_candidate_actions.py @@ -1,10 +1,10 @@ from __future__ import annotations -from collections.abc import Collection, Sequence +from collections.abc import Collection, Mapping, Sequence from datetime import datetime, timezone from typing import Any -from sqlalchemy import select +from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload @@ -19,12 +19,15 @@ FieldComparisonView, MergeCandidateCreateRequest, MergeCandidateDetailView, + MergeCandidateListItemView, + MergeCandidatePage, + MergeCandidateQueryParams, MergeCandidateReviewRequest, MergeCandidateStatus, MergeCandidateView, ) from stitch.api.observability.context import named_query -from stitch.ogsi.model import OGFieldSource +from stitch.ogsi.model import SOURCE_PRIORITY, OGFieldResource, OGFieldSource from stitch.ogsi.model.og_field import OilGasFieldBase from stitch.ogsi.model.types import OGSISrcKey @@ -35,7 +38,7 @@ ResourceModel, ) from .og_field_resource_actions import apply_resource_merge -from .utils import coalesce_resources_with_sources +from .utils import coalesce_resources, coalesce_resources_with_sources def _normalize_resource_ids(resource_ids: Sequence[int]) -> list[int]: @@ -172,14 +175,72 @@ async def _default_source_priority(session: AsyncSession) -> dict[str, int]: return {source: priority for source, priority in rows.all()} +def _ordered_resource_ids(model: MergeCandidateModel) -> list[int]: + return [item.resource_id for item in sorted(model.items, key=lambda i: i.position)] + + +def _source_rank(source: OGSISrcKey) -> int: + return ( + SOURCE_PRIORITY.index(source) + if source in SOURCE_PRIORITY + else len(SOURCE_PRIORITY) + ) + + +def _candidate_name( + model: MergeCandidateModel, coalesced: Mapping[int, OGFieldResource] +) -> str | None: + """Display name for a candidate; see ``MergeCandidateListItemView.name``. + + ``coalesced`` must hold the candidate's resources and, once merged, its + merged resource. Source resources are ranked by the global + ``SOURCE_PRIORITY`` (the same rule the frontend used to apply), with ties + going to the earlier resource. + """ + if model.merged_resource_id is not None: + merged = coalesced[model.merged_resource_id].view + if merged is not None and merged.name is not None: + return merged.name + + best: tuple[int, str] | None = None + for resource_id in _ordered_resource_ids(model): + provenance = coalesced[resource_id].provenance.get("name") + if provenance is None: + continue + name, source, _source_pk = provenance + rank = _source_rank(source) + if best is None or rank < best[0]: + best = (rank, name) + return None if best is None else best[1] + + +def _name_resource_ids(models: Sequence[MergeCandidateModel]) -> list[int]: + """Every resource ``_candidate_name`` reads for these candidates.""" + ids: list[int] = [] + for model in models: + ids.extend(_ordered_resource_ids(model)) + if model.merged_resource_id is not None: + ids.append(model.merged_resource_id) + return list(dict.fromkeys(ids)) + + +def _candidate_to_list_item_view( + model: MergeCandidateModel, name: str | None +) -> MergeCandidateListItemView: + return MergeCandidateListItemView( + **_candidate_to_view(model).model_dump(), name=name + ) + + def _candidate_to_detail_view( model: MergeCandidateModel, + name: str | None, compare: Sequence[FieldComparisonView], ) -> MergeCandidateDetailView: - # MergeCandidateDetailView is MergeCandidateView + `compare`; reuse the base + # MergeCandidateDetailView is the list item + `compare`; reuse the base # mapping so the shared fields stay defined in one place. return MergeCandidateDetailView( - **_candidate_to_view(model).model_dump(), + **_candidate_to_list_item_view(model, name).model_dump(), compare=list(compare), ) @@ -200,15 +261,64 @@ async def _load_candidate_model( return model -async def list_merge_candidates(session: AsyncSession) -> list[MergeCandidateView]: +async def list_merge_candidates( + session: AsyncSession, + params: MergeCandidateQueryParams, + licensed_sources: Collection[OGSISrcKey] | None = None, +) -> MergeCandidatePage: + """One page of merge candidates, with whole-queue counts by status.""" + filtered = select(MergeCandidateModel.id) + if params.status: + filtered = filtered.where(MergeCandidateModel.status.in_(params.status)) + + with named_query("merge_candidates.list.count"): + total = ( + await session.scalar(select(func.count()).select_from(filtered.subquery())) + ) or 0 + + sort_column = getattr(MergeCandidateModel, params.sort_by) + if params.sort_order == "asc": + order_by = (sort_column.asc().nulls_last(), MergeCandidateModel.id.asc()) + else: + order_by = (sort_column.desc().nulls_last(), MergeCandidateModel.id.desc()) stmt = ( select(MergeCandidateModel) .options(selectinload(MergeCandidateModel.items)) - .order_by(MergeCandidateModel.created.desc()) + .where(MergeCandidateModel.id.in_(filtered)) + .order_by(*order_by) + .limit(params.limit) + .offset(params.offset) ) with named_query("merge_candidates.list"): candidates = (await session.scalars(stmt)).all() - return [_candidate_to_view(candidate) for candidate in candidates] + + with named_query("merge_candidates.list.status_counts"): + rows = await session.execute( + select(MergeCandidateModel.status, func.count()).group_by( + MergeCandidateModel.status + ) + ) + status_counts = dict.fromkeys(MergeCandidateStatus, 0) + for status, count in rows.all(): + status_counts[MergeCandidateStatus(status)] = count + + with named_query("merge_candidates.list.names"): + coalesced = await coalesce_resources( + session, _name_resource_ids(candidates), licensed_sources + ) + + return MergeCandidatePage( + items=[ + _candidate_to_list_item_view( + candidate, _candidate_name(candidate, coalesced) + ) + for candidate in candidates + ], + total_count=total, + page=params.page, + page_size=params.page_size, + status_counts=status_counts, + ) async def get_merge_candidate( @@ -219,9 +329,7 @@ async def get_merge_candidate( with named_query("merge_candidates.detail.load"): candidate = await _load_candidate_model(session, candidate_id) - resource_ids = [ - item.resource_id for item in sorted(candidate.items, key=lambda i: i.position) - ] + resource_ids = _ordered_resource_ids(candidate) # Computed live: repointed (post-merge) resources coalesce to a null-shell # here, so an APPROVED candidate's `compare` reflects the emptied originals. @@ -251,7 +359,18 @@ async def get_merge_candidate( resource_views = [by_id[rid].view for rid in resource_ids] compare = _build_comparison(resource_views, sources_with_priority) - return _candidate_to_detail_view(candidate, compare) + name_sources: dict[int, OGFieldResource] = dict(by_id) + if candidate.merged_resource_id is not None: + with named_query("merge_candidates.detail.merged_name"): + name_sources.update( + await coalesce_resources( + session, [candidate.merged_resource_id], licensed_sources + ) + ) + + return _candidate_to_detail_view( + candidate, _candidate_name(candidate, name_sources), compare + ) async def create_merge_candidate( diff --git a/deployments/api/src/stitch/api/entities.py b/deployments/api/src/stitch/api/entities.py index 24fe7f42..26e4e459 100644 --- a/deployments/api/src/stitch/api/entities.py +++ b/deployments/api/src/stitch/api/entities.py @@ -184,6 +184,49 @@ class MergeCandidateView(BaseModel): reviewed_by_id: int | None = None +class MergeCandidateListItemView(MergeCandidateView): + """A merge candidate as read back by the list and detail endpoints. + + ``name`` is the candidate's display name: the merged resource's name once + the candidate is approved (its source resources are emptied by the merge), + otherwise the name from the best-ranked source across its resources. It is + null when no licensed source carries a name. + """ + + name: str | None = None + + +class MergeCandidateFilterParams(BaseModel): + """Repeat ``status`` once per value (``?status=PENDING&status=DENIED``) to + match any of them. Omitted, every status is returned.""" + + status: list[MergeCandidateStatus] | None = None + + +class MergeCandidateSortParams(BaseModel): + # `id` is always appended in the same direction as a tiebreak, so pages + # stay stable when candidates share a timestamp (bulk creation). + sort_by: Literal["created", "reviewed_at"] = "created" + sort_order: Literal["asc", "desc"] = "desc" + + +class MergeCandidateQueryParams( + PaginationParams, MergeCandidateFilterParams, MergeCandidateSortParams +): + pass + + +class MergeCandidatePage(PaginatedResponse[MergeCandidateListItemView]): + """A page of merge candidates. + + ``status_counts`` covers the whole queue regardless of the ``status`` + filter, so a reviewer always sees the full workload. Every status is + present, zero when none exist. + """ + + status_counts: dict[MergeCandidateStatus, int] + + class ComparisonValueView(OGFieldSourceValueView): """A source's value in a merge comparison, tagged with ``resource_id`` -- the candidate resource the source is currently attached to. @@ -228,5 +271,5 @@ class FieldComparisonView(BaseModel): values: list[ComparisonValueView] -class MergeCandidateDetailView(MergeCandidateView): +class MergeCandidateDetailView(MergeCandidateListItemView): compare: list[FieldComparisonView] diff --git a/deployments/api/src/stitch/api/routers/oil_gas_fields.py b/deployments/api/src/stitch/api/routers/oil_gas_fields.py index 9e0c5c04..08a13008 100644 --- a/deployments/api/src/stitch/api/routers/oil_gas_fields.py +++ b/deployments/api/src/stitch/api/routers/oil_gas_fields.py @@ -16,6 +16,8 @@ OGFieldFilterOptionsResponse, MergeCandidateCreateRequest, MergeCandidateDetailView, + MergeCandidatePage, + MergeCandidateQueryParams, MergeCandidateReviewRequest, MergeCandidateView, OGFieldQueryParams, @@ -101,13 +103,21 @@ async def get_resource_filter_options( @router.get( "/merge-candidates", - response_model=list[MergeCandidateView], + response_model=MergeCandidatePage, dependencies=[Depends(require_permissions(MERGE_CANDIDATE_READ))], ) async def list_merge_candidates( - *, uow: UnitOfWorkDep, _user: CurrentUser -) -> list[MergeCandidateView]: - return await merge_candidate_actions.list_merge_candidates(session=uow.session) + *, + uow: UnitOfWorkDep, + _user: CurrentUser, + claims: Claims, + params: Annotated[MergeCandidateQueryParams, Query()], +) -> MergeCandidatePage: + return await merge_candidate_actions.list_merge_candidates( + session=uow.session, + params=params, + licensed_sources=licensed_sources(claims), + ) @router.get( diff --git a/deployments/api/tests/observability/test_query_name_actions.py b/deployments/api/tests/observability/test_query_name_actions.py index 619d2af3..450ac947 100644 --- a/deployments/api/tests/observability/test_query_name_actions.py +++ b/deployments/api/tests/observability/test_query_name_actions.py @@ -27,6 +27,7 @@ from stitch.api.db.model.oil_gas_field_source_value import ATTRIBUTE_NAMES from stitch.api.entities import ( MergeCandidateCreateRequest, + MergeCandidateQueryParams, MergeCandidateReviewRequest, OGFieldQueryParams, User, @@ -369,8 +370,16 @@ async def test_merge_candidate_action_labels( ) captured_query_events.clear() - await mca.list_merge_candidates(session) - _assert_labels(captured_query_events, {"merge_candidates.list"}) + await mca.list_merge_candidates(session, MergeCandidateQueryParams()) + _assert_labels( + captured_query_events, + { + "merge_candidates.list.count", + "merge_candidates.list", + "merge_candidates.list.status_counts", + "merge_candidates.list.names", + }, + ) captured_query_events.clear() await mca.get_merge_candidate(session, approved.id) diff --git a/deployments/api/tests/routers/test_merge_candidates_integration.py b/deployments/api/tests/routers/test_merge_candidates_integration.py index b1d9ace7..f4fbf991 100644 --- a/deployments/api/tests/routers/test_merge_candidates_integration.py +++ b/deployments/api/tests/routers/test_merge_candidates_integration.py @@ -1,14 +1,19 @@ -"""Integration tests for the merge-candidate detail endpoint (real SQLite).""" +"""Integration tests for the merge-candidate list and detail endpoints (real SQLite).""" from collections.abc import Sequence +from datetime import datetime, timezone from httpx import AsyncClient import pytest -from sqlalchemy import select +from sqlalchemy import select, update from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from tests.factories import ResourceCreateFactory -from stitch.api.db.model import MembershipModel, OGFieldResourceSourcePriority +from stitch.api.db.model import ( + MembershipModel, + MergeCandidateModel, + OGFieldResourceSourcePriority, +) from stitch.ogsi.model import OGFieldResource, OGFieldSource @@ -534,3 +539,162 @@ async def test_composite_resource_matches_when_winners_agree( (id_a, "gem", "Bar"), (id_b, "rmi", "Foo"), } + + +async def _create_disjoint_candidates( + client: AsyncClient, fact: ResourceCreateFactory, count: int +) -> list[int]: + """``count`` two-resource candidates sharing no resources, in creation order.""" + candidate_ids = [] + for i in range(count): + id_a = await _create_resource(client, fact, f"Field {i}") + id_b = await _create_resource(client, fact, f"Field {i} dup") + candidate_ids.append(await _create_candidate(client, [id_a, id_b])) + return candidate_ids + + +class TestMergeCandidateListIntegration: + @pytest.mark.anyio + async def test_pages_are_stable_when_candidates_share_a_timestamp( + self, + integration_client: AsyncClient, + og_create_res_fact: ResourceCreateFactory, + integration_session_factory: async_sessionmaker[AsyncSession], + ): + candidate_ids = await _create_disjoint_candidates( + integration_client, og_create_res_fact, 5 + ) + # Bulk creation can stamp many candidates with the same time; the id + # tiebreak must still give every candidate exactly one page. + async with integration_session_factory() as session: + await session.execute( + update(MergeCandidateModel).values( + created=datetime(2026, 1, 1, tzinfo=timezone.utc) + ) + ) + await session.commit() + + seen = [] + for page in (1, 2, 3): + resp = await integration_client.get( + "/oil-gas-fields/merge-candidates", + params={"page": page, "page_size": 2}, + ) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["total_count"] == 5 + assert body["total_pages"] == 3 + seen.extend(item["id"] for item in body["items"]) + + assert seen == sorted(candidate_ids, reverse=True) + + @pytest.mark.anyio + async def test_status_filter_narrows_items_but_not_counts( + self, + integration_client: AsyncClient, + og_create_res_fact: ResourceCreateFactory, + ): + approved_id, denied_id, pending_id = await _create_disjoint_candidates( + integration_client, og_create_res_fact, 3 + ) + for candidate_id, action in ((approved_id, "approve"), (denied_id, "deny")): + resp = await integration_client.post( + f"/oil-gas-fields/merge-candidates/{candidate_id}/{action}" + ) + assert resp.status_code == 200, resp.text + + pending = await integration_client.get( + "/oil-gas-fields/merge-candidates", params={"status": "PENDING"} + ) + assert pending.status_code == 200, pending.text + assert [item["id"] for item in pending.json()["items"]] == [pending_id] + assert pending.json()["total_count"] == 1 + assert pending.json()["status_counts"] == { + "PENDING": 1, + "APPROVED": 1, + "DENIED": 1, + } + + reviewed = await integration_client.get( + "/oil-gas-fields/merge-candidates", + params=[("status", "APPROVED"), ("status", "DENIED")], + ) + assert {item["id"] for item in reviewed.json()["items"]} == { + approved_id, + denied_id, + } + + unfiltered = await integration_client.get("/oil-gas-fields/merge-candidates") + assert unfiltered.json()["total_count"] == 3 + + @pytest.mark.anyio + @pytest.mark.parametrize("sort_order", ["asc", "desc"]) + async def test_sort_by_reviewed_at_puts_unreviewed_last( + self, + integration_client: AsyncClient, + og_create_res_fact: ResourceCreateFactory, + sort_order: str, + ): + first, second, unreviewed = await _create_disjoint_candidates( + integration_client, og_create_res_fact, 3 + ) + for candidate_id in (first, second): + resp = await integration_client.post( + f"/oil-gas-fields/merge-candidates/{candidate_id}/deny" + ) + assert resp.status_code == 200, resp.text + + resp = await integration_client.get( + "/oil-gas-fields/merge-candidates", + params={"sort_by": "reviewed_at", "sort_order": sort_order}, + ) + assert resp.status_code == 200, resp.text + reviewed = [first, second] if sort_order == "asc" else [second, first] + assert [item["id"] for item in resp.json()["items"]] == [ + *reviewed, + unreviewed, + ] + + @pytest.mark.anyio + async def test_items_carry_display_names( + self, + integration_client: AsyncClient, + og_create_res_fact: ResourceCreateFactory, + ): + id_a = await _create_resource(integration_client, og_create_res_fact, "Ghawar") + id_b = await _create_resource(integration_client, og_create_res_fact, "Burgan") + candidate_id = await _create_candidate(integration_client, [id_a, id_b]) + + resp = await integration_client.get("/oil-gas-fields/merge-candidates") + assert resp.status_code == 200, resp.text + (item,) = resp.json()["items"] + # Same source on both resources: the tie goes to the first resource. + assert item["name"] == "Ghawar" + + # After approval the originals are emptied; the merged resource names it. + approve = await integration_client.post( + f"/oil-gas-fields/merge-candidates/{candidate_id}/approve" + ) + assert approve.status_code == 200, approve.text + resp = await integration_client.get("/oil-gas-fields/merge-candidates") + (item,) = resp.json()["items"] + assert item["name"] == "Ghawar" + + @pytest.mark.anyio + @pytest.mark.parametrize( + "params", + [ + {"status": "MAYBE"}, + {"sort_by": "name"}, + {"sort_order": "sideways"}, + {"page": 0}, + {"page_size": 201}, + ], + ) + async def test_rejects_invalid_params( + self, integration_client: AsyncClient, params: dict + ): + resp = await integration_client.get( + "/oil-gas-fields/merge-candidates", params=params + ) + assert resp.status_code == 422, resp.text diff --git a/deployments/api/tests/routers/test_route_permissions.py b/deployments/api/tests/routers/test_route_permissions.py index ebaa09a4..4c65225d 100644 --- a/deployments/api/tests/routers/test_route_permissions.py +++ b/deployments/api/tests/routers/test_route_permissions.py @@ -17,6 +17,7 @@ from stitch.api.auth import get_token_claims from stitch.api.db.config import get_uow +from stitch.api.entities import MergeCandidatePage, MergeCandidateStatus from stitch.api.main import app @@ -144,8 +145,16 @@ async def test_merge_candidate_read_permission_allows_list_route( app.dependency_overrides[get_uow] = _uow_override(mock_uow) with patch("stitch.api.routers.oil_gas_fields.merge_candidate_actions") as actions: - actions.list_merge_candidates = AsyncMock(return_value=[]) + actions.list_merge_candidates = AsyncMock( + return_value=MergeCandidatePage( + items=[], + total_count=0, + page=1, + page_size=50, + status_counts=dict.fromkeys(MergeCandidateStatus, 0), + ) + ) response = await async_client.get("/oil-gas-fields/merge-candidates") assert response.status_code == 200 - assert response.json() == [] + assert response.json()["items"] == [] diff --git a/deployments/api/tests/test_merge_candidate_actions.py b/deployments/api/tests/test_merge_candidate_actions.py index b94c9a21..333ef8c4 100644 --- a/deployments/api/tests/test_merge_candidate_actions.py +++ b/deployments/api/tests/test_merge_candidate_actions.py @@ -9,6 +9,7 @@ from stitch.api.entities import ( MergeCandidateCreateRequest, MergeCandidateDetailView, + MergeCandidateQueryParams, MergeCandidateReviewRequest, MergeCandidateStatus, ) @@ -52,6 +53,7 @@ class FakeMergedResource: class FakeSession: scalar_result: object | None = None scalars_result: list[object] = field(default_factory=list) + execute_rows: list[tuple] = field(default_factory=list) added: list[object] = field(default_factory=list) added_all: list[object] = field(default_factory=list) deleted: list[object] = field(default_factory=list) @@ -64,6 +66,9 @@ async def scalar(self, _stmt): async def scalars(self, _stmt): return SimpleNamespace(all=lambda: list(self.scalars_result)) + async def execute(self, _stmt): + return SimpleNamespace(all=lambda: list(self.execute_rows)) + def add(self, obj): self.added.append(obj) @@ -116,8 +121,18 @@ def test_candidate_to_view_sorts_items_by_position(): assert view.id == 1 +def _named(name: str | None, source: str = "rmi", source_pk: int = 1): + """A coalesced resource whose only known field is ``name``.""" + return SimpleNamespace( + view=OilGasFieldBase(name=name, country=None), + provenance={"name": None if name is None else (name, source, source_pk)}, + ) + + @pytest.mark.anyio -async def test_list_merge_candidates_returns_views_in_query_order(): +async def test_list_merge_candidates_returns_page_with_names_and_counts( + monkeypatch, +): first = FakeCandidate( id=2, status=MergeCandidateStatus.PENDING, @@ -125,16 +140,69 @@ async def test_list_merge_candidates_returns_views_in_query_order(): ) second = FakeCandidate( id=1, - status=MergeCandidateStatus.DENIED, + status=MergeCandidateStatus.APPROVED, items=[FakeItem(20, 0), FakeItem(21, 1)], + merged_resource_id=30, + ) + session = FakeSession( + scalar_result=7, + scalars_result=[first, second], + execute_rows=[("PENDING", 5), ("APPROVED", 2)], ) - session = FakeSession(scalars_result=[first, second]) - views = await mca.list_merge_candidates(session) + async def fake_coalesce(session_arg, resource_ids, licensed): + assert resource_ids == [18, 19, 20, 21, 30] + assert licensed == ["rmi"] + return { + 18: _named("From WM", source="wm"), + 19: _named("From RMI", source="rmi"), + # Merged-away originals are null shells; the merged resource names it. + 20: _named(None), + 21: _named(None), + 30: _named("Merged"), + } + + monkeypatch.setattr(mca, "coalesce_resources", fake_coalesce) - assert [view.id for view in views] == [2, 1] - assert views[0].resource_ids == [18, 19] - assert views[1].status == MergeCandidateStatus.DENIED + page = await mca.list_merge_candidates( + session, MergeCandidateQueryParams(page=1, page_size=2), ["rmi"] + ) + + assert [view.id for view in page.items] == [2, 1] + assert page.items[0].resource_ids == [18, 19] + # Best-ranked source wins regardless of resource order. + assert page.items[0].name == "From RMI" + assert page.items[1].name == "Merged" + assert page.total_count == 7 + assert page.total_pages == 4 + # Every status is reported, including ones with no candidates. + assert page.status_counts == { + MergeCandidateStatus.PENDING: 5, + MergeCandidateStatus.APPROVED: 2, + MergeCandidateStatus.DENIED: 0, + } + + +def test_candidate_name_breaks_rank_ties_by_resource_position(): + candidate = FakeCandidate( + id=1, + status=MergeCandidateStatus.PENDING, + items=[FakeItem(19, 1), FakeItem(18, 0)], + ) + coalesced = {18: _named("First"), 19: _named("Second")} + + assert mca._candidate_name(candidate, coalesced) == "First" + + +def test_candidate_name_is_none_without_a_licensed_name(): + candidate = FakeCandidate( + id=1, + status=MergeCandidateStatus.PENDING, + items=[FakeItem(18, 0), FakeItem(19, 1)], + ) + coalesced = {18: _named(None), 19: _named(None)} + + assert mca._candidate_name(candidate, coalesced) is None @pytest.mark.anyio @@ -561,6 +629,7 @@ async def fake_coalesce(session_arg, resource_ids, licensed): rid: SimpleNamespace( source_data=[SimpleNamespace(source="rmi", id=rid, name=f"name-{rid}")], view=OilGasFieldBase(name=f"name-{rid}", country="USA"), + provenance={"name": (f"name-{rid}", "rmi", rid)}, ) for rid in resource_ids } @@ -578,6 +647,7 @@ async def fake_default_priority(session_arg): assert isinstance(view, MergeCandidateDetailView) assert view.resource_ids == [18, 19] + assert view.name == "name-18" # resources detail objects are gone; compare carries the per-source values assert not hasattr(view, "resources") assert [c.field for c in view.compare] == list(OilGasFieldBase.model_fields) @@ -607,21 +677,29 @@ async def fake_load_candidate_model(session_arg, candidate_id): async def fake_coalesce(session_arg, resource_ids, licensed): return { rid: SimpleNamespace( - source_data=[], view=OilGasFieldBase(name=None, country=None) + source_data=[], + view=OilGasFieldBase(name=None, country=None), + provenance={"name": None}, ) for rid in resource_ids } + async def fake_coalesce_merged(session_arg, resource_ids, licensed): + assert resource_ids == [31] + return {31: _named("Merged")} + async def fake_default_priority(session_arg): return {"rmi": 1, "gem": 2, "wm": 3, "llm": 4} monkeypatch.setattr(mca, "_load_candidate_model", fake_load_candidate_model) monkeypatch.setattr(mca, "coalesce_resources_with_sources", fake_coalesce) monkeypatch.setattr(mca, "_default_source_priority", fake_default_priority) + monkeypatch.setattr(mca, "coalesce_resources", fake_coalesce_merged) view = await mca.get_merge_candidate(AsyncMock(), candidate_id=2) assert view.merged_resource_id == 31 + assert view.name == "Merged" # every resource is null on every field -> they all agree (match), no values assert all(c.status == "match" for c in view.compare) assert all(c.values == [] for c in view.compare) diff --git a/deployments/entity-linkage/src/stitch/entity_linkage/client.py b/deployments/entity-linkage/src/stitch/entity_linkage/client.py index fb163927..6c11a9ff 100644 --- a/deployments/entity-linkage/src/stitch/entity_linkage/client.py +++ b/deployments/entity-linkage/src/stitch/entity_linkage/client.py @@ -76,7 +76,11 @@ async def iter_oil_gas_fields( yield self._to_candidate(item) async def list_merge_candidates(self) -> list[dict[str, Any]]: - return await self._client.list_merge_candidates() + """Every merge candidate of every status, gathered page by page. + + De-duplication needs reviewed candidates too, so no status filter. + """ + return [item async for item in self._client.iter_merge_candidates()] @staticmethod def _to_candidate(item: dict[str, Any]) -> FieldCandidate: diff --git a/deployments/entity-linkage/tests/test_client.py b/deployments/entity-linkage/tests/test_client.py index 9e234eec..c1198fa7 100644 --- a/deployments/entity-linkage/tests/test_client.py +++ b/deployments/entity-linkage/tests/test_client.py @@ -71,6 +71,42 @@ def handler(request: httpx.Request) -> httpx.Response: await client.aclose() +@pytest.mark.anyio +async def test_list_merge_candidates_gathers_every_page_of_every_status( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(STITCH_CLIENT_BEARER_TOKEN_ENV_VAR, "token-123") + requests: list[list[tuple[str, str]]] = [] + + def handler(request: httpx.Request) -> httpx.Response: + params = list(request.url.params.multi_items()) + requests.append(params) + page = int(request.url.params["page"]) + # A full first page, then one more candidate. + ids = range(200) if page == 1 else [200] + return httpx.Response( + 200, + json={ + "items": [{"id": i} for i in ids], + "total_count": 201, + "page": page, + "page_size": 200, + "total_pages": 2, + }, + ) + + client = make_client(handler) + + candidates = await client.list_merge_candidates() + + assert [c["id"] for c in candidates] == list(range(201)) + # No status filter: de-dupe must see reviewed candidates too. + assert requests == [ + [("page", "1"), ("page_size", "200")], + [("page", "2"), ("page_size", "200")], + ] + + @pytest.mark.anyio async def test_post_merge_sends_current_branch_payload_shape( monkeypatch: pytest.MonkeyPatch, diff --git a/deployments/stitch-frontend/src/hooks/useResources.js b/deployments/stitch-frontend/src/hooks/useResources.js index 14009924..7b79ff4d 100644 --- a/deployments/stitch-frontend/src/hooks/useResources.js +++ b/deployments/stitch-frontend/src/hooks/useResources.js @@ -104,10 +104,14 @@ function useFieldSourceValuesReal( }); } -function useMergeCandidatesReal(endpoint = "oil-gas-fields", enabled = true) { +function useMergeCandidatesReal( + endpoint = "oil-gas-fields", + params = {}, + enabled = true, +) { const config = useConfig(); return useAuthenticatedQuery({ - ...resourceQueries.mergeCandidates(config, endpoint), + ...resourceQueries.mergeCandidates(config, endpoint, params), enabled, }); } @@ -323,10 +327,24 @@ function useFieldSourceValuesMock( }); } -function useMergeCandidatesMock(endpoint = "oil-gas-fields", enabled = true) { +function useMergeCandidatesMock( + endpoint = "oil-gas-fields", + params = {}, + enabled = true, +) { + const page = params.page ?? 1; + const page_size = params.page_size ?? 50; return useQuery({ - ...resourceQueries.mergeCandidates(UNUSED_MOCK_CONFIG, endpoint), - queryFn: () => Promise.resolve([]), + ...resourceQueries.mergeCandidates(UNUSED_MOCK_CONFIG, endpoint, params), + queryFn: () => + Promise.resolve({ + items: [], + total_count: 0, + page, + page_size, + total_pages: 0, + status_counts: { PENDING: 0, APPROVED: 0, DENIED: 0 }, + }), enabled, }); } diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx index 70ade52e..01d03cc1 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx @@ -18,6 +18,11 @@ const ENDPOINT = "oil-gas-fields"; // Tailwind's md breakpoint, where the queue moves beside the decision panel // (the md: grid classes in the page layout below). const TWO_COLUMN_LAYOUT_QUERY = "(min-width: 48rem)"; +// One API page (its maximum size) until the queue gets paging controls. +const QUEUE_PAGE_SIZE = 200; +// Approved merges are finished work, so the queue hides them unless the +// reviewer opts in. +const UNFINISHED_STATUSES = ["PENDING", "DENIED"]; function getStatusClasses(status) { if (status === "PENDING") { @@ -421,18 +426,21 @@ export default function MergeCandidateReviewPage() { : null; const { - data: candidates, + data: candidatePage, isLoading: listLoading, isError: listError, error: listErrorObj, - } = useMergeCandidates(ENDPOINT, true); - - // Approved merges are finished work, so the queue hides them unless the - // reviewer opts in. Filtering here rather than in the API keeps the header - // counts below reporting on the whole workload, not just the visible rows. - const visibleCandidates = candidates?.filter( - (c) => showApproved || c.status !== "APPROVED", + } = useMergeCandidates( + ENDPOINT, + { + page: 1, + page_size: QUEUE_PAGE_SIZE, + // Omitting the status filter returns every status. + status: showApproved ? undefined : UNFINISHED_STATUSES, + }, + true, ); + const visibleCandidates = candidatePage?.items; // Default to the first pending candidate once the list loads. Done during // render (not in an effect) so the selection is set before the first paint @@ -453,14 +461,16 @@ export default function MergeCandidateReviewPage() { // until the detail query lands. Without this, review actions dead-click // while the detail is in flight. const listCandidate = - candidates?.find((item) => item.id === selectedId) ?? null; + visibleCandidates?.find((item) => item.id === selectedId) ?? null; const candidate = candidateQuery.data ?? listCandidate; - const pendingCount = - candidates?.filter((c) => c.status === "PENDING").length ?? 0; + // The API counts the whole queue regardless of the status filter, so the + // header reports on the whole workload, not just the visible rows. + const statusCounts = candidatePage?.status_counts; + const pendingCount = statusCounts?.PENDING ?? 0; const reviewedCount = - candidates?.filter((c) => c.status === "APPROVED" || c.status === "DENIED") - .length ?? 0; + (statusCounts?.APPROVED ?? 0) + (statusCounts?.DENIED ?? 0); + const totalCount = pendingCount + reviewedCount; function handleSelect(id) { setSelectedId(id); @@ -490,7 +500,7 @@ export default function MergeCandidateReviewPage() { // once the refreshed data is on its way. Failures surface via // `reviewMutation.error` and keep the current candidate selected. onSuccess: () => { - const nextPending = candidates?.find( + const nextPending = visibleCandidates?.find( (item) => item.id !== candidate.id && item.status === "PENDING", ); if (nextPending) { @@ -530,7 +540,7 @@ export default function MergeCandidateReviewPage() {
Total
- {candidates?.length ?? 0} + {totalCount}
@@ -572,9 +582,7 @@ export default function MergeCandidateReviewPage() { onSelect={handleSelect} showApproved={showApproved} onShowApprovedChange={setShowApproved} - hasHiddenApproved={ - Boolean(candidates?.length) && !visibleCandidates?.length - } + hasHiddenApproved={!showApproved && (statusCounts?.APPROVED ?? 0) > 0} /> diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx index d88eff73..125730b1 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx @@ -105,13 +105,37 @@ const resourceDetailsById = { 301: { data: { name: "Arabian Merged" }, provenance: { name: "rmi" } }, }; +// Stands in for the list endpoint: applies the requested status filter and +// counts the whole queue by status, as the API does. +function mockQueue(allCandidates) { + vi.mocked(useMergeCandidates).mockImplementation((_endpoint, params) => { + const items = allCandidates.filter( + (c) => !params?.status || params.status.includes(c.status), + ); + const countOf = (status) => + allCandidates.filter((c) => c.status === status).length; + return { + ...defaultHookReturn, + data: { + items, + total_count: items.length, + page: 1, + page_size: params?.page_size ?? 50, + total_pages: Math.ceil(items.length / (params?.page_size ?? 50)), + status_counts: { + PENDING: countOf("PENDING"), + APPROVED: countOf("APPROVED"), + DENIED: countOf("DENIED"), + }, + }, + refetch: vi.fn(), + }; + }); +} + beforeEach(() => { vi.mocked(useAuth0).mockReturnValue(auth0TestDefaults); - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: candidates, - refetch: vi.fn(), - }); + mockQueue(candidates); vi.mocked(useMergeCandidate).mockReturnValue({ ...defaultHookReturn, data: pendingDetail, @@ -174,10 +198,7 @@ describe("MergeCandidateReviewPage", () => { async function selectCandidate13() { const user = userEvent.setup(); - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [pendingCandidate, nextPendingCandidate], - }); + mockQueue([pendingCandidate, nextPendingCandidate]); vi.mocked(useMergeCandidate).mockImplementation((_endpoint, id) => ({ ...defaultHookReturn, data: @@ -390,10 +411,7 @@ describe("MergeCandidateReviewPage", () => { it("advances to the next pending candidate and clears notes after review", async () => { const user = userEvent.setup(); - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [pendingCandidate, nextPendingCandidate, candidates[1]], - }); + mockQueue([pendingCandidate, nextPendingCandidate, candidates[1]]); vi.mocked(useMergeCandidate).mockImplementation((_endpoint, id) => ({ ...defaultHookReturn, data: @@ -498,10 +516,7 @@ describe("MergeCandidateReviewPage", () => { it("shows the merged resource instead of the source comparison once merged_resource_id is set", async () => { const mergedCandidate = candidates[1]; - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [mergedCandidate], - }); + mockQueue([mergedCandidate]); vi.mocked(useMergeCandidate).mockReturnValue({ ...defaultHookReturn, data: mergedCandidate, @@ -520,10 +535,7 @@ describe("MergeCandidateReviewPage", () => { it("shows the merged resource's name in the heading once merged", async () => { const mergedCandidate = candidates[1]; - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [mergedCandidate], - }); + mockQueue([mergedCandidate]); // Post-merge, the originals are null shells: compare carries no name. vi.mocked(useMergeCandidate).mockReturnValue({ ...defaultHookReturn, @@ -545,10 +557,7 @@ describe("MergeCandidateReviewPage", () => { it("links the merged resource id to its detail page", async () => { const mergedCandidate = candidates[1]; - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [mergedCandidate], - }); + mockQueue([mergedCandidate]); vi.mocked(useMergeCandidate).mockReturnValue({ ...defaultHookReturn, data: mergedCandidate, @@ -566,11 +575,7 @@ describe("MergeCandidateReviewPage", () => { }); it("blocks with an error when the detail query fails and the queue has no item", () => { - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [], - refetch: vi.fn(), - }); + mockQueue([]); vi.mocked(useMergeCandidate).mockReturnValue({ ...defaultHookReturn, data: null, @@ -641,10 +646,7 @@ describe("MergeCandidateReviewPage", () => { }); it("keeps denied candidates visible by default", async () => { - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [pendingCandidate, deniedCandidate, approvedCandidate], - }); + mockQueue([pendingCandidate, deniedCandidate, approvedCandidate]); renderWithQueryClient(); const user = userEvent.setup(); @@ -679,10 +681,7 @@ describe("MergeCandidateReviewPage", () => { it("does not select a hidden approved candidate by default", async () => { // No pending work left: the fallback selection must land on the visible // denied candidate, never on the filtered-out approved one. - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [approvedCandidate, deniedCandidate], - }); + mockQueue([approvedCandidate, deniedCandidate]); vi.mocked(useMergeCandidate).mockReturnValue({ ...defaultHookReturn, data: { ...deniedCandidate, compare: [] }, @@ -696,10 +695,7 @@ describe("MergeCandidateReviewPage", () => { }); it("explains an empty queue caused by the filter", async () => { - vi.mocked(useMergeCandidates).mockReturnValue({ - ...defaultHookReturn, - data: [approvedCandidate], - }); + mockQueue([approvedCandidate]); renderWithQueryClient(); expect( diff --git a/deployments/stitch-frontend/src/queries/api.js b/deployments/stitch-frontend/src/queries/api.js index fa1cfdb9..49ecdc32 100644 --- a/deployments/stitch-frontend/src/queries/api.js +++ b/deployments/stitch-frontend/src/queries/api.js @@ -189,12 +189,21 @@ export async function createSourceForResource( return await response.json(); } +// `status` is a list sent as a repeated param (any of them matches); omitted, +// the API returns every status. export async function getMergeCandidates( config, fetcher, endpoint = "oil-gas-fields", + { page = 1, page_size = 50, status, sort_by, sort_order } = {}, ) { - const url = `${config.apiBaseUrl}/${endpoint}/merge-candidates`; + const params = new URLSearchParams({ page, page_size }); + for (const value of status ?? []) { + params.append("status", value); + } + if (sort_by) params.set("sort_by", sort_by); + if (sort_order) params.set("sort_order", sort_order); + const url = `${config.apiBaseUrl}/${endpoint}/merge-candidates?${params}`; const response = await fetcher(url); if (!response.ok) { diff --git a/deployments/stitch-frontend/src/queries/api.test.js b/deployments/stitch-frontend/src/queries/api.test.js index 5722bb08..28f962f6 100644 --- a/deployments/stitch-frontend/src/queries/api.test.js +++ b/deployments/stitch-frontend/src/queries/api.test.js @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { createLLMSuggestion, createSourceForResource, + getMergeCandidates, getResourceFilterOptions, getResources, getResource, @@ -394,6 +395,50 @@ describe("API Functions", () => { }); }); + describe("getMergeCandidates", () => { + it("requests the first page of every status by default", async () => { + mockFetcher.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ items: [] }), + }); + + await getMergeCandidates(config, mockFetcher); + + expect(mockFetcher).toHaveBeenCalledWith( + "http://localhost:8000/api/v1/oil-gas-fields/merge-candidates?page=1&page_size=50", + ); + }); + + it("sends statuses as repeated params alongside paging and sort", async () => { + mockFetcher.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ items: [] }), + }); + + await getMergeCandidates(config, mockFetcher, "oil-gas-fields", { + page: 2, + page_size: 25, + status: ["PENDING", "DENIED"], + sort_by: "reviewed_at", + sort_order: "asc", + }); + + expect(mockFetcher).toHaveBeenCalledWith( + "http://localhost:8000/api/v1/oil-gas-fields/merge-candidates?page=2&page_size=25&status=PENDING&status=DENIED&sort_by=reviewed_at&sort_order=asc", + ); + }); + + it("throws with the HTTP status on failure", async () => { + mockFetcher.mockResolvedValueOnce({ ok: false, status: 503 }); + + await expect( + getMergeCandidates(config, mockFetcher), + ).rejects.toMatchObject({ status: 503 }); + }); + }); + describe("reviewMergeCandidate", () => { it("posts the requested review action and notes", async () => { mockFetcher.mockResolvedValueOnce({ diff --git a/deployments/stitch-frontend/src/queries/resources.js b/deployments/stitch-frontend/src/queries/resources.js index 4e8332eb..ea71ddab 100644 --- a/deployments/stitch-frontend/src/queries/resources.js +++ b/deployments/stitch-frontend/src/queries/resources.js @@ -45,6 +45,11 @@ const keys = { ...keys.all(endpoint), "merge-candidates", ], + mergeCandidateList: (endpoint = "oil-gas-fields", params) => [ + ...keys.mergeCandidates(endpoint), + "list", + params, + ], mergeCandidate: (endpoint = "oil-gas-fields", id) => [ ...keys.mergeCandidates(endpoint), id, @@ -119,10 +124,16 @@ export const resourceQueries = { queryFn: (fetcher) => getResource(config, id, fetcher, endpoint), }), - mergeCandidates: (config, endpoint = "oil-gas-fields") => + // `params` is { page, page_size, status, sort_by, sort_order }; see + // getMergeCandidates. + mergeCandidates: (config, endpoint = "oil-gas-fields", params = {}) => queryOptions({ - queryKey: keys.mergeCandidates(endpoint), - queryFn: (fetcher) => getMergeCandidates(config, fetcher, endpoint), + queryKey: keys.mergeCandidateList(endpoint, params), + queryFn: (fetcher) => + getMergeCandidates(config, fetcher, endpoint, params), + // Keeps the previous page's queue on screen while the next page or + // filter fetches, instead of the queue flashing to a loading state. + placeholderData: keepPreviousData, }), mergeCandidate: (config, endpoint = "oil-gas-fields", id) => diff --git a/packages/stitch-client/src/stitch/client/async_client.py b/packages/stitch-client/src/stitch/client/async_client.py index b16d9b16..21270ab9 100644 --- a/packages/stitch-client/src/stitch/client/async_client.py +++ b/packages/stitch-client/src/stitch/client/async_client.py @@ -427,13 +427,70 @@ async def create_merge_candidate(self, resource_ids: list[int]) -> dict[str, Any ) return self._expect_dict(payload, "POST /oil-gas-fields/merge-candidates") - async def list_merge_candidates(self) -> list[dict[str, Any]]: + async def list_merge_candidates_page( + self, + *, + page: int = 1, + page_size: int = 50, + status: Sequence[str] | None = None, + ) -> dict[str, Any]: + """One page of merge candidates, newest first. + + ``status`` limits the page to candidates in any of the given statuses + (``PENDING``, ``APPROVED``, ``DENIED``); omitted, every status is + returned. + + Raises: + ValueError: if ``page``/``page_size`` are out of range. + StitchAPIError: on a non-2xx response. + """ + self._validate_page_params(page, page_size) + params: dict[str, Any] = {"page": page, "page_size": page_size} + if status is not None: + params["status"] = list(status) payload = await self._request_json( method="GET", path="/oil-gas-fields/merge-candidates", operation="GET /oil-gas-fields/merge-candidates", + params=params, ) - return self._expect_list(payload, "GET /oil-gas-fields/merge-candidates") + return self._expect_dict(payload, "GET /oil-gas-fields/merge-candidates") + + async def iter_merge_candidates( + self, + *, + page_size: int = MAX_PAGE_SIZE, + status: Sequence[str] | None = None, + ) -> AsyncIterator[dict[str, Any]]: + """Yield every merge candidate, fetching one page at a time. + + Page-termination logic mirrors ``iter_oil_gas_fields``. + """ + page = 1 + while True: + payload = await self.list_merge_candidates_page( + page=page, page_size=page_size, status=status + ) + raw_item_count = self._item_count(payload) + page_items = self._extract_items(payload) + + if raw_item_count == 0: + return + + for item in page_items: + yield item + + total_pages = payload.get("total_pages") + if isinstance(total_pages, int) and page >= total_pages: + return + + if not isinstance(total_pages, int) and not page_items: + return + + if raw_item_count < page_size: + return + + page += 1 def _headers(self) -> dict[str, str]: if self._headers_provider is None: @@ -602,16 +659,6 @@ def _expect_dict(payload: Any, operation: str) -> dict[str, Any]: return payload raise StitchAPIError(f"{operation} returned non-object JSON payload") - @staticmethod - def _expect_list(payload: Any, operation: str) -> list[dict[str, Any]]: - if not isinstance(payload, list): - raise StitchAPIError(f"{operation} returned non-array JSON payload") - if not all(isinstance(item, dict) for item in payload): - raise StitchAPIError( - f"{operation} returned an array with non-object elements" - ) - return payload - @staticmethod def _extract_items(payload: dict[str, Any]) -> list[dict[str, Any]]: items = payload.get("items") diff --git a/packages/stitch-client/tests/test_async_client.py b/packages/stitch-client/tests/test_async_client.py index c1bd3694..eda8ea47 100644 --- a/packages/stitch-client/tests/test_async_client.py +++ b/packages/stitch-client/tests/test_async_client.py @@ -594,52 +594,87 @@ def handler(request: httpx.Request) -> httpx.Response: @pytest.mark.anyio -async def test_list_merge_candidates_sends_expected_request() -> None: +async def test_list_merge_candidates_page_sends_expected_request() -> None: captured: dict[str, Any] = {} def handler(request: httpx.Request) -> httpx.Response: captured["method"] = request.method captured["path"] = request.url.path + captured["params"] = list(request.url.params.multi_items()) return httpx.Response( 200, - json=[{"id": 1, "resource_ids": [1, 2], "status": "PENDING"}], + json={"items": [], "total_count": 0, "page": 2, "page_size": 10}, ) client, raw_client = make_client(handler) - candidates = await client.list_merge_candidates() + payload = await client.list_merge_candidates_page( + page=2, page_size=10, status=["PENDING", "DENIED"] + ) - assert candidates == [{"id": 1, "resource_ids": [1, 2], "status": "PENDING"}] + assert payload["page"] == 2 assert captured == { "method": "GET", "path": "/api/v1/oil-gas-fields/merge-candidates", + "params": [ + ("page", "2"), + ("page_size", "10"), + ("status", "PENDING"), + ("status", "DENIED"), + ], } await raw_client.aclose() @pytest.mark.anyio -async def test_list_merge_candidates_rejects_non_array_payload() -> None: +async def test_list_merge_candidates_page_rejects_non_object_payload() -> None: def handler(request: httpx.Request) -> httpx.Response: - return httpx.Response(200, json={"not": "a-list"}) + return httpx.Response(200, json=[{"id": 1}]) client, raw_client = make_client(handler) with pytest.raises(StitchAPIError): - await client.list_merge_candidates() + await client.list_merge_candidates_page() + + await raw_client.aclose() + + +@pytest.mark.anyio +async def test_list_merge_candidates_page_validates_page_size() -> None: + client, raw_client = make_client(lambda request: httpx.Response(200, json={})) + + with pytest.raises(ValueError, match="page_size"): + await client.list_merge_candidates_page(page_size=201) await raw_client.aclose() @pytest.mark.anyio -async def test_list_merge_candidates_rejects_non_object_elements() -> None: +async def test_iter_merge_candidates_follows_total_pages() -> None: + requested_pages: list[int] = [] + def handler(request: httpx.Request) -> httpx.Response: - return httpx.Response(200, json=[{"id": 1}, "not-a-dict"]) + page = int(request.url.params["page"]) + requested_pages.append(page) + items = {1: [{"id": 3}, {"id": 2}], 2: [{"id": 1}]}[page] + return httpx.Response( + 200, + json={ + "items": items, + "total_count": 3, + "page": page, + "page_size": 2, + "total_pages": 2, + }, + ) client, raw_client = make_client(handler) - with pytest.raises(StitchAPIError): - await client.list_merge_candidates() + ids = [c["id"] async for c in client.iter_merge_candidates(page_size=2)] + + assert ids == [3, 2, 1] + assert requested_pages == [1, 2] await raw_client.aclose() From 0b1e591f40ef3679a18011e6c8b4995778aa5488 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 14:36:58 +0200 Subject: [PATCH 3/9] feat(frontend): page, filter and sort the Merge Review queue The queue fetched a single 200-candidate page with a "Show approved" checkbox, and every row fetched each of its resources' details to show a name. It now uses the paged list endpoint end to end. - Status filter: a multi-select dropdown (Pending / Approved / Denied) replaces the checkbox. It defaults to Pending and shows each status's queue-wide count. Clearing every status shows all of them. - Sort: one plain-language select (newest/oldest first, recently/earliest reviewed), mapped to the API's sort_by + sort_order. - Paging: the shared Pagination control sits under the sticky queue, 25 per page by default; changing page scrolls the list to the top. - URL state: page, page_size, status and sort are serialized in the query string (config/mergeReviewParams.js, useMergeReviewState), so links and Back/Forward restore the view. A bare /merge-candidate-review is the pending queue; selecting no statuses is written as status=all. Documented in the README. - Names: queue rows and the panel heading use the API's `name`, so the queue makes no per-resource requests and the two always agree. - Selection: the first pending candidate on the page is selected whenever the current one is not on it, but not while the next page is still loading. A page past the end steps back to the last page. Removes the browser-side naming code this leaves unused: useMergeCandidateName, useMergeSourceDetails, utils/mergeCandidateName and pickCompareName. Co-Authored-By: Claude Opus 5.5 --- deployments/stitch-frontend/README.md | 26 ++ .../src/config/mergeReviewParams.js | 138 ++++++++ .../src/config/mergeReviewParams.test.js | 112 ++++++ .../src/hooks/useMergeCandidateName.js | 10 - .../src/hooks/useMergeReviewState.js | 39 +++ .../src/hooks/useMergeSourceDetails.js | 21 -- .../src/hooks/useMergedResourceDetail.js | 7 +- .../src/pages/MergeCandidateReviewPage.jsx | 217 +++++++----- .../pages/MergeCandidateReviewPage.test.jsx | 320 +++++++++++------- .../src/utils/candidateCompare.js | 8 - .../src/utils/candidateCompare.test.js | 32 +- .../src/utils/mergeCandidateName.js | 26 -- .../src/utils/mergeCandidateName.test.js | 54 --- 13 files changed, 652 insertions(+), 358 deletions(-) create mode 100644 deployments/stitch-frontend/src/config/mergeReviewParams.js create mode 100644 deployments/stitch-frontend/src/config/mergeReviewParams.test.js delete mode 100644 deployments/stitch-frontend/src/hooks/useMergeCandidateName.js create mode 100644 deployments/stitch-frontend/src/hooks/useMergeReviewState.js delete mode 100644 deployments/stitch-frontend/src/hooks/useMergeSourceDetails.js delete mode 100644 deployments/stitch-frontend/src/utils/mergeCandidateName.js delete mode 100644 deployments/stitch-frontend/src/utils/mergeCandidateName.test.js diff --git a/deployments/stitch-frontend/README.md b/deployments/stitch-frontend/README.md index b6f280c4..cb01ceda 100644 --- a/deployments/stitch-frontend/README.md +++ b/deployments/stitch-frontend/README.md @@ -12,6 +12,7 @@ React frontend application styled with Tailwind CSS and built with Vite. - [Testing](#testing) - [Queries](#queries) - [List view URLs](#list-view-urls) +- [Merge review URLs](#merge-review-urls) - [Resource Detail View](#resource-detail-view) ## Tech Stack @@ -217,6 +218,31 @@ Notes: The schema lives in `src/config/listParams.js`. +## Merge review URLs + +The Merge Review queue (`/merge-candidate-review`) encodes its view the same way, so a copied link opens the same page of the queue for everyone. + +| Param | Values | Omitted when | +| ------------------------ | --------------------------------------------------------- | ----------------------------- | +| `page` | integer ≥ 1 | page 1 | +| `page_size` | 10, 25, 50, 100 | 25 | +| `status` | `PENDING`, `APPROVED`, `DENIED` (repeat per value), `all` | only `PENDING` is selected | +| `sort_by` + `sort_order` | `created` or `reviewed_at`, with `asc` or `desc` | newest first (`created desc`) | + +Example — approved and denied candidates, most recently reviewed first, page 2: + +``` +/merge-candidate-review?page=2&status=APPROVED&status=DENIED&sort_by=reviewed_at&sort_order=desc +``` + +Notes: + +- The queue shows pending candidates by default, so a bare `/merge-candidate-review` is the review work still to do. Clearing every status in the Status filter shows all of them, written as `status=all`. +- The page size is not remembered between visits, unlike the resources list. +- Unknown statuses are dropped, and a malformed `page`, `page_size` or sort falls back to its default. A `page` past the end steps back to the last page. + +The schema lives in `src/config/mergeReviewParams.js`. + ## API To temporarily run the API in development mode, outside Docker: diff --git a/deployments/stitch-frontend/src/config/mergeReviewParams.js b/deployments/stitch-frontend/src/config/mergeReviewParams.js new file mode 100644 index 00000000..9af3d651 --- /dev/null +++ b/deployments/stitch-frontend/src/config/mergeReviewParams.js @@ -0,0 +1,138 @@ +/** + * URL schema for the Merge Review queue. + * + * Same contract as the resources list (config/listParams.js): the query string + * is a complete serialization of the queue view, loading a URL reproduces it + * for any user, and the param names mirror what the API takes, so there is no + * translation layer. See the "Merge review URLs" section of the frontend README + * before changing them. + * + * Rules: + * - Defaults are omitted, so the default view (pending candidates, newest + * first, page 1) is a bare /merge-candidate-review. + * - Serialization order is fixed, so the same view always produces the same URL. + * - Junk is tolerated, never fatal: an unknown status is dropped, and a + * malformed page, page_size or sort falls back to its default. + * - The default status filter is PENDING, so "no status param" cannot also + * mean "every status". Selecting nothing is written as `status=all`. + */ +import { DEFAULT_PAGE, PAGE_SIZE_OPTIONS } from "../queries/resources"; + +export const MERGE_STATUSES = ["PENDING", "APPROVED", "DENIED"]; +export const DEFAULT_STATUSES = ["PENDING"]; +const ALL_STATUSES = "all"; + +export const DEFAULT_QUEUE_PAGE_SIZE = 25; + +// Each option is one sort_by + sort_order pair the API accepts, labelled in +// plain language. The first is the default. +export const QUEUE_SORT_OPTIONS = [ + { + key: "newest", + label: "Newest first", + sortBy: "created", + sortOrder: "desc", + }, + { key: "oldest", label: "Oldest first", sortBy: "created", sortOrder: "asc" }, + { + key: "recently-reviewed", + label: "Recently reviewed", + sortBy: "reviewed_at", + sortOrder: "desc", + }, + { + key: "earliest-reviewed", + label: "Earliest reviewed", + sortBy: "reviewed_at", + sortOrder: "asc", + }, +]; +const DEFAULT_SORT = QUEUE_SORT_OPTIONS[0]; + +function parsePage(raw) { + const value = Number(raw); + return /^\d+$/.test(raw ?? "") && Number.isSafeInteger(value) && value >= 1 + ? value + : DEFAULT_PAGE; +} + +function parsePageSize(raw) { + const value = Number(raw); + return PAGE_SIZE_OPTIONS.includes(value) ? value : DEFAULT_QUEUE_PAGE_SIZE; +} + +// [] means "every status". Returned in MERGE_STATUSES order. +function parseStatuses(rawValues) { + if (rawValues.includes(ALL_STATUSES)) return []; + const statuses = MERGE_STATUSES.filter((status) => + rawValues.includes(status), + ); + return statuses.length ? statuses : DEFAULT_STATUSES; +} + +// sort_by and sort_order are a unit; an unknown pair falls back to the default. +function parseSort(rawSortBy, rawSortOrder) { + return ( + QUEUE_SORT_OPTIONS.find( + (option) => + option.sortBy === rawSortBy && option.sortOrder === rawSortOrder, + ) ?? DEFAULT_SORT + ); +} + +function isDefaultStatuses(statuses) { + return ( + statuses.length === DEFAULT_STATUSES.length && + DEFAULT_STATUSES.every((status) => statuses.includes(status)) + ); +} + +export function parseMergeReviewParams(searchParams) { + return { + page: parsePage(searchParams.get("page")), + pageSize: parsePageSize(searchParams.get("page_size")), + statuses: parseStatuses(searchParams.getAll("status")), + sortKey: parseSort( + searchParams.get("sort_by"), + searchParams.get("sort_order"), + ).key, + }; +} + +export function toMergeReviewParams({ page, pageSize, statuses, sortKey }) { + const params = new URLSearchParams(); + if (page > DEFAULT_PAGE) params.set("page", String(page)); + if (pageSize !== DEFAULT_QUEUE_PAGE_SIZE) { + params.set("page_size", String(pageSize)); + } + + if (statuses.length === 0) { + params.set("status", ALL_STATUSES); + } else if (!isDefaultStatuses(statuses)) { + for (const status of MERGE_STATUSES) { + if (statuses.includes(status)) params.append("status", status); + } + } + + const sort = QUEUE_SORT_OPTIONS.find((option) => option.key === sortKey); + if (sort && sort !== DEFAULT_SORT) { + params.set("sort_by", sort.sortBy); + params.set("sort_order", sort.sortOrder); + } + + return params; +} + +// The list endpoint's query params for a parsed view. An empty status list +// sends no status filter, which the API reads as every status. +export function toMergeCandidateQuery({ page, pageSize, statuses, sortKey }) { + const sort = + QUEUE_SORT_OPTIONS.find((option) => option.key === sortKey) ?? DEFAULT_SORT; + return { + page, + page_size: pageSize, + status: statuses.length ? statuses : undefined, + sort_by: sort.sortBy, + sort_order: sort.sortOrder, + }; +} diff --git a/deployments/stitch-frontend/src/config/mergeReviewParams.test.js b/deployments/stitch-frontend/src/config/mergeReviewParams.test.js new file mode 100644 index 00000000..dc5adc89 --- /dev/null +++ b/deployments/stitch-frontend/src/config/mergeReviewParams.test.js @@ -0,0 +1,112 @@ +import { describe, it, expect } from "vitest"; +import { + parseMergeReviewParams, + toMergeCandidateQuery, + toMergeReviewParams, +} from "./mergeReviewParams"; + +const parse = (search) => parseMergeReviewParams(new URLSearchParams(search)); +const serialize = (state) => toMergeReviewParams(state).toString(); + +const DEFAULT_STATE = { + page: 1, + pageSize: 25, + statuses: ["PENDING"], + sortKey: "newest", +}; + +describe("parseMergeReviewParams", () => { + it("returns the pending queue, newest first, for an empty query string", () => { + expect(parse("")).toEqual(DEFAULT_STATE); + }); + + it("reads every supported param", () => { + expect( + parse( + "page=3&page_size=50&status=DENIED&status=APPROVED&sort_by=reviewed_at&sort_order=asc", + ), + ).toEqual({ + page: 3, + pageSize: 50, + // Canonical order, whatever order the URL used. + statuses: ["APPROVED", "DENIED"], + sortKey: "earliest-reviewed", + }); + }); + + it("reads status=all as every status", () => { + expect(parse("status=all").statuses).toEqual([]); + }); + + it("drops unknown statuses, falling back to pending when none are left", () => { + expect(parse("status=MAYBE&status=DENIED").statuses).toEqual(["DENIED"]); + expect(parse("status=MAYBE").statuses).toEqual(["PENDING"]); + }); + + it("falls back to defaults for malformed page, page_size and sort", () => { + expect(parse("page=0&page_size=7&sort_by=name&sort_order=asc")).toEqual( + DEFAULT_STATE, + ); + expect(parse("page=abc").page).toBe(1); + // A lone sort_order has no meaning without its sort_by. + expect(parse("sort_order=asc").sortKey).toBe("newest"); + }); +}); + +describe("toMergeReviewParams", () => { + it("writes a bare query string for the default view", () => { + expect(serialize(DEFAULT_STATE)).toBe(""); + }); + + it("writes every non-default part in a fixed order", () => { + expect( + serialize({ + page: 2, + pageSize: 10, + statuses: ["DENIED", "PENDING"], + sortKey: "oldest", + }), + ).toBe( + "page=2&page_size=10&status=PENDING&status=DENIED&sort_by=created&sort_order=asc", + ); + }); + + it("writes an empty status selection as status=all", () => { + expect(serialize({ ...DEFAULT_STATE, statuses: [] })).toBe("status=all"); + }); + + it("round-trips through parse", () => { + const state = { + page: 4, + pageSize: 100, + statuses: [], + sortKey: "recently-reviewed", + }; + expect(parse(serialize(state))).toEqual(state); + }); +}); + +describe("toMergeCandidateQuery", () => { + it("maps the view to the list endpoint's params", () => { + expect( + toMergeCandidateQuery({ + page: 2, + pageSize: 50, + statuses: ["PENDING", "DENIED"], + sortKey: "recently-reviewed", + }), + ).toEqual({ + page: 2, + page_size: 50, + status: ["PENDING", "DENIED"], + sort_by: "reviewed_at", + sort_order: "desc", + }); + }); + + it("sends no status filter when every status is wanted", () => { + expect( + toMergeCandidateQuery({ ...DEFAULT_STATE, statuses: [] }).status, + ).toBeUndefined(); + }); +}); diff --git a/deployments/stitch-frontend/src/hooks/useMergeCandidateName.js b/deployments/stitch-frontend/src/hooks/useMergeCandidateName.js deleted file mode 100644 index 43bd957b..00000000 --- a/deployments/stitch-frontend/src/hooks/useMergeCandidateName.js +++ /dev/null @@ -1,10 +0,0 @@ -import { useMergeSourceDetails } from "./useMergeSourceDetails"; -import { pickCandidateName } from "../utils/mergeCandidateName"; - -// Resolves a merge candidate's display name from its source resources, using -// source-priority rules. Returns null while loading or if no source has a name. -export function useMergeCandidateName(endpoint, resourceIds) { - const { data: details } = useMergeSourceDetails(endpoint, resourceIds); - - return details ? pickCandidateName(details) : null; -} diff --git a/deployments/stitch-frontend/src/hooks/useMergeReviewState.js b/deployments/stitch-frontend/src/hooks/useMergeReviewState.js new file mode 100644 index 00000000..99961429 --- /dev/null +++ b/deployments/stitch-frontend/src/hooks/useMergeReviewState.js @@ -0,0 +1,39 @@ +/** + * Reads and writes the Merge Review queue view state from the URL. + * + * Mirrors useListState: the URL is the single source of truth, and every + * setter round-trips the complete state through the schema + * (config/mergeReviewParams.js). Refining the view (statuses, sort) rewrites + * the current history entry; paging pushes, so Back steps through pages. + * Page size is not remembered across visits, unlike the resources list. + */ +import { useSearchParams } from "react-router"; +import { + parseMergeReviewParams, + toMergeReviewParams, +} from "../config/mergeReviewParams"; +import { DEFAULT_PAGE } from "../queries/resources"; + +export function useMergeReviewState() { + const [searchParams, setSearchParams] = useSearchParams(); + const state = parseMergeReviewParams(searchParams); + + function write(changes, { replace }) { + setSearchParams(toMergeReviewParams({ ...state, ...changes }), { + replace, + }); + } + + return { + ...state, + // `replace` is for corrections the reviewer did not ask for (stepping + // back from a page past the end), which should not add a Back step. + setPage: (page, { replace = false } = {}) => write({ page }, { replace }), + setPageSize: (pageSize) => + write({ pageSize, page: DEFAULT_PAGE }, { replace: false }), + setStatuses: (statuses) => + write({ statuses, page: DEFAULT_PAGE }, { replace: true }), + setSortKey: (sortKey) => + write({ sortKey, page: DEFAULT_PAGE }, { replace: true }), + }; +} diff --git a/deployments/stitch-frontend/src/hooks/useMergeSourceDetails.js b/deployments/stitch-frontend/src/hooks/useMergeSourceDetails.js deleted file mode 100644 index 7ceeb2c3..00000000 --- a/deployments/stitch-frontend/src/hooks/useMergeSourceDetails.js +++ /dev/null @@ -1,21 +0,0 @@ -import { useConfig } from "../config/useConfig"; -import { useAuthenticatedQuery } from "./useAuthenticatedQuery"; -import { getResourceDetail } from "../queries/api"; - -// Fetches every source resource in a merge candidate as one aggregate query, so -// all entries arrive (and error) together. Callers share a cache entry keyed by -// endpoint + resource ids, so the source comparison table and the candidate -// name lookup don't issue duplicate requests for the same candidate. -export function useMergeSourceDetails(endpoint, resourceIds, enabled = true) { - const config = useConfig(); - const ids = resourceIds ?? []; - - return useAuthenticatedQuery({ - queryKey: [endpoint, "merge-source-details", ...ids], - queryFn: (fetcher) => - Promise.all( - ids.map((id) => getResourceDetail(config, id, fetcher, endpoint)), - ), - enabled: enabled && ids.length > 0, - }); -} diff --git a/deployments/stitch-frontend/src/hooks/useMergedResourceDetail.js b/deployments/stitch-frontend/src/hooks/useMergedResourceDetail.js index 1d064807..f7b1dfc9 100644 --- a/deployments/stitch-frontend/src/hooks/useMergedResourceDetail.js +++ b/deployments/stitch-frontend/src/hooks/useMergedResourceDetail.js @@ -2,10 +2,9 @@ import { useConfig } from "../config/useConfig"; import { useAuthenticatedQuery } from "./useAuthenticatedQuery"; import { getResourceDetail } from "../queries/api"; -// Fetches the resource a merge produced. Callers share a cache entry keyed by -// endpoint + resource id, so the merged resource view and the review panel -// heading don't issue duplicate requests. Disabled until a resource id exists -// (i.e. the candidate has actually been merged). +// Fetches the resource a merge produced, cached by endpoint + resource id. +// Disabled until a resource id exists (i.e. the candidate has actually been +// merged). export function useMergedResourceDetail(endpoint, resourceId) { const config = useConfig(); diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx index 01d03cc1..51fd6bf9 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx @@ -1,28 +1,32 @@ -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { Link } from "react-router"; import Button from "../components/Button"; +import FilterDropdown from "../components/FilterDropdown"; import MergeSourceComparison from "../components/MergeSourceComparison"; import MergedResourceView from "../components/MergedResourceView"; +import Pagination from "../components/Pagination"; +import { + MERGE_STATUSES, + QUEUE_SORT_OPTIONS, + toMergeCandidateQuery, +} from "../config/mergeReviewParams"; import { useDocumentTitle } from "../hooks/useDocumentTitle"; -import { useMergeCandidateName } from "../hooks/useMergeCandidateName"; -import { useMergedResourceDetail } from "../hooks/useMergedResourceDetail"; +import { useMergeReviewState } from "../hooks/useMergeReviewState"; import { useMergeCandidates, useMergeCandidate, useReviewMergeCandidate, } from "../hooks/useResources"; -import { pickCompareName } from "../utils/candidateCompare"; -import { isEmptyValue } from "../utils/mergeComparison"; const ENDPOINT = "oil-gas-fields"; // Tailwind's md breakpoint, where the queue moves beside the decision panel // (the md: grid classes in the page layout below). const TWO_COLUMN_LAYOUT_QUERY = "(min-width: 48rem)"; -// One API page (its maximum size) until the queue gets paging controls. -const QUEUE_PAGE_SIZE = 200; -// Approved merges are finished work, so the queue hides them unless the -// reviewer opts in. -const UNFINISHED_STATUSES = ["PENDING", "DENIED"]; +const STATUS_FILTER_LABELS = { + PENDING: "Pending", + APPROVED: "Approved", + DENIED: "Denied", +}; function getStatusClasses(status) { if (status === "PENDING") { @@ -62,19 +66,14 @@ function candidateSourcesTitle(candidate) { return parts.join(" · "); } +// The API names each candidate (the merged resource's name once approved); +// a candidate with no licensed name falls back to its id. +function candidateDisplayName(candidate) { + return candidate.name ?? `Candidate #${candidate.id}`; +} + function CandidateQueueItem({ candidate, isSelected, onSelect }) { - const sourceName = useMergeCandidateName(ENDPOINT, candidate.resource_ids); - // Post-merge, the source resources are null shells, so the merged resource - // is the authoritative name source. The hook is disabled until an id exists, - // so pending candidates skip the fetch. - const { data: mergedResource } = useMergedResourceDetail( - ENDPOINT, - candidate.merged_resource_id, - ); - const mergedName = isEmptyValue(mergedResource?.data?.name) - ? null - : mergedResource.data.name; - const displayName = mergedName ?? sourceName ?? `Candidate #${candidate.id}`; + const displayName = candidateDisplayName(candidate); return ( + {compact && ( + + Page {page.toLocaleString()} of {totalPages.toLocaleString()} + + )} + {slots.map((slot, i) => slot.ellipsis ? ( { ).toMatch(/(^|\s)flex-wrap(\s|$)/); }); }); + + describe("compact", () => { + it("shows the page position as text instead of page buttons", () => { + const { container } = renderPagination({ + page: 2, + totalCount: 143, + totalPages: 6, + compact: true, + }); + + expect(screen.getByText("Page 2 of 6")).toBeInTheDocument(); + expect(pageButtons(container)).toHaveLength(0); + expect(screen.getByText("Showing 26–50 of 143")).toBeInTheDocument(); + }); + + it("still steps with the arrows", async () => { + const user = userEvent.setup(); + renderPagination({ + page: 2, + totalCount: 143, + totalPages: 6, + compact: true, + }); + + await user.click(screen.getByLabelText("Previous page")); + await user.click(screen.getByLabelText("Next page")); + + expect(onPageChange.mock.calls).toEqual([[1], [3]]); + }); + + it("formats large page counts with separators", () => { + renderPagination({ + page: 12345, + totalCount: 1_000_000, + totalPages: 40_000, + compact: true, + }); + + expect(screen.getByText("Page 12,345 of 40,000")).toBeInTheDocument(); + }); + }); }); diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx index 51fd6bf9..529f6a21 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx @@ -219,6 +219,7 @@ function QueuePanel({ totalPages={candidatePage.total_pages} onPageChange={handlePageChange} onPageSizeChange={viewState.setPageSize} + compact /> ) : null} From 5bbf18965b37a735ac333ae483006d1a90ba0e95 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 15:33:14 +0200 Subject: [PATCH 5/9] chore: rm PLAN.md --- PLAN.md | 118 -------------------------------------------------------- 1 file changed, 118 deletions(-) delete mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md deleted file mode 100644 index 8eda2705..00000000 --- a/PLAN.md +++ /dev/null @@ -1,118 +0,0 @@ -# Paginate the Merge Review candidate list (STIT-786) - -## Context - -`GET /api/v1/oil-gas-fields/merge-candidates` returns every candidate of every status as a bare array. The Merge Review page renders all of them, and each queue row fetches the full detail of every resource in its candidate just to show a name. After one link-all pass on production data (about 750 candidates), page load means one large list response plus about 1,500 detail requests, all rendered without virtualization. - -**Jira:** **STIT-786 "Add paging to Merge Review page"** already exists (Backlog, reporter jhoffart, no description). We'll use it. Related tickets: -- **STIT-795** (sort control, Jason, Selected for Development). This plan covers its API and UI for *date* sorts. Alphabetical sort is deferred to a follow-up (see below). The user is coordinating with Jason. -- **STIT-745** (search, filter, sort, "virtualize or paginate"). This plan covers part of it: pagination and the status filter. - -**Done when:** -1. The page fetches one page of candidates at a time, filtered to PENDING by default. -2. Queue rows render names with no per-row detail requests. -3. The header still shows whole-queue Pending, Reviewed and Total counts. -4. Status, page, page size and sort live in the URL. -5. Entity-linkage de-dupe still sees every candidate. -6. `make check` passes. - -## Decisions (agreed) - -- **Paging:** page-number paging using the existing `PaginationParams` / `PaginatedResponse` and the `Pagination` component. -- **Status filter:** a server-side, repeatable `status` param. No param returns all statuses. The frontend sends `status=PENDING` by default. The "Show approved" checkbox is replaced by a multi-select `FilterDropdown` (PENDING / APPROVED / DENIED), like the resource filters. -- **Counts:** add `status_counts` to the response. It's computed over the whole queue, ignoring the status filter, so the header numbers keep their current meaning. -- **Names:** the server computes `name` per candidate in this change. Name *sort* goes to a follow-up ticket. -- **Sort:** `sort_by ∈ {created, reviewed_at}` and `sort_order ∈ {asc, desc}`, default `created desc`, always with an `id` tiebreak. Today's order has no tiebreak, so bulk-created rows could shift between pages. - -## API: `deployments/api` - -1. **`entities.py`** - - Add `MergeCandidateQueryParams(PaginationParams, …)` with `status: list[MergeCandidateStatus] | None`, `sort_by: Literal["created", "reviewed_at"] = "created"` and `sort_order = "desc"`, following the `OGFieldQueryParams` pattern (entities.py:101-131). - - Add `name: str | None` to `MergeCandidateView`. `MergeCandidateDetailView` inherits it. - - Add `MergeCandidatePage(PaginatedResponse[MergeCandidateView])` with `status_counts: dict[MergeCandidateStatus, int]`. -2. **`db/merge_candidate_actions.py`: `list_merge_candidates`** (line 203) - - Accept params and `licensed_sources`. Apply the `status IN (…)` filter, then count, then `ORDER BY , id ` with `.limit/.offset`, following `og_field_resource_actions.py:57-90`. - - `reviewed_at` is null for pending rows; put nulls last. - - Run one `GROUP BY status` count query, unfiltered. - - Fill in names in one batch: collect the page's resource ids and call `coalesce_resources(session, ids, licensed_sources)` (`db/utils.py:198`). Per candidate, pick the name whose provenance source ranks best in `SOURCE_PRIORITY`, with ties going to item position. This mirrors `pickCandidateName` (frontend `utils/mergeCandidateName.js:12`). - - Have the detail view use the same helper so names agree. - - Keep the `named_query` labels (`merge_candidates.list`, plus `.count` and `.status_counts`). -3. **`routers/oil_gas_fields.py:102`** - - Set `response_model=MergeCandidatePage` and bind `Annotated[MergeCandidateQueryParams, Query()]`. - - Pass `licensed_sources(claims)` through, the same way the resource list does. -4. **No migration in this change.** At about 750 rows, scanning on `status` and `created` is fine. If `merge_candidates.list` timings say otherwise, add an index on `(status, created, id)` later. -5. **`API_REFERENCE.md:89-97`:** update the response shape and params. - -## Client and entity-linkage (same PR; the response shape changes) - -- **`packages/stitch-client/.../async_client.py:430`** - - Replace `list_merge_candidates` with `list_merge_candidates_page(page, page_size, status=None)`, which returns a dict. - - Add `iter_merge_candidates(...)`, mirroring `iter_oil_gas_fields` (lines 170+) and `_validate_page_params`. -- **`deployments/entity-linkage/.../client.py:78` and `matching.py:182` `_existing_fingerprints`:** iterate every page with no status filter, at `MAX_PAGE_SIZE`, so de-dupe still covers reviewed candidates. - -## Frontend: `deployments/stitch-frontend/src` - -1. **`queries/api.js:192` `getMergeCandidates`:** accept `{page, pageSize, status[], sortBy, sortOrder}` and serialize `status` as repeated params, the way the resource filters do. -2. **`queries/resources.js:44-50, 122-132`** - - Include the params in the list query key, keeping the `[endpoint, "merge-candidates", …]` prefix so review invalidation still works. - - Add `placeholderData: keepPreviousData`, matching the resource list. -3. **`hooks/useResources.js:107` and mocks at 326:** pass the params through. Update the mock to return the paged shape. -4. **URL state:** add a merge-review-specific parse/serialize pair (page, page_size, status, sort_by, sort_order), styled after `config/listParams.js`. - - It's a separate schema because `useListState` / `listParams` is tied to resource filters and columns. Generalizing it would be a refactor nobody asked for. - - Default view is `status=PENDING`, page 1, page size 50, created desc, with defaults left out of the URL. -5. **`pages/MergeCandidateReviewPage.jsx`** - - **Queue rendering** - - Render `data.items`. - - Replace the `showApproved` checkbox and the in-browser `visibleCandidates` filter (430-435) with a `FilterDropdown` for status. - - Add a sort control (created / reviewed, asc / desc). - - Put `` at the bottom of the sticky `QueuePanel`. It must fit the `md:max-h` flex column, so the list scrolls and the pager stays visible. - - **Header and names** - - Header counts come from `status_counts` (459-463, 517-536). - - `CandidateQueueItem` uses `candidate.name` and drops `useMergeCandidateName` / per-row `useMergeSourceDetails`. - - Check whether `useMergedResourceDetail` per row is still needed for merged rows. If so, keep it; it's only for approved rows, and those are filtered out by default. - - **Selection** - - Default selection stays "first PENDING on this page, else first item." - - After a review, choose the next pending item from the current page's items. - - If the page empties after refetch and `page > total_pages`, step back to the last page. - - Clear the selection when the filter, sort or page changes and the selected id isn't in the new page. - - **`hasHiddenApproved` (575-577):** replace with an empty-state message when the filter returns nothing, for example "No candidates match these statuses." -6. **Cleanup:** remove `useMergeCandidateName`, and `pickCandidateName` with its test, only if nothing else uses them after the change. The detail panel's source comparison still uses `useMergeSourceDetails`. - -## Tests - -- **API unit** (`tests/test_merge_candidate_actions.py`): status filter, the id tiebreak in ordering, `status_counts` ignoring the filter, name selection by source priority, and a null name when nothing is licensed. -- **API integration** (`tests/routers/test_merge_candidates_integration.py`, reusing `_create_candidate`): - - paging across pages with identical `created` (no duplicates or gaps) - - repeated `status` params - - 422 on a bad `status`, `sort_by` or `page_size`, following `test_query_param_validation.py` -- **Update existing tests:** `test_route_permissions.py:139` (expects `[]`), `observability/test_query_name_actions.py:372`, `test_openapi.py`. -- **Client and linkage:** `stitch-client/tests/test_async_client.py` (iterator ends on `total_pages`), `entity-linkage/tests/test_matching.py` (fingerprints span multiple pages). -- **Frontend:** - - `MergeCandidateReviewPage.test.jsx`: - - adapt the fixtures to the paged shape - - header counts come from `status_counts` - - the status dropdown changes the query params - - paging and URL round-trips - - next-pending after review on a page boundary - - names render with no `getResourceDetail` calls from the queue - - Also update `queries/resources.test.js` (key prefix) and `queries/api.test.js` (param serialization). - -## Suggested PR split (small, reviewable) - -1. **API, client, entity-linkage, and a minimal frontend adaptation.** The frontend reads `.items` and sends `status=PENDING`, replacing the in-browser filter. The shape change breaks old callers, so these ship together. -2. **Frontend UI:** status dropdown, sort control, pager, URL state, server names in rows. - -## Follow-up ticket to draft - -- **"Sort Merge Review candidates by name"** (covers STIT-795's alphabetical requirement). It needs a SQL ranking of coalesced names across all candidates for each user's licensed sources. - -## Verification - -- `make check` from the repo root. -- `make dev-docker`, seed with `004-merge-demo.json`, and run entity-linkage `link_all` to create candidates. On `/merge-candidate-review`: - - Only pending candidates show by default, and the header counts match the database. - - The network panel shows one list request and no per-row `/detail` requests. - - Paging, status and sort changes update the URL, and Back restores the previous view. - - Approving the last pending item on a page moves the selection correctly. -- Re-run `link_all` and confirm it creates no duplicate candidates, so de-dupe still sees every page. -- For scale, set a small `page_size` (for example 2) to exercise paging with the demo data. From 3bc5e2c73c804e2ddc8e266e25bca1c2df34dd6a Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 15:45:55 +0200 Subject: [PATCH 6/9] drop per-status counts from the Merge Review Status filter (STIT-786) No other dropdown in the app shows counts, so the Status filter introduced a new UI pattern. The header's Pending / Reviewed / Total counts still come from status_counts. Co-Authored-By: Claude Opus 5.5 --- .../src/pages/MergeCandidateReviewPage.jsx | 3 --- .../src/pages/MergeCandidateReviewPage.test.jsx | 13 ------------- 2 files changed, 16 deletions(-) diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx index 529f6a21..5da9e39a 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx @@ -99,7 +99,6 @@ function CandidateQueueItem({ candidate, isSelected, onSelect }) { function QueueControls({ statuses, - statusCounts, onStatusesChange, sortKey, onSortKeyChange, @@ -107,7 +106,6 @@ function QueueControls({ const statusOptions = MERGE_STATUSES.map((status) => ({ value: status, label: STATUS_FILTER_LABELS[status], - count: statusCounts?.[status], })); return ( @@ -171,7 +169,6 @@ function QueuePanel({

Queue

{ expect(queueItems()).toHaveLength(3); }); - it("shows each status's queue-wide count in the Status filter", async () => { - const user = userEvent.setup(); - renderWithQueryClient(); - - await user.click( - within(queue()).getByRole("button", { name: /^Status/ }), - ); - - expect( - screen.getByRole("checkbox", { name: /Denied/ }).closest("label"), - ).toHaveTextContent(/^Denied\s*1$/); - }); - it("counts all candidates in the header regardless of the filter", async () => { const user = userEvent.setup(); renderWithQueryClient(); From 4d41100f60f984418922b1e96c7426516e994c6f Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 15:48:22 +0200 Subject: [PATCH 7/9] fix(frontend): handle empty Merge Review pages (STIT-786) An empty result reports total_pages 0, so a page past the end was only corrected when there were results: a link to ?page=4 with no matches stayed on page 4. Treat page 1 as the last page of an empty result. An empty page also left the previous selection in the decision panel, offering review actions for a candidate the queue was not showing. Clear the selection when a loaded page is empty. Addresses Copilot review feedback on #310. Co-Authored-By: Claude Opus 5.5 --- .../src/pages/MergeCandidateReviewPage.jsx | 21 +++++++++----- .../pages/MergeCandidateReviewPage.test.jsx | 29 +++++++++++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx index 5da9e39a..623e0a63 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx @@ -478,11 +478,12 @@ export default function MergeCandidateReviewPage() { // Reviewing the last candidate on the last page (or a link to a page that no // longer exists) leaves the page past the end; step back to the new last - // page instead of showing an empty queue. Replaces the history entry, since - // the empty page was never something the reviewer chose. - const lastPage = candidatePage?.total_pages ?? 0; + // page instead of showing an empty queue. An empty result has no pages, so + // page 1 counts as its last. Replaces the history entry, since the empty + // page was never something the reviewer chose. + const lastPage = Math.max(candidatePage?.total_pages ?? 0, 1); const pastLastPage = - !listIsPlaceholder && lastPage > 0 && viewState.page > lastPage; + Boolean(candidatePage) && !listIsPlaceholder && viewState.page > lastPage; const { setPage } = viewState; useEffect(() => { if (pastLastPage) setPage(lastPage, { replace: true }); @@ -494,10 +495,16 @@ export default function MergeCandidateReviewPage() { // paint and without a cascading re-render. Skipped while the previous // page's rows stand in for the next, so it never lands on a row that is // about to disappear. + // An empty page clears the selection instead, so the panel never offers + // review actions for a candidate the queue is not showing. const selectionOnPage = pageCandidates?.some((c) => c.id === selectedId); - if (pageCandidates?.length && !selectionOnPage && !listIsPlaceholder) { - const firstPending = pageCandidates.find((c) => c.status === "PENDING"); - setSelectedId(firstPending?.id ?? pageCandidates[0].id); + if (pageCandidates && !selectionOnPage && !listIsPlaceholder) { + if (pageCandidates.length) { + const firstPending = pageCandidates.find((c) => c.status === "PENDING"); + setSelectedId(firstPending?.id ?? pageCandidates[0].id); + } else if (selectedId !== null) { + setSelectedId(null); + } } const candidateQuery = useMergeCandidate( diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx index 6378a53a..daff9860 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx @@ -749,6 +749,35 @@ describe("MergeCandidateReviewPage", () => { expect(queueItems()).toHaveLength(3); }); + it("returns to page 1 when a page past the end has no results", async () => { + mockQueue([]); + renderWithQueryClient(, { + initialEntries: ["/?page=4"], + }); + + await waitFor(() => expect(lastQueueRequest().page).toBe(1)); + }); + + it("clears the selection when the page comes back empty", async () => { + const user = userEvent.setup(); + mockQueue([pendingCandidate, deniedCandidate]); + renderWithQueryClient(); + + expect( + screen.getByRole("heading", { name: "Bergan" }), + ).toBeInTheDocument(); + + // Only approved candidates, of which there are none. + await toggleStatus(user, "Approved"); + await user.click(screen.getByRole("checkbox", { name: /Pending/ })); + + expect(lastQueueRequest().status).toEqual(["APPROVED"]); + expect(screen.getByText("Select a candidate.")).toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Approve merge" }), + ).not.toBeInTheDocument(); + }); + it("selects the first pending candidate on the page", () => { mockQueue([approvedCandidate, deniedCandidate, pendingCandidate]); renderWithQueryClient(, { From 7b4c589351a4706ecc64dbafe7dd4deed38fde5d Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 18:31:02 +0200 Subject: [PATCH 8/9] make the queue's selects match the Status filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Merge Review sort select and the pager's per-page select were native controls (browser chevron, regular weight, and a shorter per-page box) next to the Status FilterDropdown button. Add a Select component that keeps the native onPageSizeChange(Number(e.target.value))} - className="rounded-md border border-line bg-panel px-2 py-1 text-sm text-ink focus:border-primary focus:outline-none focus:ring-2 focus:ring-primary/20" > {PAGE_SIZE_OPTIONS.map((size) => ( ))} - + ); diff --git a/deployments/stitch-frontend/src/components/Pagination.test.jsx b/deployments/stitch-frontend/src/components/Pagination.test.jsx index 298c80c5..0d7013f4 100644 --- a/deployments/stitch-frontend/src/components/Pagination.test.jsx +++ b/deployments/stitch-frontend/src/components/Pagination.test.jsx @@ -236,9 +236,11 @@ describe("Pagination", () => { expect(button.className).toMatch(/(^|\s)shrink-0(\s|$)/); } - for (const span of container.querySelectorAll( - 'span[aria-hidden="true"]', - )) { + const ellipses = [ + ...container.querySelectorAll('span[aria-hidden="true"]'), + ].filter((span) => span.textContent === "…"); + expect(ellipses.length).toBeGreaterThan(0); + for (const span of ellipses) { expect(span.className).toMatch(/(^|\s)shrink-0(\s|$)/); } diff --git a/deployments/stitch-frontend/src/components/Select.jsx b/deployments/stitch-frontend/src/components/Select.jsx new file mode 100644 index 00000000..6287873b --- /dev/null +++ b/deployments/stitch-frontend/src/components/Select.jsx @@ -0,0 +1,22 @@ +// A native + {children} + + + + ); +} diff --git a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx index 349f23c5..569b7421 100644 --- a/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx +++ b/deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx @@ -5,6 +5,7 @@ import FilterDropdown from "../components/FilterDropdown"; import MergeSourceComparison from "../components/MergeSourceComparison"; import MergedResourceView from "../components/MergedResourceView"; import Pagination from "../components/Pagination"; +import Select from "../components/Select"; import { MERGE_STATUSES, QUEUE_SORT_OPTIONS, @@ -120,18 +121,17 @@ function QueueControls({ - + ); } From 10e634c3a994478c07d658a9ec86352dc01248b0 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 2 Oct 2026 19:09:05 +0200 Subject: [PATCH 9/9] tighten the Select's right padding so the queue sort fits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ▼ glyph's 32px gutter made the sort select 169px, 1px more than the room beside the Status filter in the 20rem queue column, so it wrapped. Use 24px with the glyph 10px from the edge: 161px, and the same 6px text-to-glyph gap as the FilterDropdown button. Co-Authored-By: Claude Opus 5.5 --- deployments/stitch-frontend/src/components/Select.jsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/deployments/stitch-frontend/src/components/Select.jsx b/deployments/stitch-frontend/src/components/Select.jsx index 6287873b..93e62eeb 100644 --- a/deployments/stitch-frontend/src/components/Select.jsx +++ b/deployments/stitch-frontend/src/components/Select.jsx @@ -1,19 +1,21 @@ // A native {children}