Skip to content

fix(firestore): convert app-shaped timestamps and geopoints on admin writes - #19

Closed
EMTumariscal wants to merge 1 commit into
Flowdesktech:masterfrom
EMTumariscal:fix/timestamps-update
Closed

EMTumariscal wants to merge 1 commit into
Flowdesktech:masterfrom
EMTumariscal:fix/timestamps-update

Conversation

@EMTumariscal

@EMTumariscal EMTumariscal commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes a data-corruption bug on the firebase-admin (service-account) write path: updating any document that contains Firestore Timestamp (or GeoPoint) fields rewrote those fields as plain map objects instead of Firestore Timestamp / GeoPoint values.

The renderer represents a Firestore Timestamp as a plain { _seconds, _nanoseconds } object and a GeoPoint as { _latitude, _longitude } (decoded in electron/utils/firestoreHelpers.js). The Google/OAuth REST write path converts those shapes back through convertToFirestoreValue, but the Admin SDK path passed the raw objects straight to set() / update() / batch.set(). The Admin SDK does not recognize them, so it persisted a mapValue. Because the UI sends the whole document on save ({ ...docData, [field]: newValue }), every update degraded all existing timestamp/geopoint fields of the document.

Reproduction

  1. Connect with a service-account (Admin SDK) project.
  2. Open a collection and a document that has a Timestamp field (created elsewhere, e.g. the Firebase console).
  3. Edit any unrelated field and save.
  4. Re-read the document: the timestamp is now an object/map instead of a timestamp.

Fix

  • Add toFirestoreAdminValue(value, { Timestamp, GeoPoint, FieldValue }) to electron/utils/firestoreHelpers.js: a recursive, non-mutating converter that maps Date → Timestamp, { _seconds, _nanoseconds } / { seconds, nanoseconds | nanos } → Timestamp, { _latitude, _longitude } / { latitude, longitude } → GeoPoint, recurses through arrays and plain maps, passes primitives through, and preserves FieldValue sentinels. Constructors are injected so the module stays free of a firebase-admin import.
  • Apply it in the four Admin SDK write handlers in electron/controllers/firestoreController.js: firestore:createDocument, firestore:setDocument, firestore:updateDocument, firestore:importDocuments.
  • The REST/Google path (convertToFirestoreValue) and all read paths are unchanged.

Type of Change

  • Bug fix
  • Feature
  • Docs
  • Refactor

AI Code Generation

This change was implemented by an AI coding agent (OpenCode) under human direction. A human defined the task, reviewed the diff, and authorized the commit and this PR. AI was used for the root-cause investigation, the code changes, and the tests. The AI disclosure is provided here in full.

Related Issue

No existing upstream issue was found for this defect (searched open and closed issues for timestamp, Timestamp object, seconds nanos, update document, firestore timestamp). The reproduction steps are included in the Description above.

Technical Context

  • App-internal representation of a Firestore timestamp is { _seconds, _nanoseconds }, produced by firestoreDocumentToData / parseFirestoreValue in electron/utils/firestoreHelpers.js.
  • The Admin SDK (firebase-admin v14, @google-cloud/firestore v8) serializes an unrecognized plain object as a mapValue; only a real Timestamp / GeoPoint instance is written as a Firestore timestamp / geoPoint.
  • Firebase data types reference: https://firebase.google.com/docs/firestore/manage-data/data-types
  • The REST write path already handled this correctly; only the Admin SDK path was missing the conversion.

Test Environment

  • OS / hardware: Linux (WSL2, kernel 6.6.114.1-microsoft-standard-WSL2), AMD Ryzen 7 8840HS, x86_64.
  • Toolchain: Node.js v24.21.0, pnpm 10.15.1.
  • AI model / provider: OpenCode (opencode-go / deepseek-v4.1-flash).
  • Channels used for testing: unit tests only (no end-to-end run against a live Firestore project was performed).

Evidence

pnpm run test (vitest):

Test Files  13 passed (13)
     Tests  104 passed (104)

New/updated coverage:

  • electron/utils/firestoreHelpers.test.js (new): Date, both timestamp shapes, both geopoint shapes, nested array/map, primitive passthrough, FieldValue sentinel preservation, non-mutation.
  • electron/controllers/firestoreController.test.js: createDocument, setDocument, updateDocument, and importDocuments convert app-shaped values before writing, and the incoming payload is not mutated.

Checklist

  • I ran the project's checks locally. This repo has no Makefile/make check; the equivalent is the .github/workflows/ci.yml steps, all of which pass: pnpm run lint, pnpm run format:check, pnpm run typecheck, pnpm run test.
  • I self-reviewed the diff.
  • I added/updated tests for the change.
  • The PR is focused: 1 commit, 4 files, +269 / -7.
  • I verified the change end-to-end against a real Firestore project. Not done — only unit tests were run; see Test Environment.

@EMTumariscal

Copy link
Copy Markdown
Contributor Author

Superseded by #22 — consolidated PR from EMTumariscal:develop to master bundling this change with its original description preserved. Closing per maintainer request.

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