Add paginated item listing#8
Conversation
There was a problem hiding this comment.
Review Summary
This PR adds paginated item listing functionality, but there are 3 critical logic errors that must be fixed before merge:
Critical Issues Found
- Pagination Logic Broken: Sorting items in memory after DynamoDB pagination breaks cursor-based navigation, causing inconsistent results across pages
- Scan Without GSI: Using DynamoDB scan without a Global Secondary Index prevents proper ordered pagination and will cause data inconsistencies
- Overly Restrictive Cursor Validation: The cursor decoder rejects valid DynamoDB
LastEvaluatedKeytokens that don't match the exact{id: string}structure
Recommended Fixes
- Remove in-memory sorting and use DynamoDB's native ordering with
ScanIndexForward - Create a GSI with
createdAtas sort key for proper ordered pagination, or accept unordered results - Allow any valid key structure in cursor validation, not just
{id: string}
The error handling improvements and test coverage are solid, but the core pagination implementation needs these fixes to work correctly in production.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| const items = ((result.Items ?? []) as ItemRecord[]).sort((left, right) => | ||
| right.createdAt.localeCompare(left.createdAt), | ||
| ); |
There was a problem hiding this comment.
🛑 Logic Error: Sorting after pagination breaks cursor-based navigation. The sort occurs after DynamoDB returns paginated results, causing inconsistent ordering across pages. When the next page is fetched using the cursor, items may appear out of order or be duplicated because sorting changes which items should have been included in each page.
| const items = ((result.Items ?? []) as ItemRecord[]).sort((left, right) => | |
| right.createdAt.localeCompare(left.createdAt), | |
| ); | |
| const result = await dynamodb | |
| .scan({ | |
| TableName: requireTableName(), | |
| Limit: limit, | |
| ExclusiveStartKey: exclusiveStartKey, | |
| ScanIndexForward: false, | |
| }) | |
| .promise(); | |
| const items = (result.Items ?? []) as ItemRecord[]; |
| const result = await dynamodb | ||
| .scan({ | ||
| TableName: requireTableName(), | ||
| Limit: limit, | ||
| ExclusiveStartKey: exclusiveStartKey, | ||
| }) | ||
| .promise(); |
There was a problem hiding this comment.
🛑 Logic Error: Using scan without a GSI prevents proper ordered pagination. The cursor-based approach expects consistent ordering, but scan operations don't guarantee order. This will cause items to be missed or duplicated across pages when new items are added during pagination. Create a GSI with createdAt as the sort key or use query with proper indexes.
| try { | ||
| const decoded = JSON.parse(Buffer.from(rawCursor, 'base64url').toString('utf8')) as Partial<ItemsCursor>; | ||
|
|
||
| if (typeof decoded.id !== 'string' || decoded.id.trim() === '') { | ||
| throw new Error('Cursor does not contain a valid id'); | ||
| } | ||
|
|
||
| return { | ||
| statusCode: 200, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Access-Control-Allow-Origin': '*' | ||
| }, | ||
| body: JSON.stringify({ | ||
| message: 'Testing from Lambda!', | ||
| event, | ||
| context | ||
| }) | ||
| id: decoded.id, | ||
| }; |
There was a problem hiding this comment.
🛑 Logic Error: The decoded cursor validation rejects valid cursors without the id property. DynamoDB's LastEvaluatedKey can contain any attributes from the table schema (not just id), and the cursor validation is overly restrictive. If the cursor contains additional attributes or different key structures, valid pagination tokens will be rejected.
| try { | |
| const decoded = JSON.parse(Buffer.from(rawCursor, 'base64url').toString('utf8')) as Partial<ItemsCursor>; | |
| if (typeof decoded.id !== 'string' || decoded.id.trim() === '') { | |
| throw new Error('Cursor does not contain a valid id'); | |
| } | |
| return { | |
| statusCode: 200, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Access-Control-Allow-Origin': '*' | |
| }, | |
| body: JSON.stringify({ | |
| message: 'Testing from Lambda!', | |
| event, | |
| context | |
| }) | |
| id: decoded.id, | |
| }; | |
| try { | |
| const decoded = JSON.parse(Buffer.from(rawCursor, 'base64url').toString('utf8')) as Record<string, unknown>; | |
| if (!decoded || typeof decoded !== 'object') { | |
| throw new Error('Invalid cursor format'); | |
| } | |
| return decoded as AWS.DynamoDB.DocumentClient.Key; |
…' into feature/platform-product-surface
Summary
GET /itemswithlimit,cursor, andnextCursorsupport.400responses.Validation
npm run buildNODE_OPTIONS=--max-old-space-size=4096 npm test -- --runInBandNotes
Unrelated local changes in
lib/cdk-app-stack.ts,.aidevops/, and.codexwere left out of the pushed commit.