Direction-aware cursors (roadmap 4.1.2)#329
Merged
Merged
Conversation
Reviewer's GuideAdd direction-aware, bidirectional cursors to the pagination crate by introducing a Direction enum, extending Cursor to store direction with backward-compatible serde defaults, updating public exports and docs, and adding rstest-based unit and rstest-bdd behavioural tests plus roadmap/architecture documentation updates. Class diagram for direction-aware Cursor and Direction enumclassDiagram
class Direction {
<<enumeration>>
Next
Prev
}
class Cursor~Key~ {
-Key key
-Direction dir
+new(key: Key) Cursor~Key~
+with_direction(key: Key, dir: Direction) Cursor~Key~
+key() Key
+direction() Direction
+into_inner() Key
+into_parts() (Key, Direction)
+encode() Result~String, CursorError~
+decode(encoded: &str) Result~Cursor~Key~, CursorError~
}
class CursorError {
<<enumeration>>
Base64
Utf8
Deserialize
}
Cursor "1" *-- "1" Direction
Cursor ..> CursorError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughAdd direction-aware cursors to the pagination crate by embedding a serde-backed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The plan is internally inconsistent on the
Cursordesign: early on it chooses "Option A" (add aDirectionfield) but later the concrete interface uses a genericCursor<Key, Dir = ()>; it would help to pick one approach, explain why, and align all examples and acceptance criteria to that single design. - The backward-compatibility story for existing serialized cursors could be made more explicit: consider specifying the exact JSON shapes before/after the change and how the serde defaults/
skip_serializing_ifbehavior ensure that old tokens (withoutdir) still decode and that new tokens don’t break older consumers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The plan is internally inconsistent on the `Cursor` design: early on it chooses "Option A" (add a `Direction` field) but later the concrete interface uses a generic `Cursor<Key, Dir = ()>`; it would help to pick one approach, explain why, and align all examples and acceptance criteria to that single design.
- The backward-compatibility story for existing serialized cursors could be made more explicit: consider specifying the exact JSON shapes before/after the change and how the serde defaults/`skip_serializing_if` behavior ensure that old tokens (without `dir`) still decode and that new tokens don’t break older consumers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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.
![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: ba5f39a5ee
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 177-183: The document currently contradicts itself about the
cursor shape—decide on Option A (preferred): a concrete Cursor<Key> with a
direction field—then update all occurrences to match: replace any generic
definitions like Cursor<Key, Dir = ()> or Cursor<Key, Dir> with the concrete
interface that includes a direction: Direction field (or dir: Direction if that
name is used elsewhere) and add the const default behavior note; ensure the
interface section and all examples/reference mentions (including the earlier
generic signatures) consistently reference Cursor<Key> with direction: Direction
and remove the generic form Cursor<Key, Dir = ()> throughout the doc.
- Around line 187-287: The markdown lists restart numbering incorrectly causing
MD029 failures; update each numbered block so numbering begins at 1 and
increments sequentially within that block (e.g., change the "3. Modify `Cursor`
struct in `cursor.rs`" block to start at "1." and adjust subsequent items to
2.,3.,4., etc.), and likewise restart numbering for the test and feature blocks
(ensure entries like "Add unit tests in `cursor.rs`", "Extend or add Gherkin
feature file `tests/features/direction_aware_cursors.feature`", and the
roadmap/decision entries each start at 1 in their own blocks); after renumbering
run make markdownlint (or make check-fmt) to verify MD029 is resolved and commit
the adjusted bullets.
- Line 290: Replace the machine-specific absolute path in the sentence "Execute
these commands from the repository root
(`/data/leynos/Projects/wildside.worktrees/backend-4-1-2-direction-aware-cursors`)."
with a portable repo-relative instruction such as "Execute these commands from
the repository root." Remove the entire absolute path portion (the backticked
path) so the document is reproducible across environments and CI.
- Around line 44-45: Update the Quality gates line and the concrete command list
in the document to include documentation-specific checks: add `make
markdownlint`, `make fmt`, and `make nixie` to the Quality gates sentence (the
line starting with "**Quality gates:**") and append those three commands to the
CI command block where `make check-fmt`, `make lint`, and `make test` are listed
(the command block containing `make check-fmt`, `make lint`, `make test`);
ensure the same additions are applied to the other occurrence mentioned (lines
372-376).
🪄 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
Run ID: c31ecbea-effe-49b4-b119-b4fbac1af1bb
📒 Files selected for processing (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity.
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18497
Issue 1 — Issue 2 — Backward-compatibility story: The plan now provides a concrete "Backward compatibility" table under Artifacts and notes with exact JSON shapes:
It also explicitly explains why One minor prose note outside the scope of your two bullets: "serialized" appears a handful of times in running text (e.g., "always include |
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.
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- You changed
Cursor::newfrom aconst fnto a non-const function; if you want to preserve const usability, you can keep itconstby directly settingdir: Direction::Nextinstead of going throughDirection::default(). - There is some duplication in the tests constructing
FixtureKeyand checking that direction defaults toNext(e.g.,cursor_without_direction_defaults_to_nextandcursor_new_defaults_to_next); consider extracting a small helper or consolidating these cases to keep the test suite leaner.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You changed `Cursor::new` from a `const fn` to a non-const function; if you want to preserve const usability, you can keep it `const` by directly setting `dir: Direction::Next` instead of going through `Direction::default()`.
- There is some duplication in the tests constructing `FixtureKey` and checking that direction defaults to `Next` (e.g., `cursor_without_direction_defaults_to_next` and `cursor_new_defaults_to_next`); consider extracting a small helper or consolidating these cases to keep the test suite leaner.
## Individual Comments
### Comment 1
<location path="docs/execplans/backend-4-1-2-direction-aware-cursors.md" line_range="543" />
<code_context>
+tests::structurally_invalid_json_cursor_fails_decode ... ok
+tests::direction_round_trips_through_encoding_next ... ok
+tests::direction_round_trips_through_encoding_prev ... ok
+tests::cursor_with_direction_defaults_to_next ... ok
+...
+test result: ok. 15 passed; 0 failed; 0 ignored
</code_context>
<issue_to_address>
**suggestion (typo):** Test name in the example output likely has a "with" vs "without" typo.
Earlier you mention a test named `cursor_without_direction_defaults_to_next` for backward compatibility, but here it appears as `cursor_with_direction_defaults_to_next`. If this is meant to be the same test, please update the name here to `cursor_without_direction_defaults_to_next` for consistency.
```suggestion
tests::cursor_without_direction_defaults_to_next ... ok
```
</issue_to_address>
### Comment 2
<location path="docs/execplans/backend-4-1-2-direction-aware-cursors.md" line_range="503" />
<code_context>
+ - Crate-level docs in `lib.rs` include Direction usage example.
+ - Architecture decision logged in `docs/wildside-backend-architecture.md`.
+
+### Quality method (how we check)
+
+```bash
</code_context>
<issue_to_address>
**issue (review_instructions):** Heading text uses the first-person pronoun "we", which the style guide prohibits.
The phrase "how we check" violates the no first-/second-person rule. Consider rephrasing to something like "Quality method (how checks are performed)" or similar neutral wording.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 3
<location path="docs/execplans/backend-4-1-2-direction-aware-cursors.md" line_range="153" />
<code_context>
+### Test coverage
+
+- 11 unit tests (7 new direction-aware tests)
+- 2 BDD test suites (1 new direction-aware feature with 3 scenarios)
+- 10 doc tests (4 new direction-related examples)
+
</code_context>
<issue_to_address>
**issue (review_instructions):** The acronym "BDD" is used here without being defined on first use.
Per the guidance, uncommon acronyms should be expanded on first use. Consider something like "behaviour-driven development (BDD) test suites" the first time BDD appears in the document.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 4
<location path="docs/execplans/backend-4-1-2-direction-aware-cursors.md" line_range="21" />
<code_context>
+
+- A `Direction` enum (`Next`, `Prev`) embedded in cursors to indicate traversal
+direction.
+- Updated `Cursor<Key>` encoding/decoding that preserves direction
+through base64url JSON serialization.
+- Property tests ensuring encode-decode round-trip stability.
</code_context>
<issue_to_address>
**issue (review_instructions):** The following line uses US spelling "serialization" instead of en-GB "serialisation".
The next line reads "through base64url JSON serialization.", which uses US spelling. To align with en-GB-oxendic spelling, this should be "serialisation" (while keeping code identifiers such as `Serialize`/`Deserialize` unchanged).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d5b594bde
ℹ️ 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".
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/crates/pagination/src/cursor.rs`:
- Around line 299-317: The test new_cursor_includes_direction_in_json only
checks that Direction::Prev is serialized into the cursor JSON, but doesn't
assert the JSON shape for the default Direction::Next; update the test to also
create a Cursor via Cursor::with_direction or default Cursor creation with
Direction::Next and assert the encoded/decoded JSON string contains a "dir"
field and the string "Next" (i.e., cover Direction::Next), so both variants
(Direction::Prev and Direction::Next) are validated by calling encode() and
base64 decode as done for Prev and asserting presence of "dir" and "Next".
In `@backend/crates/pagination/tests/features/direction_aware_cursors.feature`:
- Around line 19-22: The scenario currently claims to test the defaulting
behavior but the test in pagination_bdd.rs builds the token with
Cursor::new(...).encode(), so it already includes dir: "Next"; either rename the
scenario to indicate it encodes/decodes a Cursor created with Cursor::new, or
change the test to exercise a legacy token that genuinely omits direction by
constructing a raw payload (e.g., a JSON/map with the ordering key fields but no
"dir") and base64/opaque-encoding that string, then pass it to Cursor::decode
and assert the decoded cursor.direction == Next; update the test code around
where Cursor::new, Cursor::encode and Cursor::decode are used to implement the
chosen option.
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 245-279: The docs need to be updated to match the actual Cursor
API in backend/crates/pagination/src/cursor.rs: change the field name to dir
(not direction) and note the serde fallback uses plain #[serde(default)],
document that Cursor::new and Cursor::with_direction are const fn constructors,
and that Cursor::direction(&self) returns Direction by value (not by reference);
also reflect that Cursor::into_parts(self) returns (Key, Direction) and adjust
any example signatures/comments accordingly so the markdown matches the landed
Cursor<Key> implementation.
- Around line 620-622: The "## Revision note" block contains a stale
pre-implementation sentence ("Initial version created. Awaiting approval before
implementation.") that conflicts with the file's Status: COMPLETE; remove that
sentence or replace it with a completion note (e.g., "Version implemented and
approved" or similar) so the document reflects its COMPLETE status—locate the
"## Revision note" section and edit the paragraph under that heading
accordingly.
In `@docs/wildside-backend-architecture.md`:
- Around line 339-347: The docs now state that the `pagination` crate includes a
`Direction` enum and direction-aware cursors (with `Direction::Next` as
default), but the later "Shared pagination foundation crate (Roadmap 4.1.1)"
paragraph and section 4.2 still claim direction-aware cursors are future work;
update those paragraphs to reflect that direction-aware cursors are implemented
and 4.1.2 is complete (aligning with `docs/backend-roadmap.md`), remove the
“future work” wording, and ensure mentions of the `Direction` enum and opaque
base64url JSON cursor behavior are consistent across the file.
🪄 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
Run ID: de726436-f3c7-42fa-bccc-5ca4b577b898
📒 Files selected for processing (7)
backend/crates/pagination/src/cursor.rsbackend/crates/pagination/src/lib.rsbackend/crates/pagination/tests/features/direction_aware_cursors.featurebackend/crates/pagination/tests/pagination_bdd.rsdocs/backend-roadmap.mddocs/execplans/backend-4-1-2-direction-aware-cursors.mddocs/wildside-backend-architecture.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/execplans/backend-4-1-2-direction-aware-cursors.md (2)
596-601:⚠️ Potential issue | 🟡 MinorMark the documented constructors as
const fn.The interface block still documents
pub fn newand
pub fn with_direction, while the landed API in
backend/crates/pagination/src/cursor.rsexposes both constructors as
const fn. Keep the completed ExecPlan identical to the crate API.#!/bin/bash sed -n '596,603p' docs/execplans/backend-4-1-2-direction-aware-cursors.md echo "---" rg -n 'pub const fn new|pub const fn with_direction' backend/crates/pagination/src/cursor.rsAs per coding guidelines, “Use markdown files within the
docs/directory as the knowledge base and source of truth for project requirements, dependencies, and architectural decisions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md` around lines 596 - 601, Update the docs to match the actual API by changing the documented constructors for Cursor<Key> to be const fns: replace the signatures for Cursor::new and Cursor::with_direction in the interface block so they read as pub const fn new(key: Key) -> Self and pub const fn with_direction(key: Key, dir: Direction) -> Self, ensuring the doc example matches backend/crates/pagination/src/cursor.rs.
97-98:⚠️ Potential issue | 🟡 MinorRecord the full docs gate set in the completion sections.
The constraints and Stage E require
make markdownlint,make fmt, and
make nixie, but the progress entry, the retrospective gate summary, and the
validation command block still record only a subset of those checks. Update all
three sections so theStatus: COMPLETErecord matches the gates this plan says
are mandatory.As per coding guidelines, “Validate Markdown files using
make markdownlint”, “Runmake fmtafter any documentation changes to format all Markdown files and fix table markup”, and “Validate Mermaid diagrams in Markdown files by runningmake nixie.”Also applies to: 159-165, 504-520
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md` around lines 97 - 98, Update every "Status: COMPLETE" entry in the progress entry, retrospective gate summary, and the validation command block so they list the full docs gate set required by the plan: include make markdownlint, make fmt, and make nixie (in addition to existing make check-fmt/make lint/make test where applicable); search for occurrences of the "Status: COMPLETE" lines and the validation command blocks in this document and the similar sections referenced (also apply the same change to the other affected blocks noted) and ensure the completion list and any example validation commands explicitly run make markdownlint, make fmt, and make nixie.backend/crates/pagination/src/cursor.rs (1)
299-318: 🧹 Nitpick | 🔵 TrivialAssert the JSON field instead of scanning substrings.
This test only proves that
"dir"and the expected variant text appear
somewhere in the payload. Parse the decoded JSON and assert the top-leveldir
field so the test fails if the field is renamed, nested, or emitted in the
wrong shape.Patch sketch
- let json_str = String::from_utf8(decoded_bytes).expect("valid utf8"); - - // Verify the direction is present in the JSON - assert!(json_str.contains("\"dir\"")); - assert!(json_str.contains(expected)); + let payload: serde_json::Value = + serde_json::from_slice(&decoded_bytes).expect("valid json"); + + assert_eq!( + payload.get("dir").and_then(serde_json::Value::as_str), + Some(expected), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/crates/pagination/src/cursor.rs` around lines 299 - 318, The test new_cursor_includes_direction_in_json currently only searches for substrings in json_str; instead parse the decoded JSON (e.g., with serde_json::from_str into a serde_json::Value) and assert that the top-level "dir" field exists and equals the expected string for the Cursor created with Cursor::with_direction; replace the two assert!(json_str.contains(...)) checks with a direct check like value.get("dir").and_then(|v| v.as_str()) == Some(expected) so the test fails if the field is missing, renamed, nested, or has the wrong shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/crates/pagination/src/cursor.rs`:
- Around line 299-318: The test new_cursor_includes_direction_in_json currently
only searches for substrings in json_str; instead parse the decoded JSON (e.g.,
with serde_json::from_str into a serde_json::Value) and assert that the
top-level "dir" field exists and equals the expected string for the Cursor
created with Cursor::with_direction; replace the two
assert!(json_str.contains(...)) checks with a direct check like
value.get("dir").and_then(|v| v.as_str()) == Some(expected) so the test fails if
the field is missing, renamed, nested, or has the wrong shape.
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 596-601: Update the docs to match the actual API by changing the
documented constructors for Cursor<Key> to be const fns: replace the signatures
for Cursor::new and Cursor::with_direction in the interface block so they read
as pub const fn new(key: Key) -> Self and pub const fn with_direction(key: Key,
dir: Direction) -> Self, ensuring the doc example matches
backend/crates/pagination/src/cursor.rs.
- Around line 97-98: Update every "Status: COMPLETE" entry in the progress
entry, retrospective gate summary, and the validation command block so they list
the full docs gate set required by the plan: include make markdownlint, make
fmt, and make nixie (in addition to existing make check-fmt/make lint/make test
where applicable); search for occurrences of the "Status: COMPLETE" lines and
the validation command blocks in this document and the similar sections
referenced (also apply the same change to the other affected blocks noted) and
ensure the completion list and any example validation commands explicitly run
make markdownlint, make fmt, and make nixie.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22767acd-40ef-4d99-8eea-dd0ebe514784
📒 Files selected for processing (4)
backend/crates/pagination/src/cursor.rsbackend/crates/pagination/tests/features/direction_aware_cursors.featuredocs/execplans/backend-4-1-2-direction-aware-cursors.mddocs/wildside-backend-architecture.md
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Line 109: Replace all en-GB-oxendict spelling inconsistencies by swapping the
"-ise" variants to "-ize" across the document: change "deserialisation" to
"deserialization", "Parameterised" to "Parameterized", and both occurrences of
"deserialising" to "deserializing" (search for the exact tokens
"deserialisation", "Parameterised", "deserialising" to locate each instance).
Ensure updates are applied consistently in
docs/execplans/backend-4-1-2-direction-aware-cursors.md so the document uses
en-GB-oxendict "-ize" spellings throughout.
🪄 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
Run ID: 1e4d8d52-f197-421f-a5d5-4aac51e2c564
📒 Files selected for processing (2)
backend/crates/pagination/src/cursor.rsdocs/execplans/backend-4-1-2-direction-aware-cursors.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md (1)
516-520:⚠️ Potential issue | 🟠 MajorSynchronize the final quality-gate command block with the documented gates.
Add
make markdownlint,make fmt, andmake nixieto the “Run full quality gates” block so this section matches the constraints and completion criteria in the same document.Patch
# Run full quality gates make check-fmt make lint make test +make markdownlint +make fmt +make nixieAs per coding guidelines, “Validate Markdown files using
make markdownlint”, “Runmake fmtafter any documentation changes”, and “Validate Mermaid diagrams in Markdown files by runningmake nixie.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md` around lines 516 - 520, Update the "Run full quality gates" command block under the "Run full quality gates" heading by adding the missing commands so it matches the documented gates: include `make markdownlint`, `make fmt`, and `make nixie` alongside the existing `make check-fmt`, `make lint`, and `make test` lines; ensure the code block lists all six commands in the same style/format as the existing block so the section matches the constraints and completion criteria in the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Line 1: The H1 heading "# ExecPlan: Add Direction-Aware Cursors to Pagination
Crate" is in title case; change it to sentence case by lowercasing all words
except the initial word and any proper nouns—update the heading to "# ExecPlan:
add direction-aware cursors to pagination crate" so it follows the repository's
sentence-case guideline.
---
Duplicate comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 516-520: Update the "Run full quality gates" command block under
the "Run full quality gates" heading by adding the missing commands so it
matches the documented gates: include `make markdownlint`, `make fmt`, and `make
nixie` alongside the existing `make check-fmt`, `make lint`, and `make test`
lines; ensure the code block lists all six commands in the same style/format as
the existing block so the section matches the constraints and completion
criteria in the document.
🪄 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
Run ID: 30641e3e-89b4-4355-91fb-025f7749097a
📒 Files selected for processing (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extend the pagination crate to support bidirectional pagination via a `Direction` enum (`Next` and `Prev`) embedded in cursors. Changes: - Add `Direction` enum with `Next` (default) and `Prev` variants - Update `Cursor<Key>` struct with a `dir: Direction` field using `#[serde(default)]` for backward compatibility - Add `Cursor::with_direction()` constructor for explicit direction - Add `Cursor::direction()` accessor and `Cursor::into_parts()` destructor - Add comprehensive unit tests with `rstest` for round-trip encoding, backward compatibility, and edge cases - Add BDD feature file and step definitions for direction-aware cursor scenarios - Update crate documentation with direction-aware usage examples Backward compatibility: Cursors created before this change (without the `dir` field) deserialize with `Direction::Next` as the default, preserving existing forward pagination behaviour. New cursors always include `dir` in the serialized JSON. Quality gates: - All 28 pagination tests pass (16 unit, 2 BDD suites, 10 doc tests) - `cargo clippy -p pagination` passes with no warnings - `cargo fmt -- --check` passes - `make markdownlint` passes Roadmap item 4.1.2 is now complete.
Rename the second "Backward compatibility" heading to "Backward compatibility table" to resolve markdownlint MD024 violation.
- Make `Cursor::new` a `const fn` by using `Direction::Next` directly instead of `Direction::default()` - Rename test `cursor_new_defaults_to_next_direction` to `cursor_new_uses_next_direction` to avoid confusion with the backward compatibility test - Fix typo in execplan expected test output - Replace first-person "we" with neutral "to" in execplan heading - Define BDD acronym on first use in execplan - Fix line length violation in execplan
Add a const item to verify that `Cursor::new` works in const contexts. This ensures the API remains usable for compile-time fixtures and static initialisation.
- Update `new_cursor_includes_direction_in_json` test to cover both `Direction::Next` and `Direction::Prev` using rstest parameterised cases - Rename BDD scenario to "Cursor created with Cursor::new has direction Next" for accuracy (the old name implied testing legacy tokens without dir field) - Fix execplan API documentation: - Change field name from `direction` to `dir` - Correct serde attribute to `#[serde(default)]` - Note that constructors are `const fn` - Fix `direction()` return type to `Direction` (by value) - Update revision note to reflect COMPLETE status - Update architecture doc to indicate direction-aware cursors (4.1.2) are implemented, not future work
Add `const` to `Cursor::new` and `Cursor::with_direction` signatures in the Interface definitions section to match the actual implementation.
- Update new_cursor_includes_direction_in_json to parse JSON and assert on the "dir" field value directly instead of using substring checks - Update progress entry to include full docs gate set (make markdownlint, make fmt, make nixie)
Change "-ise" variants to "-ize" for consistency with en-GB-oxendict: - deserialisation → deserialization - Parameterised → Parameterized - deserialising → deserializing (2 occurrences)
Update the "Run full quality gates" block to include all six gates: - make check-fmt - make lint - make test - make markdownlint - make fmt - make nixie
leynos
force-pushed
the
backend-4-1-2-direction-aware-cursors
branch
from
3014ba2 to
c7e5da5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/crates/pagination/src/cursor.rs`:
- Around line 288-323: Add a test that decodes a cursor JSON with an invalid
"dir" value (e.g. {"key":{...},"dir":"Sideways"}) and assert that Cursor::decode
returns an Err(CursorError::Deserialize); locate this alongside the existing
tests in cursor.rs (near cursor_without_direction_defaults_to_next and
new_cursor_includes_direction_in_json) and use the same base64 URL_SAFE_NO_PAD
encoding/decoding flow so the failing case exercises Direction deserialization
logic in Cursor::decode and ensures invalid enum variants produce the expected
CursorError::Deserialize.
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 159-165: The "Quality gates" section in
docs/execplans/backend-4-1-2-direction-aware-cursors.md is missing several
completed gates referenced elsewhere; update the "Quality gates" list to include
the omitted gates (make lint, make test, make fmt, and make nixie) so the
closure is self-contained and matches the constraints/progress log/command
block—edit the "Quality gates" heading and add bullet lines for `make lint` –
Pass, `make test` – Pass, `make fmt` – Pass, and `make nixie` – Pass (preserving
the existing entries and formatting).
🪄 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
Run ID: f518ef80-81c5-44c4-8329-5d755145fb0b
📒 Files selected for processing (7)
backend/crates/pagination/src/cursor.rsbackend/crates/pagination/src/lib.rsbackend/crates/pagination/tests/features/direction_aware_cursors.featurebackend/crates/pagination/tests/pagination_bdd.rsdocs/backend-roadmap.mddocs/execplans/backend-4-1-2-direction-aware-cursors.mddocs/wildside-backend-architecture.md
- Add invalid_direction_value_returns_deserialize_error test to verify Cursor::decode returns CursorError::Deserialize for invalid "dir" values - Update Quality gates section in execplan to include all gates: make lint, make test, make fmt, make nixie
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md (1)
1-1:⚠️ Potential issue | 🟡 MinorRewrite the H1 in sentence case.
The title still uses title case, so the file remains out of step with the documentation style rule.
As per coding guidelines, "Write headings in sentence case."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md` at line 1, The H1 is in title case; change the heading "# ExecPlan: Add Direction-Aware Cursors to Pagination Crate" to sentence case by lowercasing non-proper words so it reads "# ExecPlan: Add direction-aware cursors to pagination crate" (keep the "ExecPlan" identifier as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/crates/pagination/src/cursor.rs`:
- Around line 123-125: Add a const-context compile-time test to ensure
Cursor::with_direction remains const: update the module tests to define a const
binding (similar to the existing const _CONST_CURSOR) that calls
Cursor::with_direction("compile-time-test", Direction::Prev) (or
Direction::Next) and assign it to a const _CONST_DIRECTIONAL_CURSOR:
Cursor<&str>; this will fail compilation if with_direction loses the const
qualifier and thereby prevent silent API regressions; ensure the test is added
alongside the existing Cursor::new const check (and repeat for other affected
overloads if present).
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 138-157: Update the retrospective block under "Files modified" to
include the two docs files `docs/backend-roadmap.md` and
`docs/wildside-backend-architecture.md` and adjust the "unit tests" tally to the
correct total that reflects the current cursor suite (update the unit-test count
referenced in the summary and any later sample output); edit the summary lines
shown in the `backend-4-1-2-direction-aware-cursors.md` content so the list of
modified files includes those two docs and the unit-test total matches the
actual number of unit tests in the pagination crate.
- Around line 290-299: Update the bullet list to match the actual test names in
cursor.rs: replace the two specific tests
`direction_round_trips_through_opaque_token` and `prev_direction_round_trips`
with the parameterized test `direction_round_trips_through_encoding` (noting it
covers both Next and Prev cases), replace
`invalid_direction_json_fails_gracefully` with
`invalid_direction_value_returns_deserialize_error`, and ensure the
backward-compatibility case is described as
"cursor_without_direction_defaults_to_next" (or mention that the parameterized
test includes a case where the serialized cursor lacks the `dir` field and
should decode to Direction::Next) so the docs reflect the implemented tests.
---
Duplicate comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Line 1: The H1 is in title case; change the heading "# ExecPlan: Add
Direction-Aware Cursors to Pagination Crate" to sentence case by lowercasing
non-proper words so it reads "# ExecPlan: Add direction-aware cursors to
pagination crate" (keep the "ExecPlan" identifier as-is).
🪄 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
Run ID: f0ae2a19-2566-4fd9-a3bd-43524d571cf1
📒 Files selected for processing (2)
backend/crates/pagination/src/cursor.rsdocs/execplans/backend-4-1-2-direction-aware-cursors.md
- Add const _CONST_DIRECTIONAL_CURSOR compile-time test to verify Cursor::with_direction remains const - Fix execplan H1 to use sentence case - Add missing docs files to Files modified list - Update unit test count from 11 to 18 - Fix test names in Stage C to match actual implementation: - direction_round_trips_through_encoding (parameterized) - invalid_direction_value_returns_deserialize_error
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md (1)
44-45:⚠️ Potential issue | 🟡 MinorRemove the double comma in the quality gates list.
Line 44 has a double comma between
`make test`,and`make markdownlint`. Delete one comma.📝 Proposed fix
-- **Quality gates:** `make check-fmt`, `make lint`, `make test`, `make -markdownlint`, `make fmt`, and `make nixie` must all pass before completion. +- **Quality gates:** `make check-fmt`, `make lint`, `make test`, + `make markdownlint`, `make fmt`, and `make nixie` must all pass before + completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md` around lines 44 - 45, The quality gates list contains a duplicate comma after the `make test` item; edit the sentence that lists the quality gates (the line containing "`make check-fmt`, `make lint`, `make test`, `make markdownlint`, `make fmt`, and `make nixie`") and remove the extra comma so it reads with a single comma between `make test` and `make markdownlint`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 539-554: The "Expected test output" block is inconsistent with the
rest of the doc (shows 15 tests but the text claims 18); update the fenced
plaintext example in the "Expected test output" section so both the "running X
tests" line and the final "test result" line reflect 18 tests (e.g., "running 18
tests" and "test result: ok. 18 passed; 0 failed; 0 ignored"), keeping the same
formatting and retaining the existing list of test names.
- Around line 153-158: Update the "Test coverage" retrospective bullet to
correct the unit test counts: replace the line currently stating "18 unit tests
(13 cursor tests including 9 direction-aware tests)" with "10 unit tests (6
direction-aware or direction-related tests)" while keeping the following BDD and
doc test bullets unchanged; look for the "Test coverage" heading and the bullets
under it in backend-4-1-2-direction-aware-cursors.md to locate and edit the
specific list item.
---
Duplicate comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 44-45: The quality gates list contains a duplicate comma after the
`make test` item; edit the sentence that lists the quality gates (the line
containing "`make check-fmt`, `make lint`, `make test`, `make markdownlint`,
`make fmt`, and `make nixie`") and remove the extra comma so it reads with a
single comma between `make test` and `make markdownlint`.
🪄 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
Run ID: 591bee61-814d-41ae-a6e5-eeae7bc34317
📒 Files selected for processing (2)
backend/crates/pagination/src/cursor.rsdocs/execplans/backend-4-1-2-direction-aware-cursors.md
- Update Expected test output block to show 18 tests with complete list of test names including parameterized cases - Update Test coverage unit test count to 10 (per review feedback)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Around line 579-582: Update the sentence under the "No `skip_serializing_if`"
bullet to avoid implying universal forward compatibility: replace "older
consumers ignore unknown fields per typical JSON conventions." with a scoped
statement like "consumers that tolerate unknown or additional JSON fields will
ignore `dir`" or "consumers that accept unknown fields will ignore it," so the
claim is limited to clients that explicitly tolerate unknown fields; adjust the
surrounding sentence if needed to keep tone consistent in the "No
`skip_serializing_if`" paragraph.
- Around line 539-560: The sample test transcript under the "Expected test
output" section conflicts with the retrospective "10 unit tests" claim; update
the document so the transcript and the stated totals match by either (A) editing
the transcript block to show "running 10 tests" and removing the extra 8 test
lines (keep representative passing lines and final summary) or (B) changing the
retrospective/count language from "10 unit tests" to "18 unit tests" so it
matches the transcript; edit the "Expected test output" block and the
retrospective/count sentence (search for the phrase "10 unit tests" or the
surrounding summary paragraph) to keep the numbers consistent.
🪄 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
Run ID: 5ac524ec-f5e3-4301-bcc3-074a1a84e4ef
📒 Files selected for processing (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md
- Scoped forward compatibility claim to consumers that tolerate unknown JSON fields (not universal) - Fixed test count from "10 unit tests" to "18 unit tests" to match the actual test transcript
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md (1)
1-1:⚠️ Potential issue | 🟡 MinorRewrite the H1 heading in sentence case.
Change the heading to lowercase after the prefix to match repository docs style.
✍️ Patch sketch
-# ExecPlan: Add direction-aware cursors to pagination crate +# ExecPlan: add direction-aware cursors to pagination crateAs per coding guidelines, “Write headings in sentence case”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md` at line 1, The H1 heading "# ExecPlan: Add direction-aware cursors to pagination crate" should be rewritten in sentence case to match repo docs style; update that heading so the prefix "ExecPlan:" stays capitalized but the remainder is sentence case (e.g., "ExecPlan: Add direction-aware cursors to pagination crate" -> "ExecPlan: Add direction-aware cursors to pagination crate" adjusted to sentence case for the part after the colon), ensuring the new heading text replaces the existing "# ExecPlan: Add direction-aware cursors to pagination crate" line in the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/execplans/backend-4-1-2-direction-aware-cursors.md`:
- Line 1: The H1 heading "# ExecPlan: Add direction-aware cursors to pagination
crate" should be rewritten in sentence case to match repo docs style; update
that heading so the prefix "ExecPlan:" stays capitalized but the remainder is
sentence case (e.g., "ExecPlan: Add direction-aware cursors to pagination crate"
-> "ExecPlan: Add direction-aware cursors to pagination crate" adjusted to
sentence case for the part after the colon), ensuring the new heading text
replaces the existing "# ExecPlan: Add direction-aware cursors to pagination
crate" line in the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d57b8748-d5a7-44ef-a00a-4eef435f2125
📒 Files selected for processing (1)
docs/execplans/backend-4-1-2-direction-aware-cursors.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create a comprehensive execution plan for implementing direction-aware cursors (
NextandPrev) in the pagination crate. The plan covers:Directionenum with serde support for bidirectional paginationCursor<Key>to include direction awareness while maintaining backward compatibilityrstestto ensure encode-decode round-trip stabilityrstest-bddfollowing project conventionsmake check-fmt,make lint,make test)The ExecPlan follows the structure defined in the execplans skill and references
docs/keyset-pagination-design.mdfor cursor semantics.Summary by Sourcery
Add direction-aware pagination cursors while preserving backward compatibility and documenting the change.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: