Replace users offset pagination with keyset pagination (4.2.1)#349
Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideAdds a detailed execution plan document for replacing the unpaginated GET /api/v1/users response with keyset pagination powered by the workspace pagination crate, including constraints, risks, milestones, and the proposed interfaces and handler/adapter changes but without changing production code yet. Sequence diagram for keyset-paginated GET_api_v1_users requestsequenceDiagram
actor Client
participant Handler as list_users_handler
participant Pagination as pagination_crate
participant UsersQuery as UsersQuery
participant DieselUsersQuery as DieselUsersQuery
participant UserRepo as UserRepository
participant DB as PostgreSQL
Client->>Handler: HTTP GET /api/v1/users?cursor&limit
Handler->>Pagination: parse PageParams from query
Pagination-->>Handler: PageParams { cursor, limit }
alt cursor present
Handler->>Pagination: Cursor_decode<UserCursorKey>(cursor)
Pagination-->>Handler: UserCursorKey or error
opt decode error
Handler-->>Client: HTTP 400 invalid_cursor
end
end
Handler->>UsersQuery: list_users_page(authenticated_user_id, ListUsersPageRequest)
UsersQuery->>DieselUsersQuery: list_users_page(...)
DieselUsersQuery->>UserRepo: list_page(ListUsersPageRequest)
UserRepo->>DB: SELECT users ORDER BY created_at,id LIMIT limit+1
DB-->>UserRepo: Vec<User>
UserRepo-->>DieselUsersQuery: Vec<User>
DieselUsersQuery->>DieselUsersQuery: determine has_more, truncate to limit
DieselUsersQuery-->>UsersQuery: UsersPage { rows, has_more }
UsersQuery-->>Handler: UsersPage { rows, has_more }
Handler->>Pagination: build PaginationLinks from request_url, params, has_more
Pagination-->>Handler: PaginationLinks { self, next, prev }
Handler-->>Client: HTTP 200 Paginated<UserSchema> JSON
Class diagram for updated users pagination domain and portsclassDiagram
class User {
+UserId id
+String display_name
+DateTime_Utc created_at
+DateTime_Utc updated_at
+created_at() DateTime_Utc
+id() UserId
}
class UserCursorKey {
+DateTime_Utc created_at
+Uuid id
}
UserCursorKey ..> User : From<&User>
class ListUsersPageRequest {
+Cursor_UserCursorKey cursor
+usize limit
}
class UsersPage {
+Vec_User rows
+bool has_more
}
class UsersQuery {
<<interface>>
+list_users(authenticated_user~UserId~) Result_Vec_User_Error
+list_users_page(authenticated_user~UserId~, request~ListUsersPageRequest~) Result_UsersPage_Error
}
class UserRepository {
<<interface>>
+upsert(user~User~) Result_void_UserPersistenceError
+find_by_id(id~UserId~) Result_User_UserPersistenceError
+list_page(request~ListUsersPageRequest~) Result_Vec_User_UserPersistenceError
}
class DieselUsersQuery {
+list_users(authenticated_user~UserId~) Result_Vec_User_Error
+list_users_page(authenticated_user~UserId~, request~ListUsersPageRequest~) Result_UsersPage_Error
}
class DieselUserRepository {
+upsert(user~User~) Result_void_UserPersistenceError
+find_by_id(id~UserId~) Result_User_UserPersistenceError
+list_page(request~ListUsersPageRequest~) Result_Vec_User_UserPersistenceError
}
class PageParams {
+Option_String cursor
+Option_usize limit
}
class Cursor_UserCursorKey {
+encode() String
+decode(input~String~) Result_Cursor_UserCursorKey_Error
}
class Paginated_UserSchema {
+Vec_UserSchema data
+usize limit
+PaginationLinks links
}
class PaginationLinks {
+String self_
+Option_String next
+Option_String prev
+from_request(url~Url~, params~PageParams~, next_cursor~Option_Cursor_UserCursorKey~, prev_cursor~Option_Cursor_UserCursorKey~) PaginationLinks
}
class UserSchema {
+Uuid id
+String displayName
+DateTime_Utc createdAt
}
class list_users_handler {
+list_users(state~HttpState~, session~SessionContext~, request~HttpRequest~, params~PageParams~) ApiResult_Paginated_UserSchema
}
UsersQuery <|.. DieselUsersQuery
UserRepository <|.. DieselUserRepository
list_users_handler --> UsersQuery : uses
list_users_handler --> PageParams : parses
list_users_handler --> Cursor_UserCursorKey : decodes
list_users_handler --> Paginated_UserSchema : returns
DieselUsersQuery --> UserRepository : uses
UserRepository --> ListUsersPageRequest : parameter
DieselUsersQuery --> UsersPage : returns
Cursor_UserCursorKey --> UserCursorKey : wraps
Paginated_UserSchema --> PaginationLinks
Paginated_UserSchema --> UserSchema
UserSchema --> User : derived_from
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
SummaryReplaces the unpaginated GET /api/v1/users array response with keyset-based pagination built on the workspace-local pagination crate, implementing backend roadmap task 4.2.1. The change is documented in the new execplan (docs/execplans/backend-4-2-1-replace-users-offset-pagination-with-new-crate.md), which specifies opaque cursor tokens, (created_at ASC, id ASC) ordering, limit semantics, and a paginated envelope with conditional next/prev links. Core implementation
Validation
Notes & reviewer guidance
WalkthroughImplement keyset pagination for GET /api/v1/users: add created_at to User, introduce UserCursorKey/ListUsersPageRequest/UsersPage ports, add HTTP parsing and paginated response builders, implement Diesel keyset listing using a (created_at,id) index, update OpenAPI and tests, and add BDD scenarios and docs. ChangesUsers List Keyset Pagination
Sequence Diagram (high-level request flow) sequenceDiagram
participant Client
participant HTTPHandler
participant UsersQuery
participant DieselRepo
participant PostgresDB
Client->>HTTPHandler: GET /api/v1/users?cursor&limit
HTTPHandler->>UsersQuery: list_users_page(authenticated_user, request)
UsersQuery->>DieselRepo: list_page(ListUsersPageRequest)
DieselRepo->>PostgresDB: SELECT ... ORDER BY created_at,id LIMIT N+1
PostgresDB-->>DieselRepo: rows
DieselRepo-->>UsersQuery: Vec<User>
UsersQuery-->>HTTPHandler: UsersPage(rows, has_more)
HTTPHandler-->>Client: 200 Paginated response with data, limit, links
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
lodyai
Bot
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationbackend/src/outbound/persistence/diesel_user_repository.rs: What lead to degradation?The module contains 2 functions with similar structure: list_page_after,list_page_before Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationbackend/src/outbound/persistence/diesel_users_query.rs: What lead to degradation?The module contains 2 functions with similar structure: tests.list_users_page_trims_forward_overflow_row,tests.list_users_page_trims_reverse_overflow_row Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationbackend/src/inbound/http/users_pagination.rs: What lead to degradation?The module contains 2 functions with similar structure: next_cursor,prev_cursor Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationbackend/tests/users_list_pagination_bdd/flow_support.rs: What lead to degradation?The module contains 3 functions with similar structure: run_authenticated_request,run_first_page,run_unauthenticated_request Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Draft the execution plan for backend roadmap task 4.2.1: replace the unpaginated `Vec<User>` response on `GET /api/v1/users` with the workspace `pagination` crate's keyset envelope, ordered by `(created_at, id)` and backed by a new composite Diesel index. The plan splits the work into five gated milestones (crate dependency, schema migration, domain/port additions, Diesel adapter, handler plus behavioural tests), captures hexagonal-architecture constraints, records the design calls (additive `created_at` on `User`, new `UserCursorKey` in the domain layer, extended rather than replaced `UsersQuery` port), and signposts the relevant docs and skills. Status: DRAFT — implementation will not start until the plan is approved.
Make the backend crate depend on the workspace `pagination` crate so the users endpoint can adopt the shared keyset pagination types in the next implementation step. Record the M0 gate results and the transient embedded PostgreSQL fixture startup failure in the execplan before moving on to behavioural changes.
Create `idx_users_created_at_id` so the planned users pagination query can walk the `users` table in `(created_at, id)` order without relying on an offset scan. Record the completed M1 gate evidence in the execplan.
Expose `created_at` on `User` and normalise it to PostgreSQL microsecond precision so persisted values compare cleanly after a round trip. Add the users cursor key, paginated request/page port types, and a fixture implementation for the new query method. Keep the first port change additive with default errors until the Diesel keyset query lands in the next milestone.
Add the Diesel `list_page` implementation over `(created_at, id)` with `limit + 1` fetch semantics for next and previous cursors. Trim the overflow row in `DieselUsersQuery` so callers receive a stable `UsersPage` with `has_more` while repository rows stay in ascending order. Cover forward and reverse page boundaries with unit and embedded PostgreSQL tests.
Switch `GET /api/v1/users` to decode opaque users cursors, call the paginated users query port, and return the shared pagination envelope. Add endpoint-local limit validation so oversized users pages return the project error schema while the generic pagination crate keeps its existing normalisation contract. Update OpenAPI schema tokens and existing handler, startup, and guardrail tests for the new `data` envelope.
Add embedded-PostgreSQL BDD coverage for the paginated users list endpoint. Cover first-page links, forward traversal, reverse traversal, oversized limits, invalid cursors, and unauthenticated requests. Mark roadmap task 4.2.1 complete and record the final gate evidence in the execplan.
Update the execplan to reflect that the implementation branch now has a draft review PR. Keep the living plan aligned with the final handoff state.
Export the local Cargo, Bun, Python, Go, and workspace binary paths from the Makefile so non-login hook environments can run the same format and lint targets as interactive shells. Record the hook failure and restricted-path validation replay in the users pagination execplan.
Replace the duplicated forward and reverse overflow trimming tests with one `rstest` table that covers both cursor directions. Keep the same expected page boundaries while reducing repeated fixture setup.
Replace the duplicated next and previous users cursor helpers with one direction-aware boundary helper. Keep the existing pagination emission rules while sharing the cursor encoding call.
Extract the repeated authenticated and unauthenticated users list request setup in the pagination BDD flow helpers. Keep the multi-step traversal helpers separate because they exercise distinct pagination journeys.
Replace the duplicated next and previous users repository page queries with one direction-aware keyset helper. Preserve the existing predicates, sort orders, and reverse handling for previous-page results.
Update pnpm security overrides for vulnerable transitive packages reported by `make audit`, including `basic-ftp` and `ip-address`. Regenerate `pnpm-lock.yaml` so the audit gate resolves patched versions.
lodyai
Bot
force-pushed
the
4-2-1-replace-users-offset-pagination-with-new-crate
branch
from
f4f3ebe to
3c06b1d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c06b1d7ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/domain/user.rs (1)
246-307: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd Rustdoc examples for the new public APIs.
Document the public methods introduced or changed at Line 247, Line 256, Line 283, and Line 305 with clear usage and outcome examples.
📝 Proposed doc update pattern
impl User { /// Build a new [`User`] from validated components. + /// + /// # Examples + /// + /// ```rust + /// use backend::domain::{DisplayName, User, UserId}; + /// use chrono::{TimeZone, Utc}; + /// + /// let id = UserId::new("11111111-1111-1111-1111-111111111111").expect("valid id"); + /// let name = DisplayName::new("Ada Lovelace").expect("valid display name"); + /// let created_at = Utc.with_ymd_and_hms(2026, 5, 1, 12, 0, 0).single().expect("valid ts"); + /// let user = User::new(id, name, created_at); + /// assert_eq!(user.created_at(), created_at); + /// ``` pub fn new(id: UserId, display_name: DisplayName, created_at: DateTime<Utc>) -> Self { ... } /// Build a new [`User`] from validated components with the current time. + /// + /// # Examples + /// + /// ```rust + /// use backend::domain::{DisplayName, User, UserId}; + /// let id = UserId::new("11111111-1111-1111-1111-111111111111").expect("valid id"); + /// let name = DisplayName::new("Ada Lovelace").expect("valid display name"); + /// let user = User::with_current_timestamp(id, name); + /// assert!(!user.id().as_ref().is_empty()); + /// ``` pub fn with_current_timestamp(id: UserId, display_name: DisplayName) -> Self { ... } /// Fallible constructor enforcing invariants with an explicit timestamp. + /// + /// # Examples + /// + /// ```rust + /// use backend::domain::User; + /// use chrono::{TimeZone, Utc}; + /// let created_at = Utc.with_ymd_and_hms(2026, 5, 1, 12, 0, 0).single().expect("valid ts"); + /// let user = User::try_from_strings_at( + /// "11111111-1111-1111-1111-111111111111", + /// "Ada Lovelace", + /// created_at, + /// ).expect("valid user"); + /// assert_eq!(user.created_at(), created_at); + /// ``` pub fn try_from_strings_at(...) -> Result<Self, UserValidationError> { ... } /// Timestamp when the user was first created. + /// + /// # Examples + /// + /// ```rust + /// # use backend::domain::User; + /// # let user = User::from_strings("11111111-1111-1111-1111-111111111111", "Ada Lovelace"); + /// let _created_at = user.created_at(); + /// ``` pub fn created_at(&self) -> DateTime<Utc> { ... } }As per coding guidelines, "Function documentation must include clear examples demonstrating usage and outcome".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/domain/user.rs` around lines 246 - 307, Add Rustdoc examples for the public APIs: User::new, User::with_current_timestamp, User::try_from_strings_at (and the related try_from_strings) and User::created_at by adding an "Examples" section (/// # Examples) above each method showing minimal, compileable usage with imports (use backend::domain::{User, UserId, DisplayName}; and chrono where needed), constructing valid inputs (UserId::new, DisplayName::new or from_strings helper), calling the method, and asserting expected outcome (e.g. created_at equality for User::new/try_from_strings_at, non-empty id for with_current_timestamp, and using a hidden setup line for created_at example). Ensure examples use real types and error handling (.expect(...)) so they compile as doctests and keep them short.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/migrations/2026-05-01-000000_add_users_created_at_id_index/up.sql`:
- Line 3: Replace the blocking CREATE INDEX with an online-safe build: use
CREATE INDEX CONCURRENTLY idx_users_created_at_id ON users (created_at, id) and
ensure this migration is executed non-transactionally (CONCURRENTLY cannot run
inside a transaction), e.g., mark the migration as non-transactional in your
migration runner or set the migration file to disable transactions so the index
build does not block writes.
In `@backend/src/domain/ports/user_repository.rs`:
- Around line 39-40: Change ListUsersPageRequest to encode the non-zero
invariant by using NonZeroUsize: replace the limit: usize field with limit:
NonZeroUsize and update the constructor ListUsersPageRequest::new to no longer
accept a raw usize (or, if you must accept usize, return Result and validate) —
ideally make the signature pub fn new(cursor: Option<Cursor<UserCursorKey>>,
limit: NonZeroUsize) -> Self so callers must call
NonZeroUsize::new(limit).ok_or(...) or otherwise handle the zero case; update
all call sites that construct ListUsersPageRequest to convert/validate the usize
before calling new and remove any silent acceptance of limit == 0.
In `@backend/src/inbound/http/users_pagination.rs`:
- Around line 66-109: Add Rustdoc "/// # Examples" blocks to the public API
items UsersPageDirection, parse_users_page_params, and
build_users_page_response: for UsersPageDirection show simple construction and
pattern match outcomes; for parse_users_page_params show a sample
UsersListQueryParams (cursor and limit) and the expected returned tuple types
(PageParams, ListUsersPageRequest, UsersPageDirection) with the outcome
described; for build_users_page_response show a sample
HttpRequest/PageParams/UsersPage input and the expected Paginated<User> envelope
structure (including how cursors and next/prev links map to UsersPageDirection).
Keep examples short, use realistic values for cursor/limit, reference the exact
symbols PageParams, ListUsersPageRequest, UsersPageDirection,
ListUsersPageRequest, build_users_page_response, and Paginated<User>, and ensure
the examples compile or clearly document the expected result.
In `@backend/src/outbound/persistence/diesel_users_query.rs`:
- Around line 285-316: The test list_users_page_trims_overflow_row is not
exercising the forward-cursor path because the Direction::Next case sets
cursor_index to None; change the test cases so the Direction::Next branch
supplies a concrete cursor_index (use the same index values used for Prev) and
construct the request with request_with_cursor(&rows[i], direction, limit)
instead of ListUsersPageRequest::new(None, limit) so page_direction() is invoked
for a real forward cursor; update the #[case(...)] tuple for Direction::Next to
provide an appropriate Some(usize) index and keep using request_with_cursor in
the match arm.
In `@backend/tests/diesel_user_repository.rs`:
- Around line 308-354: Test currently uses unique created_at values so it
doesn't exercise the id tie-breaker in list_page_keyset(); update the users
fixture created via paginated_user to include at least one pair with the same
created_at (e.g., make "Ada Two" and "Ada Three" share "2026-01-02T00:00:00Z"),
keep their ids different, then adjust the expected slices/assertions after
calling repo.list_page (and when constructing
Cursor::with_direction(UserCursorKey::from(...), Direction::Next/Prev)) to
reflect the stable traversal order by (created_at, id) so both Next and Prev
cursors assert the deterministic order across that tie — use the existing
symbols paginated_user, users, repo.upsert, repo.list_page,
Cursor::with_direction, UserCursorKey and Direction to locate and change the
test.
In `@backend/tests/features/users_list_pagination.feature`:
- Around line 33-36: Update the "Users list requires a session" scenario so the
unauthorised assertion also verifies a trace identifier is present in the 401
response envelope: modify the step "Then the users response is unauthorised" (or
add a new step immediately after it) to assert that the response includes a
trace id (e.g. response.header['x-trace-id'] exists and is non-empty, or
response.body.traceId / response.body.trace_id exists and is non-empty) so the
test locks the auth error contract and observability behaviour.
In `@docs/backend-roadmap.md`:
- Around line 232-234: Update the roadmap line to reference the versioned API
route string: replace the `GET /api/users` reference with `GET /api/v1/users` so
the task description matches the implemented endpoint and avoids migration
drift; ensure the bullet text still mentions offset pagination replacement,
Diesel filters using `(created_at, id)` ordering, and bb8 connection pooling
unchanged.
In
`@docs/execplans/backend-4-2-1-replace-users-offset-pagination-with-new-crate.md`:
- Around line 432-433: The line instructing readers to add the dependency is
stale; update the docs to either remove that sentence or rewrite it to state
that the dependency pagination = { path = "crates/pagination" } is already
present in Cargo.toml (or present historically) and rephrase into
historical/contextual language, ensuring the docs copy follows the project's
documentation guideline that the docs are the source of truth.
---
Outside diff comments:
In `@backend/src/domain/user.rs`:
- Around line 246-307: Add Rustdoc examples for the public APIs: User::new,
User::with_current_timestamp, User::try_from_strings_at (and the related
try_from_strings) and User::created_at by adding an "Examples" section (/// #
Examples) above each method showing minimal, compileable usage with imports (use
backend::domain::{User, UserId, DisplayName}; and chrono where needed),
constructing valid inputs (UserId::new, DisplayName::new or from_strings
helper), calling the method, and asserting expected outcome (e.g. created_at
equality for User::new/try_from_strings_at, non-empty id for
with_current_timestamp, and using a hidden setup line for created_at example).
Ensure examples use real types and error handling (.expect(...)) so they compile
as doctests and keep them short.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d15ff6ba-163f-45c9-b49a-80e08515ef6c
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
Makefilebackend/Cargo.tomlbackend/migrations/2026-05-01-000000_add_users_created_at_id_index/down.sqlbackend/migrations/2026-05-01-000000_add_users_created_at_id_index/up.sqlbackend/src/doc.rsbackend/src/domain/example_data.rsbackend/src/domain/mod.rsbackend/src/domain/ports/example_data_seed_repository.rsbackend/src/domain/ports/mod.rsbackend/src/domain/ports/user_profile_query.rsbackend/src/domain/ports/user_repository.rsbackend/src/domain/ports/users_query.rsbackend/src/domain/user.rsbackend/src/domain/user/tests.rsbackend/src/domain/user_onboarding.rsbackend/src/domain/users_pagination.rsbackend/src/inbound/http/mod.rsbackend/src/inbound/http/schemas.rsbackend/src/inbound/http/users.rsbackend/src/inbound/http/users/tests.rsbackend/src/inbound/http/users_pagination.rsbackend/src/inbound/ws/messages.rsbackend/src/outbound/persistence/diesel_example_data_seed_repository.rsbackend/src/outbound/persistence/diesel_login_service.rsbackend/src/outbound/persistence/diesel_user_repository.rsbackend/src/outbound/persistence/diesel_users_query.rsbackend/src/outbound/persistence/models.rsbackend/tests/adapter_guardrails/harness_defaults.rsbackend/tests/adapter_guardrails/steps.rsbackend/tests/diesel_login_users_adapters.rsbackend/tests/diesel_user_repository.rsbackend/tests/features/users_list_pagination.featurebackend/tests/ports_behaviour.rsbackend/tests/user_state_startup_modes_bdd.rsbackend/tests/users_list_pagination_bdd.rsbackend/tests/users_list_pagination_bdd/flow_support.rsdocs/backend-roadmap.mddocs/execplans/backend-4-2-1-replace-users-offset-pagination-with-new-crate.mdpackage.json
Remove the default `UsersQuery::list_users_page` implementation so missing pagination support fails at compile time instead of returning a runtime internal error. Make the adapter guardrail users test double implement the paginated method explicitly so it follows the tightened trait contract.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/domain/ports/users_query.rs`:
- Around line 37-53: Public API docs are missing Rustdoc examples for pagination
helpers; add concrete "/// # Examples" blocks showing usage and expected outcome
for the methods rows, into_rows, has_more and the function list_users_page. For
each item (UserPage::rows, UserPage::into_rows, UserPage::has_more and
list_users_page) add a short runnable example that constructs a sample page or
calls list_users_page, demonstrates the method call, and asserts the expected
result (e.g., lengths, contents, and has_more boolean) so cargo doc shows usage
and outcome; reference the symbols rows, into_rows, has_more, and
list_users_page when editing their doc comments.
In `@backend/tests/adapter_guardrails/doubles_users.rs`:
- Around line 96-105: The two methods in RecordingUsersQuery (the async
list_users and the other users-related method referenced around lines 107-119)
duplicate the logic that records the incoming authenticated_user into self.calls
and matches on self.response to return either Ok(users) or Err(error); extract
that shared behavior into a private helper on RecordingUsersQuery (e.g., a fn
take_users_response(&self, authenticated_user: &UserId) -> Result<Vec<User>,
Error> or a generic helper that returns the cloned UsersResponse) which locks
and pushes to self.calls, clones and returns the response, and then have both
list_users and the other method call that helper and map the UsersResponse into
the appropriate Result; ensure the helper uses the same self.calls and
self.response locks and reuses the UsersResponse::Ok/Err matching so the public
methods are just one-liners delegating to it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d384bcdc-f8dc-41ce-9804-3a396f43fcbc
📒 Files selected for processing (2)
backend/src/domain/ports/users_query.rsbackend/tests/adapter_guardrails/doubles_users.rs
Apply the still-valid review findings for users keyset pagination: build the users index concurrently, encode page limits with `NonZeroUsize`, strengthen cursor and trace-id coverage, and update the related documentation. Add the requested Rustdoc examples while keeping the existing module size gate satisfied.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. backend/tests/adapter_guardrails/doubles_users.rs Comment on lines +107 to +119 async fn list_users_page(
&self,
authenticated_user: &UserId,
_request: ListUsersPageRequest,
) -> Result<UsersPage, Error> {
self.calls
.lock()
.expect("users calls lock")
.push(authenticated_user.to_string());
match self.response.lock().expect("users response lock").clone() {
UsersResponse::Ok(users) => Ok(UsersPage::new(users, false)),
UsersResponse::Err(error) => Err(error),
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Extract the shared response path in `RecordingUsersQuery` so the list and paginated list methods record calls and map configured responses through one helper.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add runnable Rustdoc examples for the users page accessors and paginated query method so the public pagination port shows expected usage and results.
1 participant
Summary
This branch implements backend roadmap task 4.2.1 by replacing the raw
GET /api/v1/usersarray response with a keyset-paginated envelope built onthe workspace
paginationcrate. The endpoint now orders users by(created_at, id), follows opaque direction-aware cursor tokens, returnsself,next, andprevlinks, and rejects endpoint-specific malformedpagination input with structured error details.
Roadmap task: (4.2.1), now marked complete in
docs/backend-roadmap.md
Execplan:
docs/execplans/backend-4-2-1-replace-users-offset-pagination-with-new-crate.md
Status: Draft implementation PR. The execplan has been updated with progress,
decisions, validation evidence, and retrospective notes.
Review walkthrough
backend/src/inbound/http/users.rs
and
backend/src/inbound/http/users_pagination.rs
to review request parsing, cursor decoding, link construction, and error
mapping at the inbound boundary.
backend/src/domain/users_pagination.rs,
backend/src/domain/ports/users_query.rs,
and
backend/src/domain/ports/user_repository.rs
for the domain-owned cursor key and additive port changes.
backend/src/outbound/persistence/diesel_user_repository.rs
and
backend/src/outbound/persistence/diesel_users_query.rs
for the
limit + 1keyset reads, reverse-page ordering, and overflowtrimming.
backend/migrations/2026-05-01-000000_add_users_created_at_id_index/up.sql.
backend/tests/features/users_list_pagination.feature
and
backend/tests/users_list_pagination_bdd.rs.
Validation
cargo check -p backend: pass during M0, M3, and M4 focused gates.cargo test -p backend --test users_list_pagination_bdd: pass, 14 tests.make fmt: pass.make markdownlint: pass.make check-fmt: pass.make lint: pass, including Rust docs, Clippy, architecture lint, Biome,API spec validation, Makefile validation, and action lint checks.
make test: pass; nextest ran 1220 Rust tests with 1220 passed and 4skipped, followed by TypeScript checks, root Vitest, frontend workspace
Vitest, and token contrast checks.
PATH=/usr/local/bin:/usr/bin:/bin make check-fmt lint: pass.PATH=/usr/local/bin:/usr/bin:/bin make markdownlint: pass.PATH=/usr/local/bin:/usr/bin:/bin make test: pass.Notes
limitvalues abovepagination::MAX_LIMITin the inbound adapter, while the generic paginationcrate continues to normalise oversized limits for reusable callers.
User::created_atis normalised to PostgreSQL microsecond precision at thedomain boundary so cursor keys and persisted rows compare consistently.
document and keeping this roadmap task inside its planned scope.