-
Notifications
You must be signed in to change notification settings - Fork 17
feat: add list chats endpoint #1425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 86a54e3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Preview Environment (PR #1425)Preview environment scaled down after 12h of inactivity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| HAVING if( | ||
| ? = '', | ||
| true, | ||
| if( | ||
| ? = 'asc', | ||
| min(time_unix_nano) > (SELECT min(time_unix_nano) FROM telemetry_logs WHERE gram_chat_id = ? GROUP BY gram_chat_id LIMIT 1), | ||
| min(time_unix_nano) < (SELECT min(time_unix_nano) FROM telemetry_logs WHERE gram_chat_id = ? GROUP BY gram_chat_id LIMIT 1) | ||
| ) | ||
| ) | ||
| ORDER BY | ||
| IF(? = 'asc', start_time_unix_nano, 0) ASC, | ||
| IF(? = 'desc', start_time_unix_nano, 0) DESC | ||
| LIMIT ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 ListChats pagination is unstable and can skip/duplicate results because ORDER BY lacks a tie-breaker
ListChats paginates by comparing min(time_unix_nano) against the cursor chat's min(time_unix_nano) and orders only by start_time_unix_nano. If multiple chats share the same min(time_unix_nano) (likely with batched ingestion or coarse timestamps), pagination becomes non-deterministic:
- Page 1 might end with a chat whose
start_time_unix_nanoties with other chats. - Page 2 uses
>/<on only the timestamp, so tied chats can be skipped (never appear) or duplicated depending on ordering.
Actual: pagination may miss chats or return duplicates across pages.
Expected: deterministic pagination with a stable sort key and cursor that matches the sort.
Evidence
- Cursor predicate compares only
min(time_unix_nano):
server/internal/telemetry/repo/queries.sql.go:423-431 ORDER BYonly usesstart_time_unix_nanoand no secondary key:
server/internal/telemetry/repo/queries.sql.go:432-435
Recommendation: Add a deterministic tie-breaker to the sort and cursor comparison (e.g., (start_time_unix_nano, gram_chat_id)), and compute next_cursor based on the same composite key (or make cursor a composite token).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| FROM telemetry_logs | ||
| WHERE gram_project_id = ? | ||
| AND time_unix_nano >= ? | ||
| AND time_unix_nano <= ? | ||
| AND gram_chat_id IS NOT NULL | ||
| AND gram_chat_id != '' | ||
| AND (? = '' OR gram_deployment_id = toUUIDOrNull(?)) | ||
| AND if(? = '', true, position(telemetry_logs.gram_urn, ?) > 0) | ||
| GROUP BY gram_chat_id | ||
| HAVING if( | ||
| ? = '', | ||
| true, | ||
| if( | ||
| ? = 'asc', | ||
| min(time_unix_nano) > (SELECT min(time_unix_nano) FROM telemetry_logs WHERE gram_chat_id = ? GROUP BY gram_chat_id LIMIT 1), | ||
| min(time_unix_nano) < (SELECT min(time_unix_nano) FROM telemetry_logs WHERE gram_chat_id = ? GROUP BY gram_chat_id LIMIT 1) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Chat cursor subquery is not scoped by project/time filters, making pagination unstable if gram_chat_id is not globally unique
The cursor boundary subquery used in ListChats looks up the cursor chat’s min(time_unix_nano) by gram_chat_id only:
SELECT min(time_unix_nano) FROM telemetry_logs WHERE gram_chat_id = ? GROUP BY gram_chat_idActual behavior: if gram_chat_id can collide across projects (or if there are logs for the same gram_chat_id outside the requested time window), the cursor’s start time may be computed from unrelated rows. This makes pagination jump backwards/forwards unexpectedly.
Expected behavior: the cursor lookup should be scoped consistently with the main query (at minimum by gram_project_id, and ideally also by the same time range / filters).
Click to expand
The main query scopes to a project/time window:
WHERE gram_project_id = ? AND time_unix_nano >= ? AND time_unix_nano <= ? ...
But the cursor subquery does not.
Recommendation: Add gram_project_id = ? (and optionally time_unix_nano bounds / same filters) to the cursor subquery so it computes the cursor boundary in the same domain as the main result set.
Was this helpful? React with 👍 or 👎 to provide feedback.
This adds an endpoint to list chats (by grouping them by chat ID).
First step into having the full view of chat traces.