fix(webapp): Fix backwards nextCursor for runs edge case#3073
fix(webapp): Fix backwards nextCursor for runs edge case#3073
Conversation
|
WalkthroughTwo pagination repository implementations (ClickHouse and Postgres) are being modified to improve boundary handling in backward pagination. When a result set contains fewer items than the requested page size, the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (1)
104-108: Simplify: use.at(-1)instead of.at(reversedRunIds.length - 1).The fix is correct — using the actual array length instead of a fixed
pageSizeindex avoids the out-of-bounds edge case. However,.at(-1)is the idiomatic way to access the last element and is equivalent here.✏️ Suggested simplification
- nextCursor = reversedRunIds.at(reversedRunIds.length - 1) ?? null; + nextCursor = reversedRunIds.at(-1) ?? null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts` around lines 104 - 108, Replace the verbose last-element access in the nextCursor assignment: instead of reversedRunIds.at(reversedRunIds.length - 1) use the idiomatic reversedRunIds.at(-1) and keep the null fallback, i.e. set nextCursor = reversedRunIds.at(-1) ?? null so the logic around reversedRunIds and nextCursor (in clickhouseRunsRepository.server.ts) is unchanged but simplified.apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts (1)
78-82: Same simplification: use.at(-1)for consistency.Fix is correct. Same as the ClickHouse file —
.at(-1)is cleaner.✏️ Suggested simplification
- nextCursor = reversedRuns.at(reversedRuns.length - 1)?.id ?? null; + nextCursor = reversedRuns.at(-1)?.id ?? null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts` around lines 78 - 82, The code determines the forward cursor using reversedRuns but uses reversedRuns.at(reversedRuns.length - 1) which is verbose; replace that expression with reversedRuns.at(-1) when assigning nextCursor (keep the nullish fallback: ?? null) to make it consistent and clearer alongside the ClickHouse change—update the assignment that references nextCursor and reversedRuns accordingly.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
Applied to files:
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.tsapps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Cloudflare Workers
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts`:
- Around line 104-108: Replace the verbose last-element access in the nextCursor
assignment: instead of reversedRunIds.at(reversedRunIds.length - 1) use the
idiomatic reversedRunIds.at(-1) and keep the null fallback, i.e. set nextCursor
= reversedRunIds.at(-1) ?? null so the logic around reversedRunIds and
nextCursor (in clickhouseRunsRepository.server.ts) is unchanged but simplified.
In `@apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts`:
- Around line 78-82: The code determines the forward cursor using reversedRuns
but uses reversedRuns.at(reversedRuns.length - 1) which is verbose; replace that
expression with reversedRuns.at(-1) when assigning nextCursor (keep the nullish
fallback: ?? null) to make it consistent and clearer alongside the ClickHouse
change—update the assignment that references nextCursor and reversedRuns
accordingly.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| previousCursor = reversedRunIds.at(1) ?? null; | ||
| nextCursor = reversedRunIds.at(options.page.size) ?? null; |
There was a problem hiding this comment.
🚩 Pre-existing: backward+hasMore branch has inconsistent overflow item handling
In the hasMore backward case (not changed by this PR), the overflow/sentinel item handling appears inconsistent with the forward case.
For forward+hasMore: the query returns [D, C, B, A] (DESC), the code keeps [D, C, B] via runs.slice(0, pageSize), and A is the overflow item (furthest from cursor, oldest). This is correct.
For backward+hasMore: the query returns [A, B, C, D] (ASC), but the code keeps [B, C, D] via runs.slice(1, pageSize+1) at clickhouseRunsRepository.server.ts:117, discarding A (closest to cursor). By analogy with forward, the overflow should be D (furthest from cursor, newest), and the page should display [A, B, C].
This also affects cursor values in the hasMore backward branch at clickhouseRunsRepository.server.ts:101-102:
previousCursor = reversedRunIds.at(1)— points to the second-newest item (C), but since "Previous" in the UI triggersdirection=backward(seeListPagination.tsx:35), using C as cursor would re-fetch D which is already on the displayed page.nextCursor = reversedRunIds.at(options.page.size)— points to the overflow item A rather than the oldest displayed item B.
This may not cause visible issues in practice if page boundaries overlap by one item (the UI would just show a duplicated boundary item), but it's worth investigating whether users see repeated runs when navigating backward through multiple pages.
(Refers to lines 100-102)
Was this helpful? React with 👍 or 👎 to provide feedback.
| previousCursor = reversedRuns.at(1)?.id ?? null; | ||
| nextCursor = reversedRuns.at(options.page.size)?.id ?? null; |
There was a problem hiding this comment.
🚩 Same pre-existing overflow issue exists in the Postgres repository
The Postgres backward+hasMore branch at postgresRunsRepository.server.ts:74-76 has the same inconsistency as the ClickHouse version — previousCursor and nextCursor are computed from reversedRuns using the same indices, and the runsToReturn slicing at line 89-91 discards the first ASC item rather than the last. This is expected since both repositories mirror each other's pagination logic.
(Refers to lines 74-76)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
✅ Checklist
Testing
Created a new run, moved forwards and backwards between run list pages. The issue did not reporduce.
Changelog
Changed runs list backwards cursor to take into account runs length not page size.
Screenshots
Screen.Recording.2026-02-17.at.02.10.57.mov