Skip to content

Commit ede6911

Browse files
committed
fix review comments
1 parent eeb9ba0 commit ede6911

2 files changed

Lines changed: 28 additions & 12 deletions

File tree

src/services/consumer-view-v2.ts

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ export async function sendFrontendView(
333333
const query = buildResult.sql;
334334

335335
try {
336-
const { pageNumber = 1, pageSize = queryStore.totalLines, locale, cursor } = pageOptions;
336+
const { pageNumber = 1, pageSize = queryStore.totalLines, locale } = pageOptions;
337337
const lang = locale.includes('en') ? 'en-gb' : 'cy-gb';
338338
const langForHash = locale.includes('en') ? 'en-GB' : 'cy-GB';
339339

@@ -423,15 +423,29 @@ export async function sendFrontendView(
423423
let next_cursor: string | null = null;
424424
let prev_cursor: string | null = null;
425425
if (buildResult.sortPlan && buildResult.sortPlan.length > 0 && buildResult.sortHash && rowKeyValues.length > 0) {
426-
// Offset mode emits next_cursor whenever there are more rows after the
427-
// current page — it's the seam the frontend uses to swap from
428-
// page_number paging into cursor paging. Cursor mode emits next_cursor
429-
// only when there is genuinely more data ahead (the over-fetch row).
426+
// `hasMore` gates the direction the request just walked. Forward cursor
427+
// requests over-fetch ahead → hasMore means more rows further forward.
428+
// Backward requests over-fetch behind → hasMore means more rows further
429+
// backward. The opposite direction's cursor is always emittable in
430+
// cursor mode because we necessarily came from there.
431+
//
432+
// Offset mode only emits next_cursor — it's the seam the frontend uses
433+
// to swap from page_number paging into cursor paging at the page-cap
434+
// boundary. prev_cursor stays null because offset callers navigate
435+
// backwards with page_number, not a cursor.
430436
const lastKeys = rowKeyValues[rowKeyValues.length - 1];
431437
const firstKeys = rowKeyValues[0];
432-
const offsetHasMore = !isCursorMode && pageSize * pageNumber < queryStore.totalLines;
433438

434-
if ((!isCursorMode && offsetHasMore) || (isCursorMode && hasMore)) {
439+
let emitNext = false;
440+
let emitPrev = false;
441+
if (isCursorMode) {
442+
emitNext = buildResult.direction === 'b' ? true : hasMore;
443+
emitPrev = buildResult.direction === 'b' ? hasMore : true;
444+
} else {
445+
emitNext = pageSize * pageNumber < queryStore.totalLines;
446+
}
447+
448+
if (emitNext) {
435449
next_cursor = buildCursorFromRow(
436450
lastKeys,
437451
buildResult.sortPlan,
@@ -441,7 +455,7 @@ export async function sendFrontendView(
441455
buildResult.sortHash
442456
);
443457
}
444-
if (isCursorMode && cursor) {
458+
if (emitPrev) {
445459
prev_cursor = buildCursorFromRow(
446460
firstKeys,
447461
buildResult.sortPlan,

test/integration/routes/consumer-v2/data-and-pivot.test.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,6 @@ describe('Consumer V2 — data + pivot endpoints', () => {
229229
describe('cursor pagination', () => {
230230
it('frontend response includes next_cursor / prev_cursor in page_info', async () => {
231231
const res = await request(app).get(`/v2/${DATASET_ID}/data`).query({ format: 'frontend', page_size: 5 });
232-
// eslint-disable-next-line no-console
233-
console.log('DEBUG headers:', JSON.stringify(res.body.headers));
234-
// eslint-disable-next-line no-console
235-
console.log('DEBUG first row keys (data):', res.body.data?.[0]);
236232
expect(res.status).toBe(200);
237233
expect(res.body.page_info).toHaveProperty('next_cursor');
238234
expect(res.body.page_info).toHaveProperty('prev_cursor');
@@ -343,6 +339,12 @@ describe('Consumer V2 — data + pivot endpoints', () => {
343339
expect(back.status).toBe(200);
344340
// Walking back should land us on the original first-page rows
345341
expect(JSON.stringify(back.body.data)).toBe(JSON.stringify(first.body.data));
342+
// Backward-direction responses must always carry next_cursor: we came
343+
// from somewhere forward, so a forward link back to it has to exist.
344+
// The first-page boundary still nulls prev_cursor (no rows further
345+
// behind) — that's what stops the loop.
346+
expect(back.body.page_info.next_cursor).toBeTruthy();
347+
expect(back.body.page_info.prev_cursor).toBeNull();
346348
});
347349
});
348350
});

0 commit comments

Comments
 (0)