fix: encodePaginationTokens to encode plain objects without secondary… #372
Conversation
WalkthroughA new utility function, Sequence Diagram(s)sequenceDiagram
participant Caller
participant encodePaginationTokens
participant isPlainObject
Caller->>encodePaginationTokens: Call with params and response
encodePaginationTokens->>isPlainObject: Check if 'previous' token is plain object
isPlainObject-->>encodePaginationTokens: Return true/false
encodePaginationTokens->>isPlainObject: Check if 'next' token is plain object
isPlainObject-->>encodePaginationTokens: Return true/false
encodePaginationTokens->>encodePaginationTokens: Encode tokens if plain object
encodePaginationTokens-->>Caller: Return (pagination tokens possibly encoded)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ahmed-anas
force-pushed
the
ahmed-anas/PLAT-186/fix-encode-bug
branch
from
63c3562 to
b6005bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/utils/query.test.ts (2)
52-52: Consider improving type safety by avoidingas any.The
as anytype assertion bypasses TypeScript's type checking. Consider defining a more specific type for the response object or using a type that better represents the expected structure.- } as any; + };If the TypeScript compiler requires explicit typing, consider creating a specific interface or using a more precise type annotation instead of
as any.
41-41: Consider adding a test comment for clarity.The test name is descriptive, but adding a brief comment explaining why this scenario was previously problematic would provide helpful context for future maintainers.
+ // This test verifies the fix for the issue where encoding was silently skipped + // for plain objects without _id when shouldSecondarySortOnId was true it('encodes tokens when cursor is a plain object that lacks _id', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/utils/query.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/search.ts:0-0
Timestamp: 2025-01-07T13:38:36.860Z
Learning: In the mongo-cursor-pagination project, the `next` parameter in search and pagination interfaces should use the `Encodable` type from `bsonUrlEncoding` to ensure proper typing of encoded/decoded values.
test/utils/query.test.ts (1)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/search.ts:0-0
Timestamp: 2025-01-07T13:38:36.860Z
Learning: In the mongo-cursor-pagination project, the `next` parameter in search and pagination interfaces should use the `Encodable` type from `bsonUrlEncoding` to ensure proper typing of encoded/decoded values.
🔇 Additional comments (1)
test/utils/query.test.ts (1)
41-59: Excellent test coverage for the reported bug scenario.This test case directly addresses the core issue described in the PR objectives where encoding was silently skipped for plain objects lacking
_idfields. The test logic correctly verifies that cursors containing only the paginated field are properly encoded.
|
🎉 This PR is included in version 9.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
… sort ID
📚 Context/Description Behind The Change
Reviewing the original JS code (converted to TS in this PR) we always encode when shouldSecondarySortOnId is true.
With the new guard the branch is silently skipped whenever the cursor object isn’t a plain object that passes the guard.
Two common scenarios fail:
🚨 Potential Risks & What To Monitor After Deployment
We need to test any usages of this method
🧑🔬 How Has This Been Tested?
🚚 Release Plan