fix(stats): track last read separately from progress row updates

Sorting by "Last Read" ordered on reading_progress.updated_at, but the KOReader sync path
deliberately freezes that column so it stays a reliable "last local write" marker when resolving
conflicts against device progress. Readers who sync from KOReader therefore kept the timestamp of
their first ever sync of each book forever, so the smart scope sort, the jump-bucket rails and the
dashboard Continue Reading list all looked unordered.

Add reading_progress.last_read_at, which advances on every write unless a path opts out, and move
every "last read" reader onto it. Leave updated_at to the conflict-resolution and progress-resume
paths that genuinely mean "last local write". Kobo bookmark-location stamping opts out because
recording which Location a bookmark maps to is bookkeeping rather than reading, while the Kobo
progress write and the migration importer pass their source timestamps through so imported history
is not stamped as read today.

Move the ebook-versus-audiobook recency tiebreaks onto the new column as well. They compared a
frozen timestamp against a live one, which selected the wrong percentage for the same reason.

Swap the (user_id, updated_at) index for (user_id, last_read_at), since the ordering consumer that
justified it moved. Backfill last_read_at from updated_at in the migration, as the generated
DEFAULT now() would otherwise make every existing book look as though it had just been read.

Fixes #1013
This commit is contained in:
neon
2026-08-16 01:04:34 +00:00
parent d7556011df
commit 7b43756258
21 changed files with 16286 additions and 25 deletions
@@ -0,0 +1,24 @@
import { drizzle } from 'drizzle-orm/node-postgres';
import * as schema from '../../db/schema';
export type CapturedQuery = { sql: string; params: unknown[] };
/**
* Drizzle instance over a stub client that records the compiled SQL instead of running it.
*
* A hand-mocked `db` only sees the arguments a repository passes in, but column defaults and
* `$onUpdateFn` columns are injected later, by the dialect. Timestamp behaviour is therefore
* invisible to those mocks and has to be asserted against what Postgres would actually receive.
*/
export function createCapturingDb(): { db: ReturnType<typeof drizzle>; queries: CapturedQuery[] } {
const queries: CapturedQuery[] = [];
const client = {
query: (query: unknown, params?: unknown[]) => {
const text = typeof query === 'string' ? query : ((query as { text?: string })?.text ?? '');
queries.push({ sql: text, params: params ?? [] });
return Promise.resolve({ rows: [], fields: [], rowCount: 0 });
},
};
return { db: drizzle(client as never, { schema }), queries };
}
@@ -0,0 +1,4 @@
DROP INDEX "reading_progress_user_updated_at_idx";--> statement-breakpoint
ALTER TABLE "reading_progress" ADD COLUMN "last_read_at" timestamp with time zone DEFAULT now() NOT NULL;--> statement-breakpoint
UPDATE "reading_progress" SET "last_read_at" = "updated_at";--> statement-breakpoint
CREATE INDEX "reading_progress_user_last_read_at_idx" ON "reading_progress" USING btree ("user_id","last_read_at");
File diff suppressed because it is too large Load Diff
@@ -470,6 +470,13 @@
"when": 1786683893541,
"tag": "0066_add_koreader_device_retirements",
"breakpoints": true
},
{
"idx": 67,
"version": "7",
"when": 1786840894954,
"tag": "0067_add_reading_progress_last_read_at",
"breakpoints": true
}
]
}
+13
View File
@@ -35,6 +35,19 @@ describe('reader schema', () => {
expect(readingProgress.updatedAt.onUpdateFn?.()).toBeInstanceOf(Date);
});
it('tracks last read separately from the row update timestamp', () => {
const config = getTableConfig(readingProgress);
const indexNames = config.indexes.map((idx) => idx.config.name);
// Ordering by "last read" needs an always-advancing column: updated_at is deliberately
// frozen by the KOReader sync path so it can act as a "last local write" marker.
expect(readingProgress.lastReadAt.notNull).toBe(true);
expect(readingProgress.lastReadAt.hasDefault).toBe(true);
expect(readingProgress.lastReadAt.onUpdateFn?.()).toBeInstanceOf(Date);
expect(indexNames).toContain('reading_progress_user_last_read_at_idx');
expect(indexNames).not.toContain('reading_progress_user_updated_at_idx');
});
it('stores reading sessions with idempotency and time indexes', () => {
const config = getTableConfig(readingSessions);
const uniqueIndexes = config.indexes.filter((idx) => idx.config.unique);
+11 -1
View File
@@ -165,15 +165,25 @@ export const readingProgress = pgTable(
koboContentSourceProgressPercent: real('kobo_content_source_progress_percent'),
// KOReader: XPointer progress string generated from the EPUB DOM
koreaderProgress: text('koreader_progress'),
// Last time this row was written by the web reader or Kobo. The KOReader sync path
// deliberately freezes it so it stays a reliable "last local write" marker to compare
// against koreader_device_progress.updatedAt, so it is NOT a last-read timestamp.
updatedAt: timestamp('updated_at', { withTimezone: true })
.notNull()
.defaultNow()
.$onUpdateFn(() => new Date()),
// Last time the user actually read this file, from any client. Advances on every write
// unless a path opts out, because some writes (Kobo location stamping) are bookkeeping
// rather than reading. This is the column "last read" ordering must use.
lastReadAt: timestamp('last_read_at', { withTimezone: true })
.notNull()
.defaultNow()
.$onUpdateFn(() => new Date()),
},
(t) => [
primaryKey({ columns: [t.bookFileId, t.userId] }),
index('reading_progress_user_id_idx').on(t.userId),
index('reading_progress_user_updated_at_idx').on(t.userId, t.updatedAt),
index('reading_progress_user_last_read_at_idx').on(t.userId, t.lastReadAt),
check('reading_progress_percentage_range_chk', sql`${t.percentage} >= 0 and ${t.percentage} <= 100`),
check('reading_progress_page_number_nonnegative_chk', sql`${t.pageNumber} is null or ${t.pageNumber} >= 0`),
check('reading_progress_position_seconds_nonnegative_chk', sql`${t.positionSeconds} is null or ${t.positionSeconds} >= 0`),
@@ -1769,6 +1769,9 @@ describe('BookQueryBuilder.buildCollapseOrderBy', () => {
expect(result).toContain('rp.user_id = 7');
expect(result).toContain('bf.book_id = r.id');
expect(result).toContain('ASC NULLS LAST');
// Must aggregate last_read_at; rp.updated_at is frozen by the KOReader sync path.
expect(result).toContain('SELECT max(rp.last_read_at)');
expect(result).not.toContain('max(rp.updated_at)');
});
it('generates user-scoped subquery for finishedAt', () => {
@@ -836,9 +836,9 @@ export class BookQueryBuilder {
private upNextInSeriesSql(userId: number): SQL {
const mergedProgress = sql`coalesce(
case
when rp.updated_at is null then ab.percentage
when rp.last_read_at is null then ab.percentage
when ab.updated_at is null then rp.percentage
when rp.updated_at >= ab.updated_at then rp.percentage
when rp.last_read_at >= ab.updated_at then rp.percentage
else ab.percentage
end,
rp.percentage,
@@ -1126,7 +1126,7 @@ export class BookQueryBuilder {
break;
case 'lastReadAt':
parts.push(
`(SELECT max(rp.updated_at) FROM reading_progress rp INNER JOIN book_files bf ON rp.book_file_id = bf.id WHERE bf.book_id = r.id AND rp.user_id = ${safeUserId}) ${D} NULLS LAST`,
`(SELECT max(rp.last_read_at) FROM reading_progress rp INNER JOIN book_files bf ON rp.book_file_id = bf.id WHERE bf.book_id = r.id AND rp.user_id = ${safeUserId}) ${D} NULLS LAST`,
);
break;
case 'finishedAt':
@@ -160,6 +160,17 @@ describe('BookSortBuilder', () => {
);
});
// reading_progress.updated_at is frozen by the KOReader sync path, so ordering on it left
// KOReader-only readers with an effectively random "Last Read" order.
it('orders lastReadAt on last_read_at rather than the row update timestamp', () => {
const result = service.build([{ field: 'lastReadAt', dir: 'desc' }], 42);
const text = (result[0] as unknown as { text: string }).text;
expect(text).toContain('SELECT max(rp.last_read_at)');
expect(text).not.toContain('rp.updated_at');
expect(result[0]?.values[0]).toBe(42);
});
it('throws for finishedAt sort without userId', () => {
expect(() => service.build([{ field: 'finishedAt', dir: 'asc' }])).toThrow(
new BadRequestException('finishedAt sort requires an authenticated user'),
@@ -93,7 +93,7 @@ export class BookSortBuilder {
case 'lastReadAt':
if (userId === undefined) throw new BadRequestException('lastReadAt sort requires an authenticated user');
result.push(
sql`(SELECT max(rp.updated_at) FROM reading_progress rp INNER JOIN book_files bf ON rp.book_file_id = bf.id WHERE bf.book_id = books.id AND rp.user_id = ${userId}) ${sql.raw(D)} NULLS LAST`,
sql`(SELECT max(rp.last_read_at) FROM reading_progress rp INNER JOIN book_files bf ON rp.book_file_id = bf.id WHERE bf.book_id = books.id AND rp.user_id = ${userId}) ${sql.raw(D)} NULLS LAST`,
);
break;
case 'finishedAt':
@@ -1171,6 +1171,8 @@ describe('BookRepository', () => {
const query = dialect.sqlToQuery(execute.mock.calls[0]![0]).sql;
expect(query).toContain('rail_last_read AS MATERIALIZED');
expect(query).toContain('max(rail_rp.last_read_at)');
expect(query).not.toContain('rail_rp.updated_at');
expect(query).toContain('GROUP BY rail_bf.book_id');
expect(query).toContain('base_rows AS MATERIALIZED');
expect(query).toContain('representatives AS');
+5 -5
View File
@@ -322,7 +322,7 @@ function temporalSourceParts(field: SortField, userId: number, timeZone: string,
case 'lastReadAt':
return {
prefixCte: sql`rail_last_read AS MATERIALIZED (
SELECT rail_bf.book_id, max(rail_rp.updated_at) AS value
SELECT rail_bf.book_id, max(rail_rp.last_read_at) AS value
FROM ${readingProgress} rail_rp
INNER JOIN ${bookFiles} rail_bf ON rail_bf.id = rail_rp.book_file_id
WHERE rail_rp.user_id = ${userId}
@@ -361,7 +361,7 @@ function temporalSourceParts(field: SortField, userId: number, timeZone: string,
case 'lastReadAt':
return {
prefixCte: sql`rail_last_read AS MATERIALIZED (
SELECT rail_bf.book_id, max(rail_rp.updated_at) AS value
SELECT rail_bf.book_id, max(rail_rp.last_read_at) AS value
FROM ${readingProgress} rail_rp
INNER JOIN ${bookFiles} rail_bf ON rail_bf.id = rail_rp.book_file_id
WHERE rail_rp.user_id = ${userId}
@@ -683,11 +683,11 @@ export class BookRepository {
.select({
bookFileId: readingProgress.bookFileId,
percentage: readingProgress.percentage,
updatedAt: readingProgress.updatedAt,
lastReadAt: readingProgress.lastReadAt,
})
.from(readingProgress)
.where(and(eq(readingProgress.userId, userId), inArray(readingProgress.bookFileId, primaryFileIds)))
: Promise.resolve([] as { bookFileId: number; percentage: number; updatedAt: Date }[]),
: Promise.resolve([] as { bookFileId: number; percentage: number; lastReadAt: Date }[]),
this.db
.select({
bookId: audiobookProgress.bookId,
@@ -709,7 +709,7 @@ export class BookRepository {
const mergedPercentage =
fileProgress && audioProgress
? fileProgress.updatedAt >= audioProgress.updatedAt
? fileProgress.lastReadAt >= audioProgress.updatedAt
? fileProgress.percentage
: audioProgress.percentage
: (fileProgress?.percentage ?? audioProgress?.percentage ?? null);
@@ -83,8 +83,8 @@ export class DashboardWidgetRepository {
coalesce(
(
case
when ${readingProgress.updatedAt} is not null
and (${audiobookProgress.updatedAt} is null or ${readingProgress.updatedAt} >= ${audiobookProgress.updatedAt})
when ${readingProgress.lastReadAt} is not null
and (${audiobookProgress.updatedAt} is null or ${readingProgress.lastReadAt} >= ${audiobookProgress.updatedAt})
then ${readingProgress.percentage}
else ${audiobookProgress.percentage}
end
@@ -94,7 +94,7 @@ export class DashboardWidgetRepository {
0
)
`;
const mergedLastReadAt = sql<Date | null>`coalesce(${readingProgress.updatedAt}, ${audiobookProgress.updatedAt})`;
const mergedLastReadAt = sql<Date | null>`coalesce(${readingProgress.lastReadAt}, ${audiobookProgress.updatedAt})`;
const rows = await this.db
.select({
@@ -120,7 +120,7 @@ export class DashboardWidgetRepository {
...cfClauses,
),
)
.orderBy(desc(sql`coalesce(${readingProgress.updatedAt}, ${audiobookProgress.updatedAt}, ${userBookStatus.updatedAt})`))
.orderBy(desc(sql`coalesce(${readingProgress.lastReadAt}, ${audiobookProgress.updatedAt}, ${userBookStatus.updatedAt})`))
.limit(CURRENTLY_READING_LIMIT);
if (rows.length === 0) return { books: [] };
@@ -63,7 +63,7 @@ export class DashboardRepository {
...cfClauses,
),
)
.orderBy(desc(readingProgress.updatedAt), desc(books.id))
.orderBy(desc(readingProgress.lastReadAt), desc(books.id))
.limit(limit);
return rows.map((row) => row.id);
@@ -134,9 +134,9 @@ export class DashboardRepository {
const mergedProgress = sql<number>`
coalesce(
case
when ${readingProgress.updatedAt} is null then ${audiobookProgress.percentage}
when ${readingProgress.lastReadAt} is null then ${audiobookProgress.percentage}
when ${audiobookProgress.updatedAt} is null then ${readingProgress.percentage}
when ${readingProgress.updatedAt} >= ${audiobookProgress.updatedAt} then ${readingProgress.percentage}
when ${readingProgress.lastReadAt} >= ${audiobookProgress.updatedAt} then ${readingProgress.percentage}
else ${audiobookProgress.percentage}
end,
${readingProgress.percentage},
@@ -144,11 +144,11 @@ export class DashboardRepository {
0
)
`;
const mergedUpdatedAt = sql<Date | null>`
const mergedLastReadAt = sql<Date | null>`
case
when ${readingProgress.updatedAt} is null then ${audiobookProgress.updatedAt}
when ${audiobookProgress.updatedAt} is null then ${readingProgress.updatedAt}
when ${readingProgress.updatedAt} >= ${audiobookProgress.updatedAt} then ${readingProgress.updatedAt}
when ${readingProgress.lastReadAt} is null then ${audiobookProgress.updatedAt}
when ${audiobookProgress.updatedAt} is null then ${readingProgress.lastReadAt}
when ${readingProgress.lastReadAt} >= ${audiobookProgress.updatedAt} then ${readingProgress.lastReadAt}
else ${audiobookProgress.updatedAt}
end
`;
@@ -177,7 +177,7 @@ export class DashboardRepository {
when ${completionPredicate}
then greatest(
coalesce(${userBookStatus.updatedAt}, to_timestamp(0)),
coalesce(${mergedUpdatedAt}, to_timestamp(0))
coalesce(${mergedLastReadAt}, to_timestamp(0))
)
else null
end as completion_updated_at
@@ -1,6 +1,7 @@
import { Logger } from '@nestjs/common';
import { KoboReadingStateService } from './kobo-reading-state.service';
import { createCapturingDb } from '../../../common/test-utils/capture-sql-db';
import { ACHIEVEMENT_EVENT_BOOK_PROGRESS_CHANGED } from '../../achievement/achievement-events.service';
function makeInsertChain() {
@@ -531,4 +532,25 @@ describe('KoboReadingStateService', () => {
expect(bookAccessService.assertBookAccessible).not.toHaveBeenCalled();
expect(db.query.koboReadingStates.findFirst).not.toHaveBeenCalled();
});
// Recording which Location a bookmark maps to is bookkeeping, not reading, so it must not
// count as a read event. lastReadAt advances by default, so this path has to opt out.
it('leaves both timestamps untouched when stamping a bookmark location', async () => {
const { db, queries } = createCapturingDb();
const service = makeService(db as never);
await (
service as unknown as {
stampProgressLocation: (
userId: number,
fileId: number,
point: { source: string; value: string; contentSourceProgressPercent: number | null },
) => Promise<void>;
}
).stampProgressLocation(7, 44, { source: 'OEBPS/ch1.xhtml', value: 'kobo.3.1', contentSourceProgressPercent: 12 });
expect(queries).toHaveLength(1);
expect(queries[0]!.sql).toContain('"updated_at" = "reading_progress"."updated_at"');
expect(queries[0]!.sql).toContain('"last_read_at" = "reading_progress"."last_read_at"');
});
});
@@ -322,7 +322,7 @@ export class KoboReadingStateService {
return { bookmark: merged, lastModifiedKobo: nowIso };
}
/** Records which Location the bookmark reflects; deliberately keeps updatedAt untouched. */
/** Records which Location the bookmark reflects; deliberately keeps updatedAt and lastReadAt untouched. */
private async stampProgressLocation(
userId: number,
fileId: number,
@@ -336,6 +336,7 @@ export class KoboReadingStateService {
koboLocationValue: point.value,
koboContentSourceProgressPercent: point.contentSourceProgressPercent,
updatedAt: sql`"reading_progress"."updated_at"`,
lastReadAt: sql`"reading_progress"."last_read_at"`,
})
.where(and(eq(schema.readingProgress.userId, userId), eq(schema.readingProgress.bookFileId, fileId)));
}
@@ -455,6 +456,7 @@ export class KoboReadingStateService {
koboContentSourceProgressPercent,
koreaderProgress: nextXpointer,
updatedAt: sourceUpdatedAt,
lastReadAt: sourceUpdatedAt,
})
.onConflictDoUpdate({
target: [schema.readingProgress.bookFileId, schema.readingProgress.userId],
@@ -469,6 +471,7 @@ export class KoboReadingStateService {
koboContentSourceProgressPercent,
...(nextXpointer != null ? { koreaderProgress: nextXpointer } : {}),
updatedAt: sourceUpdatedAt,
lastReadAt: sourceUpdatedAt,
},
});
}
@@ -1,4 +1,5 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { createCapturingDb } from '../../common/test-utils/capture-sql-db';
import { sqlChunkText } from '../../common/test-utils/sql-chunk-text';
import { KoreaderRepository } from './koreader.repository';
@@ -889,6 +890,53 @@ describe('KoreaderRepository', () => {
expect(conflictSet(onConflictDoUpdate)).toEqual(expect.objectContaining({ pageNumber: null }));
});
// Regression: sorting by "Last Read" ordered on reading_progress.updated_at, which this path
// freezes on purpose so it stays a "last local write" marker. A KOReader-only reader therefore
// kept the timestamp of their first ever sync forever and the sort looked random.
describe('last-read timestamps in the compiled SQL', () => {
async function compileUpsert() {
const { db: capturingDb, queries } = createCapturingDb();
const capturingRepo = new KoreaderRepository(capturingDb as never);
await capturingRepo.upsertReadingProgress({ bookFileId: 44, userId: 12, percentage: 41.25 });
expect(queries).toHaveLength(1);
return queries[0]!;
}
function stampedLastReadAt({ sql: text, params }: { sql: string; params: unknown[] }) {
const match = /"last_read_at" = \$(\d+)/.exec(text);
expect(match).not.toBeNull();
return new Date(String(params[Number(match![1]) - 1])).getTime();
}
it('freezes updated_at but advances last_read_at on conflict', async () => {
const query = await compileUpsert();
expect(query.sql).toContain('"updated_at" = "reading_progress"."updated_at"');
expect(query.sql).toMatch(/"last_read_at" = \$\d+/);
expect(query.sql).not.toMatch(/"last_read_at" = "reading_progress"\."last_read_at"/);
expect(stampedLastReadAt(query)).not.toBeNaN();
});
it('stamps last_read_at close to now rather than reusing a stored value', async () => {
const before = Date.now();
const query = await compileUpsert();
const after = Date.now();
const stamped = stampedLastReadAt(query);
expect(stamped).toBeGreaterThanOrEqual(before);
expect(stamped).toBeLessThanOrEqual(after);
});
it('lets the insert branch default last_read_at instead of leaving it unset', async () => {
const { sql: text } = await compileUpsert();
expect(text).toContain('"last_read_at"');
expect(text.indexOf('"last_read_at"')).toBeLessThan(text.indexOf('on conflict'));
});
});
});
describe('file naming settings', () => {
@@ -550,6 +550,7 @@ export class MigrationImportRepository {
pageNumber: sql`excluded.page_number`,
positionSeconds: sql`excluded.position_seconds`,
updatedAt: sql`excluded.updated_at`,
lastReadAt: sql`excluded.last_read_at`,
},
});
}
@@ -157,6 +157,10 @@ describe('UserStateImporter', () => {
bookFileId: 500,
userId: 10,
percentage: 100,
// Imported history must keep its own last-read time; defaulting to now would make every
// migrated book look like it was just read.
updatedAt: new Date('2026-01-03T00:00:00.000Z'),
lastReadAt: new Date('2026-01-03T00:00:00.000Z'),
}),
]);
expect(importRepo.batchUpsertAudiobookProgress).toHaveBeenCalledWith([
@@ -75,6 +75,7 @@ type ReadingProgressUpsert = {
pageNumber: number | null;
positionSeconds: number | null;
updatedAt: Date;
lastReadAt: Date;
};
type AudiobookProgressUpsert = {
@@ -294,6 +295,7 @@ export class UserStateImporter {
typeof row.pageNumber === 'number' && Number.isFinite(row.pageNumber) && row.pageNumber >= 0 ? Math.trunc(row.pageNumber) : null;
const cfi = row.cfi?.trim() ? row.cfi : null;
const sourceUpdatedAt = toDate(row.updatedAt) ?? new Date();
batch.push({
bookFileId: targetFileId,
userId: targetUserId,
@@ -301,7 +303,8 @@ export class UserStateImporter {
cfi,
pageNumber,
positionSeconds,
updatedAt: toDate(row.updatedAt) ?? new Date(),
updatedAt: sourceUpdatedAt,
lastReadAt: sourceUpdatedAt,
});
counters.imported += 1;
}
@@ -319,7 +319,8 @@ describe('Jump buckets invariant (e2e)', { timeout: SCENARIO_TIMEOUT_MS }, () =>
const day = String(index + 1).padStart(2, '0');
const lastReadAt = `2025-05-${day}`;
seededById.get(file.bookId)!.lastReadAt = lastReadAt;
return { bookFileId: file.id, userId: admin.id, percentage: 50, updatedAt: new Date(`${lastReadAt}T12:00:00Z`) };
const readAt = new Date(`${lastReadAt}T12:00:00Z`);
return { bookFileId: file.id, userId: admin.id, percentage: 50, updatedAt: readAt, lastReadAt: readAt };
}),
);
});