fix: close Lease and Schedule review follow-ups - #227
Conversation
smiggleworth
left a comment
There was a problem hiding this comment.
Automated review — findings below, ranked roughly by severity. Happy to follow up on any of these.
There was a problem hiding this comment.
🟡 Changes recommended
Schedule execution paging is applied over HashMap iteration order, which makes pagination/has_more potentially unstable and should be made deterministic (e.g., sort before offset/limit).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Follow-up fixes that tighten Lease/Schedule behavior after prior review: preserves typed authenticated Lease selector errors, adds operation-aware Schedule observation filtering and paging semantics, and extends the Admin UI with a Schedule operation drilldown plus updated regressions.
Changes:
- Schedule admin endpoints: add
operationfiltering,offsetpaging,has_moresignaling, and per-operationpending_handoffsin execution observations. - Admin UI: add
/schedule/.../{operation}drilldown page and update resource page to list/paginate individual operations. - Tests/adapters/docs: regenerate UI adapters from OpenAPI and expand Rust/UI/browser/service-contract coverage for the new behavior.
File summaries
| File | Description |
|---|---|
| ui/tests/service-contract-domains.test.ts | Updates service contract expectations for offset and operation-scoped schedule calls. |
| ui/tests/queries.test.ts | Adds query exports/keys for schedule operation drilldown and validates key uniqueness with paging params. |
| ui/tests/page-smoke/harness.ts | Extends query mocks to include schedule operation query state. |
| ui/tests/page-smoke/fixtures.ts | Adds schedule operation fixtures and updates schedule execution observation fixture shape (has_more, offset, pending_handoffs). |
| ui/tests/page-smoke-domains.test.tsx | Updates smoke tests for new resource-page content and operation drilldown navigation. |
| ui/tests/e2e/shell/resource-mocks.ts | Enhances schedule execution mocks to support operation, offset, and limit slicing with has_more. |
| ui/tests/e2e/shell/resource-fixtures.ts | Expands e2e fixtures to cover multiple operations and new fields. |
| ui/tests/e2e/shell/chrome.ts | Adds a route scenario for the new schedule operation page. |
| ui/tests/e2e/shell-domains.spec.ts | Updates Playwright flow to click through to schedule operation detail. |
| ui/tests/app.test.tsx | Registers the new ScheduleOperationPage in the app import smoke test. |
| ui/src/shared/navigation/domains.ts | Extends Schedule domain scope to include operation and updates scope href generation accordingly. |
| ui/src/pages/app/schedule-resource.tsx | Reworks schedule resource view to list per-operation rows and paginate via offset + has_more. |
| ui/src/pages/app/schedule-operation.tsx | New operation-scoped schedule detail page with missed-handoff table. |
| ui/src/pages/app/_routes.tsx | Registers operation-level schedule routes and route handler mapping. |
| ui/src/features/schedule/schedule-service.ts | Adds getScheduleOperation; threads offset/operation into schedule observation endpoints. |
| ui/src/features/schedule/schedule-query.ts | Adds schedule operation query + query keys; includes offset/operation in relevant keys and inputs. |
| ui/src/features/schedule/schedule-models.ts | Introduces ScheduleOperationView and extends request types with offset/operation. |
| ui/src/features/schedule/schedule-format.ts | New shared schedule param decoding + timestamp/timing formatting utilities. |
| ui/src/adapters/generated/schemas.ts | Regenerated schema types for schedule observation paging fields and pending_handoffs. |
| ui/src/adapters/generated/operations.ts | Regenerated operation query types for new schedule query params. |
| ui/src/adapters/generated/api.ts | Regenerated API client definitions to include operation/offset query params. |
| src/api/runtime_ingress/tests/authorization_routes.rs | Adds an authenticated actor helper and a fixed-depth domain authorization regression test. |
| src/api/runtime_ingress/tests/authorization_routes_lease.rs | Refactors lease malformed-selector authenticated tests to cover SUBSCRIBE/UNSUBSCRIBE/LIST with typed errors. |
| src/api/admin/list/tests.rs | Adds schedule tests verifying operation filtering happens before limit and that paging behaves after filtering. |
| src/api/admin/list/dto_operations.rs | Extends schedule DTOs with paging fields and adds explicit request DTOs for schedule observations. |
| src/api/admin/list/admin_reads/schedule.rs | Implements schedule operation filtering, paging, has_more, and per-operation pending handoff counts. |
| src/api/admin/list.rs | Re-exports new schedule request DTOs for handler usage. |
| src/api/admin/handlers/routing/hierarchical_get.rs | Builds/threads new schedule request DTOs from query params into list handlers. |
| public/openapi.yml | Documents new schedule query params and response fields for adapters and consumers. |
Review details
- Files reviewed: 26/29 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Review follow-up is complete at
|
Summary
This is a clean follow-up to merged PR #221 and closed issue #219, based on the latest
mainafter PR #226.Validation
cargo fmt --all -- --checkcargo test --locked --workspacecargo clippy --locked --workspace --all-targets --all-features -- -D warnings -D clippy::pedanticnpm cinpm run gennpm run fmt:checknpm run testnpm run lintnpm run type-checknpm run buildnpx playwright test tests/e2e/shell-domains.spec.ts --grep "navigates schedule scope"