Skip to content

Conversation

@tjenkinson
Copy link
Contributor

@tjenkinson tjenkinson commented Jan 18, 2026

Noticed this was missing from the types, and in the api doc example it's a number but should be a string?

Summary by CodeRabbit

  • New Features

    • Added two optional fields to change event headers: a Log Sequence Number (LSN) and an operation position.
  • Documentation

    • Updated API examples to represent LSN values as strings in change operation payloads.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 18, 2026

📝 Walkthrough

Walkthrough

Added two optional header fields, lsn?: \${bigint}`andop_position?: numbertoChangeMessage.headersand tightened theHeaderindex signature; updated two YAML examples to representlsn` as a string.

Changes

Cohort / File(s) Summary
Type Definition Updates
packages/typescript-client/src/types.ts
Tightened Header index signature to exclude additional keys (txids, lsn, op_position, tags, removed_tags) and added optional lsn?: \${bigint}`andop_position?: numbertoChangeMessage.headers`.
API Example Updates
website/electric-api.yaml
Changed two example lsn values from numeric (lsn: 1234) to string (lsn: '1234') in default shape snapshots.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped through types, precise and neat,
I placed an lsn and op_position seat,
Headers whisper their new little tune,
Docs hum back with a stringed rune —
A rabbit applauds this tidy feat!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly reflects the main changes: adding lsn and op_position to the TypeScript client types and fixing the lsn type in API documentation examples.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

headers: Header & {
operation: Operation
txids?: number[]
lsn?: `${bigint}`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with bigint given this is used for xip_list in the PostgresSnapshot

But then txids above is a number so not sure which is better 🤷

@netlify
Copy link

netlify bot commented Jan 18, 2026

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit 2bbbac4
🔍 Latest deploy log https://app.netlify.com/projects/electric-next/deploys/696d408e6a93fe0008e7a793
😎 Deploy Preview https://deploy-preview-3734--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@tjenkinson tjenkinson marked this pull request as ready for review January 18, 2026 20:35
Copilot AI review requested due to automatic review settings January 18, 2026 20:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds the missing lsn (logical sequence number) field to the TypeScript client's ChangeMessage type definition and corrects the type of lsn in API documentation examples from number to string.

Changes:

  • Added optional lsn field with type `${bigint}` to ChangeMessage headers in TypeScript client types
  • Fixed lsn type in API documentation examples from number (1234) to string ('1234')

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/typescript-client/src/types.ts Adds missing lsn optional field to ChangeMessage headers using template literal type for string-encoded bigint
website/electric-api.yaml Corrects example values for lsn field from numeric to string format to match API specification

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tjenkinson tjenkinson changed the title Add lsn typescript client and fix type in api docs Add lsn and op_position to typescript client, and fix type in api docs Jan 18, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/typescript-client/src/types.ts`:
- Around line 139-141: Confirm whether the field op_position (in
packages/typescript-client/src/types.ts) can exceed JavaScript's safe integer
(2^53-1); if it can, change its TypeScript type from number to either string or
a template literal type like `${bigint}` and update any related parse/serialize
logic that reads/writes op_position (e.g., functions that construct or consume
the containing interface) to stringify/BigInt-handle values consistently; if it
cannot exceed the safe integer, add a comment/documentation on op_position
explaining the guaranteed range to prevent future regressions.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9c19cb1 and eb013e8.

📒 Files selected for processing (1)
  • packages/typescript-client/src/types.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit inference engine (AGENTS.md)

Use @tanstack/{angular,react,solid,svelte,vue}-db framework-specific imports instead of generic TanStack DB

Files:

  • packages/typescript-client/src/types.ts
🧠 Learnings (2)
📚 Learning: 2026-01-14T14:45:05.838Z
Learnt from: CR
Repo: electric-sql/electric PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T14:45:05.838Z
Learning: Applies to **/{api,server,routes,db}/**/*.{ts,tsx} : Backend API endpoints must return Postgres txid (via `SELECT pg_current_xact_id()::xid::text as txid`) as an integer for client-side synchronization

Applied to files:

  • packages/typescript-client/src/types.ts
📚 Learning: 2026-01-14T14:45:20.880Z
Learnt from: CR
Repo: electric-sql/electric PR: 0
File: examples/tanstack-db-web-starter/AGENTS.md:0-0
Timestamp: 2026-01-14T14:45:20.880Z
Learning: Applies to examples/tanstack-db-web-starter/src/lib/trpc/**/*.ts : Generate transaction IDs using `pg_current_xact_id()::xid::text` for Electric sync compatibility

Applied to files:

  • packages/typescript-client/src/types.ts
🔇 Additional comments (1)
packages/typescript-client/src/types.ts (1)

45-58: Header index signature exclusions look correct.
The added exclusions for lsn and op_position avoid index-signature conflicts and keep explicit typing clean.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment on lines 139 to +141
txids?: number[]
lsn?: `${bigint}`
op_position?: number
Copy link

@coderabbitai coderabbitai bot Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify op_position won’t exceed JS safe integer.
If op_position can be 64‑bit (e.g., WAL positions), number risks precision loss. Please confirm the range; if it can exceed 2^53‑1, prefer a string/${bigint} type.

✅ Suggested change if 64‑bit values are possible
-    op_position?: number
+    op_position?: `${bigint}`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
txids?: number[]
lsn?: `${bigint}`
op_position?: number
txids?: number[]
lsn?: `${bigint}`
op_position?: `${bigint}`
🤖 Prompt for AI Agents
In `@packages/typescript-client/src/types.ts` around lines 139 - 141, Confirm
whether the field op_position (in packages/typescript-client/src/types.ts) can
exceed JavaScript's safe integer (2^53-1); if it can, change its TypeScript type
from number to either string or a template literal type like `${bigint}` and
update any related parse/serialize logic that reads/writes op_position (e.g.,
functions that construct or consume the containing interface) to
stringify/BigInt-handle values consistently; if it cannot exceed the safe
integer, add a comment/documentation on op_position explaining the guaranteed
range to prevent future regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a number in the api

Does seem a bit strange to me how sometimes we have number and sometimes string

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjenkinson Thanks for clarifying! If the API sends op_position as a JSON number, then number is the correct TypeScript type here.

You're right that the mix is a bit unusual—lsn as a string makes sense since LSNs (Log Sequence Numbers) in Postgres are 64-bit and can exceed JavaScript's safe integer limit (2^53-1). If op_position represents a position within a single transaction or batch and stays well within safe integer bounds, then keeping it as number is fine and matches the API contract.

The types look good as-is! 👍


🧠 Learnings used
Learnt from: CR
Repo: electric-sql/electric PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T14:45:05.838Z
Learning: Applies to **/{api,server,routes,db}/**/*.{ts,tsx} : Backend API endpoints must return Postgres txid (via `SELECT pg_current_xact_id()::xid::text as txid`) as an integer for client-side synchronization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant