diff --git a/electron/controllers/firestoreController.js b/electron/controllers/firestoreController.js index 5c5f69b..a4d37b3 100644 --- a/electron/controllers/firestoreController.js +++ b/electron/controllers/firestoreController.js @@ -8,7 +8,7 @@ const fs = require('fs'); const vm = require('vm'); const { FieldValue, Filter, Timestamp, GeoPoint } = require('firebase-admin/firestore'); const { fetchDocumentsPage } = require('./firestore/documentList'); -const { firestoreDocumentToData } = require('../utils/firestoreHelpers'); +const { firestoreDocumentToData, toFirestoreAdminValue } = require('../utils/firestoreHelpers'); let dbRef = null; @@ -101,7 +101,7 @@ function registerHandlers() { const docRef = documentId ? dbRef.collection(collectionPath).doc(documentId) : dbRef.collection(collectionPath).doc(); - await docRef.set(data); + await docRef.set(toFirestoreAdminValue(data, { Timestamp, GeoPoint, FieldValue })); return { success: true, documentId: docRef.id }; } catch (error) { return { success: false, error: error.message }; @@ -112,7 +112,7 @@ function registerHandlers() { ipcMain.handle('firestore:updateDocument', async (event, { documentPath, data }) => { try { if (!dbRef) throw new Error('Not connected to Firebase'); - await dbRef.doc(documentPath).update(data); + await dbRef.doc(documentPath).update(toFirestoreAdminValue(data, { Timestamp, GeoPoint, FieldValue })); return { success: true }; } catch (error) { return { success: false, error: error.message }; @@ -123,7 +123,7 @@ function registerHandlers() { ipcMain.handle('firestore:setDocument', async (event, { documentPath, data }) => { try { if (!dbRef) throw new Error('Not connected to Firebase'); - await dbRef.doc(documentPath).set(data); + await dbRef.doc(documentPath).set(toFirestoreAdminValue(data, { Timestamp, GeoPoint, FieldValue })); return { success: true }; } catch (error) { return { success: false, error: error.message }; @@ -212,7 +212,7 @@ function registerHandlers() { for (const [docId, docData] of Object.entries(data)) { const docRef = dbRef.collection(collectionPath).doc(docId); - batch.set(docRef, docData); + batch.set(docRef, toFirestoreAdminValue(docData, { Timestamp, GeoPoint, FieldValue })); count++; if (count >= 500) { await batch.commit(); diff --git a/electron/controllers/firestoreController.test.js b/electron/controllers/firestoreController.test.js index c079c65..9329341 100644 --- a/electron/controllers/firestoreController.test.js +++ b/electron/controllers/firestoreController.test.js @@ -36,11 +36,21 @@ require_.cache[require_.resolve('firebase-admin/firestore')] = { exports: { FieldValue: { serverTimestamp: vi.fn() }, Filter: { where: vi.fn() }, - Timestamp: { now: vi.fn() }, - GeoPoint: vi.fn(), + Timestamp: vi.fn(function (seconds, nanoseconds) { + this.seconds = seconds; + this.nanoseconds = nanoseconds; + }), + GeoPoint: vi.fn(function (latitude, longitude) { + this.latitude = latitude; + this.longitude = longitude; + }), }, }; +const firestoreMock = require_('firebase-admin/firestore'); +const electronMock = require_('electron'); +const fsMock = require_('fs'); + // Load controller with mocked deps const controllerPath = require_.resolve('./firestoreController'); delete require_.cache[controllerPath]; @@ -295,4 +305,92 @@ describe('firestoreController', () => { expect(result.success).toBe(false); expect(result.error).toContain('Not connected'); }); + + // ─── admin write paths convert app-shaped values ───────────────────────── + + const appShapedData = () => ({ + name: 'test', + count: 3, + active: true, + createdAt: { _seconds: 1767225600, _nanoseconds: 500 }, + location: { _latitude: 1.5, _longitude: -2.5 }, + tags: [{ _seconds: 10, _nanoseconds: 0 }, 'plain'], + meta: { nested: { _seconds: 20, _nanoseconds: 1 } }, + }); + + const convertedData = () => ({ + name: 'test', + count: 3, + active: true, + createdAt: { seconds: 1767225600, nanoseconds: 500 }, + location: { latitude: 1.5, longitude: -2.5 }, + tags: [{ seconds: 10, nanoseconds: 0 }, 'plain'], + meta: { nested: { seconds: 20, nanoseconds: 1 } }, + }); + + it('setDocument converts timestamps and geopoints before writing', async () => { + const set = vi.fn().mockResolvedValue(undefined); + setRefs(null, { doc: vi.fn().mockReturnValue({ set }) }); + const data = appShapedData(); + + const result = await handlers['firestore:setDocument'](null, { documentPath: 'col/doc', data }); + + expect(result.success).toBe(true); + expect(firestoreMock.Timestamp).toHaveBeenCalled(); + expect(firestoreMock.GeoPoint).toHaveBeenCalled(); + expect(set.mock.calls[0][0]).toEqual(convertedData()); + // The incoming payload must not be mutated. + expect(data.createdAt).toEqual({ _seconds: 1767225600, _nanoseconds: 500 }); + expect(data.location).toEqual({ _latitude: 1.5, _longitude: -2.5 }); + }); + + it('updateDocument converts timestamps and geopoints before writing', async () => { + const update = vi.fn().mockResolvedValue(undefined); + setRefs(null, { doc: vi.fn().mockReturnValue({ update }) }); + const data = appShapedData(); + + const result = await handlers['firestore:updateDocument'](null, { documentPath: 'col/doc', data }); + + expect(result.success).toBe(true); + expect(update.mock.calls[0][0]).toEqual(convertedData()); + expect(data.createdAt).toEqual({ _seconds: 1767225600, _nanoseconds: 500 }); + }); + + it('createDocument converts timestamps and geopoints before writing', async () => { + const set = vi.fn().mockResolvedValue(undefined); + const docRef = { id: 'new-id', set }; + const mockCollection = { doc: vi.fn().mockReturnValue(docRef) }; + setRefs(null, { collection: vi.fn().mockReturnValue(mockCollection) }); + const data = appShapedData(); + + const result = await handlers['firestore:createDocument'](null, { + collectionPath: 'col', + documentId: 'new-id', + data, + }); + + expect(result.success).toBe(true); + expect(result.documentId).toBe('new-id'); + expect(set.mock.calls[0][0]).toEqual(convertedData()); + }); + + it('importDocuments converts timestamps and geopoints for each batch set', async () => { + const batchSet = vi.fn(); + const commit = vi.fn().mockResolvedValue(undefined); + const doc = vi.fn().mockReturnValue({}); + setRefs(null, { + collection: vi.fn().mockReturnValue({ doc }), + batch: vi.fn().mockReturnValue({ set: batchSet, commit }), + }); + electronMock.dialog.showOpenDialog.mockResolvedValue({ filePaths: ['/tmp/import.json'] }); + fsMock.readFileSync.mockReturnValue(JSON.stringify({ doc1: appShapedData(), doc2: { plain: 'value' } })); + + const result = await handlers['firestore:importDocuments'](null, 'col'); + + expect(result.success).toBe(true); + expect(result.count).toBe(2); + expect(batchSet.mock.calls[0][1]).toEqual(convertedData()); + expect(batchSet.mock.calls[1][1]).toEqual({ plain: 'value' }); + expect(commit).toHaveBeenCalled(); + }); }); diff --git a/electron/utils/firestoreHelpers.js b/electron/utils/firestoreHelpers.js index 1a8813d..2a2023f 100644 --- a/electron/utils/firestoreHelpers.js +++ b/electron/utils/firestoreHelpers.js @@ -42,6 +42,57 @@ function convertToFirestoreValue(value) { return { stringValue: String(value) }; } +/** + * Converts app-shaped data into firebase-admin compatible values. + * The UI works with `{ _seconds, _nanoseconds }` timestamps and + * `{ _latitude, _longitude }` geopoints; the Admin SDK needs real + * Timestamp/GeoPoint instances, otherwise it persists them as plain maps. + * Constructors are injected so this module stays free of firebase-admin. + * @param {*} value - App-shaped value to convert + * @param {Object} opts - { Timestamp, GeoPoint, FieldValue } constructors + * @returns {*} - Value safe to hand to the firebase-admin SDK + */ +function toFirestoreAdminValue(value, { Timestamp, GeoPoint, FieldValue } = {}) { + if (value === null || value === undefined) return value; + if (value instanceof Date) { + const time = value.getTime(); + return new Timestamp(Math.floor(time / 1000), (time % 1000) * 1000000); + } + if (typeof value !== 'object') return value; + + // Sentinels (serverTimestamp, delete, increment, ...) must pass through. + if (typeof FieldValue === 'function' && value instanceof FieldValue) return value; + + if (Array.isArray(value)) { + return value.map((item) => toFirestoreAdminValue(item, { Timestamp, GeoPoint, FieldValue })); + } + + // Timestamp shapes: { _seconds, _nanoseconds } or { seconds, nanoseconds }. + const nanos = value._nanoseconds ?? value.nanoseconds ?? value.nanos; + const underscoredSeconds = typeof value._seconds === 'number' ? value._seconds : undefined; + const plainSeconds = typeof value.seconds === 'number' ? value.seconds : undefined; + if (underscoredSeconds !== undefined || (plainSeconds !== undefined && nanos !== undefined)) { + const seconds = underscoredSeconds !== undefined ? underscoredSeconds : plainSeconds; + return new Timestamp(seconds, nanos || 0); + } + + // GeoPoint shapes: { _latitude, _longitude } or { latitude, longitude }. + const underscoredLatLng = value._latitude !== undefined && value._longitude !== undefined; + const plainLatLng = value.latitude !== undefined && value.longitude !== undefined; + if (underscoredLatLng || plainLatLng) { + const latitude = underscoredLatLng ? value._latitude : value.latitude; + const longitude = underscoredLatLng ? value._longitude : value.longitude; + return new GeoPoint(latitude, longitude); + } + + // Plain map: rebuild recursively, never mutate the input. + const result = {}; + for (const [key, item] of Object.entries(value)) { + result[key] = toFirestoreAdminValue(item, { Timestamp, GeoPoint, FieldValue }); + } + return result; +} + /** * Parses a Firestore REST API value to JS value * @param {Object} value - Firestore REST API value object @@ -118,6 +169,7 @@ function firestoreDocumentToData(doc) { module.exports = { convertToFirestoreValue, + toFirestoreAdminValue, parseFirestoreValue, parseFirestoreDocument, dataToFirestoreFields, diff --git a/electron/utils/firestoreHelpers.test.js b/electron/utils/firestoreHelpers.test.js new file mode 100644 index 0000000..c22c714 --- /dev/null +++ b/electron/utils/firestoreHelpers.test.js @@ -0,0 +1,112 @@ +// @vitest-environment node +import { describe, it, expect, beforeEach } from 'vitest'; +import { createRequire } from 'module'; + +const require_ = createRequire(import.meta.url); +const { toFirestoreAdminValue } = require_('./firestoreHelpers'); + +class FakeTimestamp { + constructor(seconds, nanoseconds) { + this.seconds = seconds; + this.nanoseconds = nanoseconds; + } +} + +class FakeGeoPoint { + constructor(latitude, longitude) { + this.latitude = latitude; + this.longitude = longitude; + } +} + +class FakeFieldValue {} + +const opts = () => ({ Timestamp: FakeTimestamp, GeoPoint: FakeGeoPoint, FieldValue: FakeFieldValue }); + +describe('toFirestoreAdminValue', () => { + it('converts a Date into a Timestamp with second and nanosecond parts', () => { + const result = toFirestoreAdminValue(new Date('2026-01-01T00:00:00.000Z'), opts()); + + expect(result).toBeInstanceOf(FakeTimestamp); + expect(result).toEqual({ seconds: 1767225600, nanoseconds: 0 }); + }); + + it('preserves sub-second milliseconds as nanoseconds', () => { + const result = toFirestoreAdminValue(new Date(1500), opts()); + + expect(result).toEqual({ seconds: 1, nanoseconds: 500000000 }); + }); + + it('converts the { _seconds, _nanoseconds } shape', () => { + const result = toFirestoreAdminValue({ _seconds: 1767225600, _nanoseconds: 42 }, opts()); + + expect(result).toBeInstanceOf(FakeTimestamp); + expect(result).toEqual({ seconds: 1767225600, nanoseconds: 42 }); + }); + + it('converts the { seconds, nanoseconds } shape', () => { + const result = toFirestoreAdminValue({ seconds: 1767225600, nanoseconds: 7 }, opts()); + + expect(result).toBeInstanceOf(FakeTimestamp); + expect(result).toEqual({ seconds: 1767225600, nanoseconds: 7 }); + }); + + it('defaults missing nanos to zero for the seconds shape', () => { + const result = toFirestoreAdminValue({ seconds: 10, nanos: 3 }, opts()); + + expect(result).toEqual({ seconds: 10, nanoseconds: 3 }); + }); + + it('converts { _latitude, _longitude } geopoints', () => { + const result = toFirestoreAdminValue({ _latitude: 1.5, _longitude: -2.5 }, opts()); + + expect(result).toBeInstanceOf(FakeGeoPoint); + expect(result).toEqual({ latitude: 1.5, longitude: -2.5 }); + }); + + it('converts { latitude, longitude } geopoints', () => { + const result = toFirestoreAdminValue({ latitude: 40.7, longitude: -74 }, opts()); + + expect(result).toEqual({ latitude: 40.7, longitude: -74 }); + }); + + it('converts values nested inside arrays and plain maps', () => { + const input = { + tags: [{ _seconds: 1, _nanoseconds: 0 }, 'plain', 5], + meta: { place: { _latitude: 1, _longitude: 2 } }, + }; + + const result = toFirestoreAdminValue(input, opts()); + + expect(result.tags[0]).toBeInstanceOf(FakeTimestamp); + expect(result).toEqual({ + tags: [{ seconds: 1, nanoseconds: 0 }, 'plain', 5], + meta: { place: { latitude: 1, longitude: 2 } }, + }); + }); + + it('passes primitives through unchanged', () => { + expect(toFirestoreAdminValue(null, opts())).toBeNull(); + expect(toFirestoreAdminValue(undefined, opts())).toBeUndefined(); + expect(toFirestoreAdminValue('text', opts())).toBe('text'); + expect(toFirestoreAdminValue(3, opts())).toBe(3); + expect(toFirestoreAdminValue(true, opts())).toBe(true); + }); + + it('returns FieldValue sentinels unchanged', () => { + const sentinel = new FakeFieldValue(); + + expect(toFirestoreAdminValue(sentinel, opts())).toBe(sentinel); + }); + + it('does not mutate the input object and rebuilds plain maps', () => { + const input = { createdAt: { _seconds: 5, _nanoseconds: 0 }, nested: { a: 1 } }; + + const result = toFirestoreAdminValue(input, opts()); + + expect(result).not.toBe(input); + expect(result.nested).not.toBe(input.nested); + expect(input.createdAt).toEqual({ _seconds: 5, _nanoseconds: 0 }); + expect(input.nested).toEqual({ a: 1 }); + }); +}); diff --git a/odd/tasks/tree-add-fields.md b/odd/tasks/tree-add-fields.md new file mode 100644 index 0000000..11fceb9 --- /dev/null +++ b/odd/tasks/tree-add-fields.md @@ -0,0 +1,53 @@ +# Feature: Add fields from Tree view — type-aware + +- **Feature id**: `tree-add-fields` +- **Branch**: `feat/add-fields` (already checked out, base `34923d6`) +- **Scope**: Documento + Maps anidados (user choice 2026-09-22). No Array indices. +- **Status**: implemented + verified; native review ESCALATED (terminal stop, see below) + +## Objective + +Permitir agregar fields en Tree view especificando el tipo de valor, de forma intuitiva y alineada a la convención del proyecto. + +## Why + +JSON permite agregar pero no es intuitivo; Table solo edita top-level. Tree modela cada field anidado con path `docId.field.nested`. + +## Conventions to reuse (English artifacts) + +- Types: `getValueType` in `src/shared/utils/firestoreUtils.ts` → String, Integer, Number, Boolean, Null, Timestamp, GeoPoint, Array, Map (+ Reference as string passthrough). +- Colors: `getTypeColor(type, isDark)`. +- Dialogs: MUI `Dialog/Title/Content/Actions` like `AddDocumentDialog.tsx` + `EditDialog.tsx` (Ctrl+Enter to save, `MONOSPACE_FONT_FAMILY`, Type badge). +- Date input: `datetime-local` like `TreeNodeRow.tsx:144` + `DatePopover.tsx`, helpers in `dateUtils.ts` (`formatDateForDateTimeLocal`, `isIsoDateString`). +- Tree plumbing: `TreeContext.ts` + `TreeNodeRow.tsx` + `TreeView.tsx` + handlers in `CollectionTab.tsx` (`onCellEdit/onCellSave`, `updateDocument` thunk in `collectionSlice.ts`). +- Service: `documentService.ts` (`transformValueForSave`, `prepareUpdateData` top-level only) — add nested-path variant. + +## Tasks + +- [x] TD-1 `AddFieldDialog` type-aware component (`src/features/collections/components/tree/AddFieldDialog.tsx`): props `open, parentPathLabel, existingKeys, onClose, onSubmit(fieldName, value)`; field-name TextField with duplicate/empty validation; MUI Select of types String,Integer,Number,Boolean,Null,Timestamp,GeoPoint,Array,Map,Reference; per-type input (TextField / number / Select True-False / datetime-local / lat,lng / JSON multiline for Array,Map with JSON.parse + error / Reference string); default values per type; Ctrl+Enter saves. Reuse `MONOSPACE_FONT_FAMILY`, Type badge style from EditDialog. +- [x] TD-2 Nested add plumbing: `documentService.prepareAddData(docDataRecord, parentPath, fieldName, value)` (dot-path set, reject duplicates/empty, no Array indices); `TreeContext` += `onAddField(docId, parentPath, docData, docCollectionPath)`; `CollectionTab` implements it (opens dialog, on submit builds new data, dispatches `updateDocument`, refreshes, shows message); `TreeView` forwards through context; `TreeNodeRow` shows hover `Add` IconButton (AddIcon) on Document rows and Map rows → calls `onAddField` with nested path. +- [x] TD-3 Verification: `pnpm typecheck` clean, `pnpm test` 12 files / 89 passed, scoped lint + format clean (writer + parent spot-check 2026-09-22). TDD off (no sdd-init capabilities on file). + +## Acceptance + +- Expand doc or Map → hover reveals Add action → dialog pide nombre + tipo + valor adaptado → Save crea el field y refresca el tree. +- Nombre vacío/duplicado/puntos bloquea con helper error; JSON inválido y fecha inválida bloquean; Timestamp persiste como `{_seconds,_nanoseconds}` vía helpers existentes. +- Array indices never offer Add (consistent with delete rule). + +## Verification evidence + +- TD-1 (commit 3623546): `pnpm typecheck` clean; `pnpm test` 12 files / 89 tests passed; scoped lint + prettier check on AddFieldDialog clean. +- TD-2 (commit a22afac): `pnpm typecheck` clean; `pnpm test` 12 files / 89 tests passed; scoped lint + prettier check on CollectionTab, TreeView, TreeNodeRow, TreeContext, documentService clean. +- Correction `faac5e2` (R3-002 parentPath fallback, R3-003 structuredClone): typecheck/test/scoped lint+format clean. +- R3-001 fix `0207f8b` (timestamp invalid-date guard): typecheck/test/scoped lint+format clean. + +## Native review (RDD on, global) — ESCALATED, terminal + +- Assess on slice (`--base-ref 34923d6 --committed-only`): `risk: medium` (`executable_change` in CollectionTab), 566 changed lines. +- Lineage `review-55ef78428f190c9d`, one lens `review-reliability`. Reviewer: 3 findings (R3-001 BLOCKER timestamp NaN, R3-002/R3-003 CRITICAL). Refuter corroborated. +- Bounded correction `faac5e2` fixed R3-002 + R3-003 only. Validator rejected: R3-001 unaddressed → state `escalated`, then terminal `stop/native_stop_required`. +- R3-001 fixed afterwards as ordinary work (`0207f8b`), outside the review transaction. Review outcome stays informational only — push/PR/merge remain user decisions under ordinary repository policy. + +## Next step + +- Delivery decision owned by the user: push / open PR / merge (slice is ~590 lines, over the 400-line budget — needs `size:exception` or split per repo policy). diff --git a/odd/tasks/tree-delete-fields.md b/odd/tasks/tree-delete-fields.md new file mode 100644 index 0000000..1ec1004 --- /dev/null +++ b/odd/tasks/tree-delete-fields.md @@ -0,0 +1,194 @@ +# Feature: Delete document fields from Tree view + +- **Feature id**: `tree-delete-fields` +- **Repo locator**: `odd/tasks/tree-delete-fields.md` +- **Branch**: `feat/remove-fields` +- **First reviewed boundary**: `34923d6` (branch point) + +## Objective + +Let the user delete a field from a Firestore document directly in the Tree view, without having to hand-edit the document JSON. + +## Problem + +Today the only way to remove a field is the JSON editor. That works but it is not +intuitive: the user has to locate the key in a free-form blob and re-save the whole +document. Table view cannot host this either — `TableRow` only addresses top-level +keys (`doc.data?.[f]`) and collapses nested Maps/Arrays into one `JSON.stringify` +cell, so a nested key like `profile.displayName` is not addressable there. Tree view +already renders every field (including nested ones) as its own node with a path, so a +field is a first-class target for deletion there. + +## Why + +Tree is the only view where "a field of one document" is an unambiguous, addressable +node. Deleting there maps 1:1 onto the data model, and it covers nested fields that +table cannot even display as separate cells. + +## Scope (authorized edit roots) + +- `src/features/collections/**` +- `odd/tasks/tree-delete-fields.md` (this document) + +Out of scope (do not touch): + +- Table view delete affordance (possible follow-up, top-level fields only) +- JSON view changes +- Fixing the pre-existing nested **edit** limitation (see "Known pre-existing limitation") +- Array element deletion / splicing +- Electron IPC or auth-method changes — deletion reuses the existing `updateDocument` thunk + +## Constraints + +- Deletion must be explicit and confirmed: destructive and permanent. Never delete + on a single stray click. +- Offer delete only for **field** nodes (top-level keys and Map keys). Never on + Document or Collection nodes — those already have their own delete flows. +- Do **not** offer delete on Array _element_ nodes (index children). Deleting an + array element is a splice, not a field removal. +- Nested Maps/Arrays are deletable **as a field** (i.e. removing `profile` or `tags` + entirely). +- Persistence follows the existing pattern: `updateDocument` does a full + `setDocument`/`googleSetDocument` with the document data object, so omitting a key + removes it from Firestore. Do not introduce field-mask or `FieldValue.delete` + plumbing. +- UI copy and code comments in English (project convention). No AI attribution in + commits. Conventional Commits only. + +## TDD + +- **Mode**: `off` +- **Source**: no explicit project or session TDD configuration found +- **Runner**: `vitest` (`pnpm test`) + +Ordinary functional checks apply. `prepareDeleteData` is a pure function and MUST +ship with unit tests covering nested and top-level paths. + +## Delivery strategy + +- **Strategy**: `exception-ok` (maintainer-approved `size:exception`) +- **Forecast at creation**: ~185 authored changed lines — under the 400-line budget +- **Actual running count** from boundary `34923d6`: **421** (411+/10-) — over budget +- **Breakdown**: `odd/tasks/tree-delete-fields.md` 188 (this planning document) + + product code/tests 233 +- **Per commit**: `9044a77` = 252, `12469d2` = 197 — both individually under 400 +- **Chain strategy**: none — maintainer accepted `size:exception` for a single PR + instead of a chained split (2026-09-22) +- **Slice boundaries**: one PR holding `9044a77` and `12469d2` + +## Known pre-existing limitation (out of scope, follow-up candidate) + +`TreeEditingCell.field` carries only the leaf `nodeKey`, so editing a nested field +today writes to the top level of the document. Nested **delete** must NOT inherit +this: it needs a document-relative field path. Fixing nested **edit** is a separate +change. + +## Acceptance criteria + +1. In Tree view, a field node (top-level or nested Map key) exposes a visible delete + affordance on hover. +2. Activating it opens a confirmation that names the document id and the + document-relative field path (e.g. `profile.displayName`). Cancel leaves data + untouched. +3. Confirming removes exactly that key — and only that key — from the document in + Firestore, for both `google` and service-account auth methods (via the existing + `updateDocument` thunk). +4. Nested paths work: `profile.displayName` removes `displayName` from `profile` and + leaves sibling keys intact; `profile` removes the whole map. +5. Document nodes and Collection nodes show no delete affordance. Array element + nodes show none either. +6. `pnpm test`, `pnpm typecheck`, and `pnpm lint` all pass. + +## Applicable checks + +- `pnpm test` +- `pnpm typecheck` +- `pnpm lint` + +## Tasks + +### TD-1 — Field-path removal in `documentService` + unit tests + +- [x] Add `prepareDeleteData(doc, fieldPath)` to + `src/features/collections/services/documentService.ts`. - `fieldPath` is dot notation relative to `doc.data` + (`displayName`, `profile`, `profile.displayName`). - Returns a **new** object with that key removed; never mutates the input. - Missing intermediate path, or a non-Map intermediate (array/primitive), is a + no-op returning the data unchanged — do not throw. - Mirrors `prepareUpdateData` so the result can be passed straight to + `updateDocument`. +- [x] Add `src/features/collections/services/documentService.test.ts` covering: + top-level key, whole-map key, nested key, nested key under a missing parent + (no-op), non-Map intermediate (no-op), input not mutated. +- [x] Checks: `pnpm test`, `pnpm typecheck`, `pnpm lint` +- [x] Commit: `feat(collections): add field-path removal to document service` +- [x] Commit id: `9044a77` + +### TD-2 — Tree view delete affordance + wiring + +- [x] Thread a document-relative `fieldPath` through `TreeNodeRow` / `TreeContext` + (root fields get the key; nested fields get `parent.field`). Do not change the + existing `field`/`nodeKey` used by edit. +- [x] Add `onDeleteField(docId, fieldPath, docData, docCollectionPath)` to + `TreeContextValue`, provided by `TreeView`. +- [x] In `TreeNodeRow`, show a small delete icon on hover for field nodes only: + not `isDoc`, not `isCollection`, not array-element nodes. Keep the row dense; + the icon must not shift layout when it appears. +- [x] Confirmation dialog naming the document id and the field path, with explicit + Cancel / Delete actions. English copy. +- [x] Implement the handler in `CollectionTab`: `prepareDeleteData` then dispatch + `updateDocument`, then refresh/notify via the existing message path. +- [x] Checks: `pnpm test`, `pnpm typecheck`, `pnpm lint` +- [x] Commit: `feat(collections): allow deleting fields from documents in tree view` +- [x] Commit id: `12469d2` + +## Authorized scope notes for implementer + +Follow `work-unit-commits`: one commit per task above, tests and docs with the +behavior, Conventional Commit message, no `Co-Authored-By` or AI attribution. +Rollback boundary per commit is the files named in that task only. + +## Progress + +| Task | Status | Evidence | +| ---- | ------ | ---------------- | +| TD-1 | done | commit `9044a77` | +| TD-2 | done | commit `12469d2` | + +## Verification evidence + +### TD-1 — commit `9044a77` + +- `pnpm test`: 96 passed (13 files) — includes `documentService.test.ts`, 7 tests +- `pnpm typecheck`: clean, no output +- `pnpm lint` (scoped to changed files): `ESLint: No issues found` +- `pnpm lint` (full script): 1 pre-existing error in `firebaseController.js` + (`no-unused-vars`). File is untouched by this change — known environmental + failure, not introduced here. +- Runtime harness: N/A (Electron desktop UI; no runtime boundary exercised) + +Rollback boundary TD-1: `src/features/collections/services/documentService.ts`, +`src/features/collections/services/documentService.test.ts`. + +### TD-2 + +- `pnpm test`: 96 passed (13 files) +- `pnpm typecheck`: clean, no output +- `pnpm lint` (scoped to 4 changed files): `ESLint: No issues found` +- `pnpm lint` (full script): 1 pre-existing error in `firebaseController.js` + (same known environmental failure as TD-1) +- Runtime harness: N/A (Electron desktop UI; no runtime boundary exercised) + +Array-element vs Map-key distinction: `TreeNodeRow`'s nested recursion passes +`fieldPath` only when `!Array.isArray(value)`. Array children are index elements +and get no `fieldPath`, which is what disables their delete affordance. This +matches `prepareDeleteData`, whose non-Map-intermediate rule already rejects +`tags.0`-style paths. + +Rollback boundary TD-2: `src/features/collections/components/CollectionTab.tsx`, +`src/features/collections/components/TreeView.tsx`, +`src/features/collections/components/tree/TreeContext.ts`, +`src/features/collections/components/tree/TreeNodeRow.tsx`. + +## Next step + +Run the deferred native review for the PR slice. Risk was assessed `medium` +(`executable_change` on the test file) and deferred to slice close; the feature is +complete, so the preflight STATUS runs now with `--base-ref 34923d6 --committed-only`. diff --git a/src/features/collections/components/CollectionTab.tsx b/src/features/collections/components/CollectionTab.tsx index ad030df..ca69537 100644 --- a/src/features/collections/components/CollectionTab.tsx +++ b/src/features/collections/components/CollectionTab.tsx @@ -64,6 +64,7 @@ import { getErrorMessage, } from '../../../shared/utils'; import { generateJsQueryFromSimpleParams } from '../../../shared/utils/queryUtils'; +import { documentService } from '../services/documentService'; // Sub-components import QueryBar from './QueryBar'; @@ -75,6 +76,7 @@ import TreeView from './TreeView'; import JsonView from './JsonView'; import { useTreeSubcollections } from '../hooks/useTreeSubcollections'; import CreateDocumentDialog from './CreateDocumentDialog'; +import AddFieldDialog from './tree/AddFieldDialog'; import SettingsDialog from '../../../app/components/SettingsDialog'; type ViewMode = SettingsState['defaultViewType']; @@ -400,6 +402,12 @@ const CollectionTab: React.FC = ({ const [selectedRows, setSelectedRows] = useState([]); const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); const [deleteLoading, setDeleteLoading] = useState(false); + const [addFieldTarget, setAddFieldTarget] = useState<{ + docId: string; + parentPath: string; + docData: DocumentData; + collectionPath?: string; + } | null>(null); const [collectionPathInput, setCollectionPathInput] = useState(documentPath || collectionPath); useEffect(() => { @@ -611,6 +619,39 @@ const CollectionTab: React.FC = ({ [handleCellSave], ); + // Delete a field from a document. Reuses `updateDocument`, which writes the whole + // document data object, so omitting the key removes it from Firestore. + const handleDeleteField = useCallback( + async (docId: string, fieldPath: string, docData: DocumentData, docCollectionPath?: string) => { + // Subcollection documents live at their own collection path; root docs keep the tab's path + const targetCollectionPath = docCollectionPath ?? collectionPath; + const source: DocumentData | undefined = + docData && typeof docData === 'object' ? docData : documents.find((d) => d.id === docId)?.data; + if (!source) return; + + const newData = documentService.prepareDeleteData({ id: docId, data: source }, fieldPath); + + try { + await dispatch( + updateDocument({ + project, + collection: targetCollectionPath, + docId, + docData: newData, + firestoreDatabaseId, + }), + ).unwrap(); + showMessage?.(`Deleted field ${fieldPath} from document ${docId}`, 'success'); + if (targetCollectionPath !== collectionPath) { + refreshDocuments(targetCollectionPath); + } + } catch (error) { + showError(error); + } + }, + [documents, dispatch, project, collectionPath, firestoreDatabaseId, refreshDocuments, showMessage, showError], + ); + // JSON Save Handler const handleJsonSave = useCallback(async () => { try { @@ -698,6 +739,68 @@ const CollectionTab: React.FC = ({ } }, [selectedRows, project, collectionPath, firestoreDatabaseId, showMessage, dispatch, showError]); + // Add Field Handlers (Tree view) + const handleAddField = useCallback( + (docId: string, parentPath: string, docData: DocumentData, docCollectionPath?: string) => { + setAddFieldTarget({ docId, parentPath, docData, collectionPath: docCollectionPath }); + }, + [], + ); + + const handleAddFieldSubmit = useCallback( + async (fieldName: string, value: FirestoreValue) => { + if (!addFieldTarget) return; + const targetCollectionPath = addFieldTarget.collectionPath ?? collectionPath; + const docRecord = addFieldTarget.docData as unknown as Record; + const result = documentService.prepareAddData(docRecord, addFieldTarget.parentPath, fieldName, value); + if ('error' in result) { + showMessage?.(result.error, 'error'); + return; + } + try { + await dispatch( + updateDocument({ + project, + collection: targetCollectionPath, + docId: addFieldTarget.docId, + docData: result.data as DocumentData, + firestoreDatabaseId, + }), + ).unwrap(); + const location = addFieldTarget.parentPath + ? `${addFieldTarget.docId}.${addFieldTarget.parentPath}.${fieldName}` + : `${addFieldTarget.docId}.${fieldName}`; + showMessage?.(`Added field ${location}`, 'success'); + if (targetCollectionPath !== collectionPath) { + refreshDocuments(targetCollectionPath); + } + setAddFieldTarget(null); + } catch (error) { + showError(error); + } + }, + [addFieldTarget, collectionPath, dispatch, project, firestoreDatabaseId, refreshDocuments, showMessage, showError], + ); + + const addFieldExistingKeys = (() => { + if (!addFieldTarget) return [] as string[]; + let parent: unknown = addFieldTarget.docData as unknown; + if (addFieldTarget.parentPath) { + for (const segment of addFieldTarget.parentPath.split('.')) { + if (parent === null || typeof parent !== 'object' || Array.isArray(parent)) return []; + parent = (parent as Record)[segment]; + } + } + if (parent === null || typeof parent !== 'object' || Array.isArray(parent)) return []; + return Object.keys(parent as Record); + })(); + + const addFieldParentLabel = addFieldTarget + ? addFieldTarget.parentPath + ? `${addFieldTarget.docId}.${addFieldTarget.parentPath}` + : addFieldTarget.docId + : ''; + // Type utility wrappers for child components const getType = useCallback((value: FirestoreValue) => getValueType(value), []); const formatValue = useCallback((value: FirestoreValue, type: string) => formatDisplayValue(value, type), []); @@ -859,6 +962,8 @@ const CollectionTab: React.FC = ({ handleCellSave(); }} // Explicitly call handleCellSave onCellKeyDown={handleCellKeyDown} + onDeleteField={handleDeleteField} + onAddField={handleAddField} getType={getType} getTypeColor={getColor} formatValue={formatValue} @@ -894,6 +999,15 @@ const CollectionTab: React.FC = ({ onCreate={handleCreateDocument} /> + {/* Add Field Dialog (Tree view) */} + setAddFieldTarget(null)} + onSubmit={handleAddFieldSubmit} + /> + {/* Settings Dialog */} setSettingsDialogOpen(false)} /> diff --git a/src/features/collections/components/TreeView.tsx b/src/features/collections/components/TreeView.tsx index 23bb5b4..29e185c 100644 --- a/src/features/collections/components/TreeView.tsx +++ b/src/features/collections/components/TreeView.tsx @@ -22,6 +22,8 @@ interface TreeViewProps { ) => void; onCellSave: () => void; onCellKeyDown: (e: React.KeyboardEvent) => void; + onDeleteField: (docId: string, fieldPath: string, docData: DocumentData, docCollectionPath?: string) => void; + onAddField: (docId: string, parentPath: string, docData: DocumentData, docCollectionPath?: string) => void; getType: (value: FirestoreValue) => string; getTypeColor: (type: string, isDark: boolean) => string; formatValue: (value: FirestoreValue, type: string) => string; @@ -43,6 +45,8 @@ const TreeView: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, + onAddField, getType, getTypeColor, formatValue, @@ -121,6 +125,8 @@ const TreeView: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, + onAddField, getType, getTypeColor, formatValue, @@ -142,6 +148,8 @@ const TreeView: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, + onAddField, getType, getTypeColor, formatValue, diff --git a/src/features/collections/components/tree/AddFieldDialog.tsx b/src/features/collections/components/tree/AddFieldDialog.tsx new file mode 100644 index 0000000..18b558d --- /dev/null +++ b/src/features/collections/components/tree/AddFieldDialog.tsx @@ -0,0 +1,374 @@ +import React, { useEffect, useState } from 'react'; +import { + Button, + Dialog, + DialogActions, + DialogContent, + DialogTitle, + FormControl, + InputLabel, + MenuItem, + Select, + TextField, + Typography, +} from '@mui/material'; +import { FirestoreValue } from '../../../../shared/utils/firestoreUtils'; +import { formatDateForDateTimeLocal } from '../../../../shared/utils/dateUtils'; +import { MONOSPACE_FONT_FAMILY } from '../../../../shared/utils/constants'; + +export type AddFieldType = + 'String' | 'Integer' | 'Number' | 'Boolean' | 'Null' | 'Timestamp' | 'GeoPoint' | 'Array' | 'Map' | 'Reference'; + +const FIELD_TYPES: AddFieldType[] = [ + 'String', + 'Integer', + 'Number', + 'Boolean', + 'Null', + 'Timestamp', + 'GeoPoint', + 'Array', + 'Map', + 'Reference', +]; + +interface AddFieldDialogProps { + open: boolean; + parentPathLabel: string; + existingKeys: string[]; + onClose: () => void; + onSubmit: (fieldName: string, value: FirestoreValue) => void; +} + +const defaultTimestampLocal = (): string => { + try { + const formatted = formatDateForDateTimeLocal(new Date()); + if (formatted) return formatted.slice(0, 16); + } catch { + // fall through to ISO fallback + } + return new Date().toISOString().slice(0, 16); +}; + +/** + * AddFieldDialog Component + * Type-aware dialog for adding a field to a document or nested map in Tree view + */ +const AddFieldDialog: React.FC = ({ open, parentPathLabel, existingKeys, onClose, onSubmit }) => { + const [fieldName, setFieldName] = useState(''); + const [fieldType, setFieldType] = useState('String'); + const [stringValue, setStringValue] = useState(''); + const [numberValue, setNumberValue] = useState('0'); + const [boolValue, setBoolValue] = useState('true'); + const [timestampValue, setTimestampValue] = useState(defaultTimestampLocal()); + const [latValue, setLatValue] = useState('0'); + const [lngValue, setLngValue] = useState('0'); + const [jsonValue, setJsonValue] = useState('[]'); + const [referenceValue, setReferenceValue] = useState(''); + + useEffect(() => { + if (open) { + setFieldName(''); + setFieldType('String'); + setStringValue(''); + setNumberValue('0'); + setBoolValue('true'); + setTimestampValue(defaultTimestampLocal()); + setLatValue('0'); + setLngValue('0'); + setJsonValue('[]'); + setReferenceValue(''); + } + }, [open]); + + useEffect(() => { + if (fieldType === 'Array') setJsonValue('[]'); + if (fieldType === 'Map') setJsonValue('{}'); + }, [fieldType]); + + const trimmedName = fieldName.trim(); + const nameError = !trimmedName + ? 'Field name is required' + : trimmedName.includes('.') + ? 'Field name cannot contain dots' + : existingKeys.includes(trimmedName) + ? 'Field already exists' + : ''; + + const jsonError = (() => { + if (fieldType !== 'Array' && fieldType !== 'Map') return ''; + try { + const parsed = JSON.parse(jsonValue); + if (fieldType === 'Array' && !Array.isArray(parsed)) return 'Must be a valid JSON array'; + if (fieldType === 'Map' && (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed))) + return 'Must be a valid JSON object'; + return ''; + } catch { + return 'Invalid JSON'; + } + })(); + + const geoError = (() => { + if (fieldType !== 'GeoPoint') return ''; + if (latValue.trim() === '' || lngValue.trim() === '' || isNaN(Number(latValue)) || isNaN(Number(lngValue))) + return 'Latitude and longitude must be numbers'; + return ''; + })(); + + const numberError = (() => { + if (fieldType !== 'Integer' && fieldType !== 'Number') return ''; + if (numberValue.trim() === '' || isNaN(Number(numberValue))) return 'Must be a valid number'; + return ''; + })(); + + const timestampError = (() => { + if (fieldType !== 'Timestamp') return ''; + if (timestampValue.trim() === '' || isNaN(new Date(timestampValue).getTime())) + return 'Must be a valid date and time'; + return ''; + })(); + + const isValid = !nameError && !jsonError && !geoError && !numberError && !timestampError; + + const buildValue = (): FirestoreValue => { + switch (fieldType) { + case 'String': + return stringValue; + case 'Integer': + return Math.trunc(Number(numberValue)); + case 'Number': + return Number(numberValue); + case 'Boolean': + return boolValue === 'true'; + case 'Null': + return null; + case 'Timestamp': { + const date = new Date(timestampValue); + return { _seconds: Math.floor(date.getTime() / 1000), _nanoseconds: 0 }; + } + case 'GeoPoint': + return { _latitude: Number(latValue), _longitude: Number(lngValue) }; + case 'Array': + case 'Map': + return JSON.parse(jsonValue) as FirestoreValue; + case 'Reference': + return referenceValue; + default: + return stringValue; + } + }; + + const handleSave = () => { + if (!isValid) return; + onSubmit(trimmedName, buildValue()); + }; + + const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'Enter' && (e.ctrlKey || e.metaKey)) { + e.preventDefault(); + handleSave(); + } + }; + + return ( + + + Add Field + + {parentPathLabel} + + + + + Type: {fieldType} + + setFieldName(e.target.value)} + onKeyDown={handleKeyDown} + error={Boolean(nameError)} + helperText={nameError || 'Name for the new field'} + sx={{ mt: 1 }} + /> + + Type + + + + {fieldType === 'String' && ( + setStringValue(e.target.value)} + onKeyDown={handleKeyDown} + sx={{ + mt: 2, + '& .MuiInputBase-input': { fontFamily: MONOSPACE_FONT_FAMILY, fontSize: '0.9rem' }, + }} + /> + )} + {(fieldType === 'Integer' || fieldType === 'Number') && ( + setNumberValue(e.target.value)} + onKeyDown={handleKeyDown} + error={Boolean(numberError)} + helperText={numberError || undefined} + sx={{ + mt: 2, + '& .MuiInputBase-input': { fontFamily: MONOSPACE_FONT_FAMILY, fontSize: '0.9rem' }, + }} + /> + )} + {fieldType === 'Boolean' && ( + + Value + + + )} + {fieldType === 'Null' && ( + + No value needed — saves as null. + + )} + {fieldType === 'Timestamp' && ( + setTimestampValue(e.target.value)} + onKeyDown={handleKeyDown} + InputLabelProps={{ shrink: true }} + error={Boolean(timestampError)} + helperText={timestampError || undefined} + sx={{ + mt: 2, + '& .MuiInputBase-input': { fontFamily: MONOSPACE_FONT_FAMILY, fontSize: '0.9rem' }, + }} + /> + )} + {fieldType === 'GeoPoint' && ( + <> + setLatValue(e.target.value)} + onKeyDown={handleKeyDown} + sx={{ + mt: 2, + '& .MuiInputBase-input': { fontFamily: MONOSPACE_FONT_FAMILY, fontSize: '0.9rem' }, + }} + /> + setLngValue(e.target.value)} + onKeyDown={handleKeyDown} + sx={{ + mt: 2, + '& .MuiInputBase-input': { fontFamily: MONOSPACE_FONT_FAMILY, fontSize: '0.9rem' }, + }} + /> + {geoError && ( + + {geoError} + + )} + + )} + {(fieldType === 'Array' || fieldType === 'Map') && ( + setJsonValue(e.target.value)} + onKeyDown={handleKeyDown} + error={Boolean(jsonError)} + helperText={ + jsonError || (fieldType === 'Array' ? 'JSON array, e.g. ["a", 1]' : 'JSON object, e.g. {"k": "v"}') + } + placeholder={fieldType === 'Array' ? '["item1", "item2"]' : '{"key": "value"}'} + sx={{ + mt: 2, + '& .MuiInputBase-input': { + fontFamily: MONOSPACE_FONT_FAMILY, + fontSize: '0.9rem', + lineHeight: 1.5, + }, + }} + /> + )} + {fieldType === 'Reference' && ( + setReferenceValue(e.target.value)} + onKeyDown={handleKeyDown} + placeholder="projects/p/databases/d/documents/c/doc" + sx={{ + mt: 2, + '& .MuiInputBase-input': { fontFamily: MONOSPACE_FONT_FAMILY, fontSize: '0.9rem' }, + }} + /> + )} + + Press Ctrl+Enter to save, Escape to cancel + + + + + + + + ); +}; + +export default AddFieldDialog; diff --git a/src/features/collections/components/tree/TreeContext.ts b/src/features/collections/components/tree/TreeContext.ts index c360511..a2dd7f5 100644 --- a/src/features/collections/components/tree/TreeContext.ts +++ b/src/features/collections/components/tree/TreeContext.ts @@ -28,6 +28,12 @@ export interface TreeContextValue { ) => void; onCellSave: () => void; onCellKeyDown: (e: React.KeyboardEvent) => void; + /** + * Permanently removes a field from a document. + * @param fieldPath - Document-relative dot path (e.g. "profile.displayName") + */ + onDeleteField: (docId: string, fieldPath: string, docData: DocumentData, docCollectionPath?: string) => void; + onAddField: (docId: string, parentPath: string, docData: DocumentData, docCollectionPath?: string) => void; getType: (value: FirestoreValue) => string; getTypeColor: (type: string, isDark: boolean) => string; formatValue: (value: FirestoreValue, type: string) => string; diff --git a/src/features/collections/components/tree/TreeNodeRow.tsx b/src/features/collections/components/tree/TreeNodeRow.tsx index f370301..8b28c75 100644 --- a/src/features/collections/components/tree/TreeNodeRow.tsx +++ b/src/features/collections/components/tree/TreeNodeRow.tsx @@ -1,10 +1,24 @@ -import React, { useContext, useEffect } from 'react'; -import { Box, IconButton, TableCell, TableRow, TextField, Typography } from '@mui/material'; +import React, { useContext, useEffect, useState } from 'react'; +import { + Box, + Button, + Dialog, + DialogActions, + DialogContent, + DialogTitle, + IconButton, + TableCell, + TableRow, + TextField, + Typography, +} from '@mui/material'; import { ExpandMore as ExpandMoreIcon, ChevronRight as ChevronRightIcon, Storage as CollectionIcon, Description as DocumentIcon, + DeleteOutline as DeleteOutlineIcon, + Add as AddIcon, } from '@mui/icons-material'; import { FirestoreValue } from '../../../../shared/utils/firestoreUtils'; import { @@ -15,6 +29,7 @@ import { import { DocumentData } from '../../store/collectionSlice'; import { TreeContext } from './TreeContext'; import { singleLineTruncation } from '../../../../shared/ui/textStyles'; +import { MONOSPACE_FONT_FAMILY } from '../../../../shared/utils/constants'; interface TreeNodeRowProps { nodeKey: string; @@ -27,6 +42,12 @@ interface TreeNodeRowProps { isDoc?: boolean; isCollection?: boolean; missing?: boolean; + /** + * Document-relative dot path of this field (e.g. "profile.displayName"). + * Omitted for documents, collections and array elements — those are not + * deletable fields, and their presence disables the delete affordance. + */ + fieldPath?: string; } const TreeNodeRow: React.FC = ({ @@ -40,6 +61,7 @@ const TreeNodeRow: React.FC = ({ isDoc = false, isCollection = false, missing = false, + fieldPath, }) => { const ctx = useContext(TreeContext); if (!ctx) throw new Error('TreeNodeRow must be rendered inside a TreeContext provider'); @@ -54,6 +76,8 @@ const TreeNodeRow: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, + onAddField, getType, getTypeColor, formatValue, @@ -66,6 +90,30 @@ const TreeNodeRow: React.FC = ({ const nodeType = isCollection ? 'Collection' : isDoc ? 'Document' : getType(value); const isExpandable = isCollection || isDoc || nodeType === 'Array' || nodeType === 'Map'; + // Only real fields (top-level keys and map keys) are deletable. `fieldPath` is + // deliberately omitted for documents, collections and array elements. + const canDeleteField = Boolean(fieldPath && docId && docData && !isDoc && !isCollection); + const isMapRow = !isCollection && !isDoc && nodeType === 'Map'; + const showAdd = isDoc || isMapRow; + const parentPath = (() => { + if (isDoc) return ''; + if (!isMapRow || !docId) return ''; + const prefix = docCollectionPath ? `${docCollectionPath}/${docId}.` : null; + if (prefix && path.startsWith(prefix)) return path.slice(prefix.length); + // Fallback when the collection path is unavailable: strip up to the + // document boundary instead of returning the bare key, so deeply nested + // maps still resolve to their full relative parent path. + const marker = `${docId}.`; + const idx = path.indexOf(marker); + if (idx >= 0) return path.slice(idx + marker.length); + return nodeKey; + })(); + + const handleAddField = (e: React.MouseEvent) => { + e.stopPropagation(); + if (!docId || !docData) return; + onAddField(docId, parentPath, docData, docCollectionPath); + }; const isExpanded = expandedNodes[path]; const displayValue = isExpandable ? '' : formatValue(value, nodeType); const isEditing = @@ -78,6 +126,7 @@ const TreeNodeRow: React.FC = ({ const isDateLike = nodeType === 'Timestamp' || isFirestoreTimestamp(value) || isUnixTimestampMs(value); const [dateValue, setDateValue] = React.useState(''); + const [pendingDelete, setPendingDelete] = useState(null); useEffect(() => { if (isEditing && isDateLike) { @@ -100,7 +149,13 @@ const TreeNodeRow: React.FC = ({ return ( <> - + = ({ > {nodeKey} + {showAdd && ( + + + + )} @@ -204,12 +271,32 @@ const TreeNodeRow: React.FC = ({ )} - - {nodeType} - + + + {nodeType} + + {/* Fixed-width slot so the row does not shift when the button appears on hover. */} + + {canDeleteField && ( + { + event.stopPropagation(); + if (fieldPath) setPendingDelete(fieldPath); + }} + sx={{ p: 0.25, width: 20, height: 20, visibility: 'hidden', color: 'error.main' }} + > + + + )} + + @@ -254,6 +341,9 @@ const TreeNodeRow: React.FC = ({ docData={docData} docCollectionPath={docCollectionPath} depth={depth + 1} + // Array children are index elements, not fields: deleting one would + // be a splice, so they get no fieldPath and no delete affordance. + fieldPath={fieldPath && !Array.isArray(value) ? `${fieldPath}.${k}` : undefined} /> ))} @@ -271,6 +361,7 @@ const TreeNodeRow: React.FC = ({ docData={docData} docCollectionPath={docCollectionPath} depth={depth + 1} + fieldPath={k} /> ))} {(subcollectionIds ?? []).map((id) => ( @@ -287,6 +378,40 @@ const TreeNodeRow: React.FC = ({ )} )} + + setPendingDelete(null)} maxWidth="xs" fullWidth> + Delete field? + + + This permanently removes the field{' '} + + {pendingDelete} + {' '} + from document{' '} + + {docId} + + . This cannot be undone. + + + + + + + ); }; diff --git a/src/features/collections/services/documentService.test.ts b/src/features/collections/services/documentService.test.ts new file mode 100644 index 0000000..9a2b2b8 --- /dev/null +++ b/src/features/collections/services/documentService.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from 'vitest'; +import { documentService } from './documentService'; + +describe('prepareDeleteData', () => { + it('removes a top-level key', () => { + const doc = { id: 'doc-1', data: { a: 1, b: 'keep' } }; + expect(documentService.prepareDeleteData(doc, 'a')).toEqual({ b: 'keep' }); + }); + + it('removes a whole map field', () => { + const doc = { id: 'doc-1', data: { profile: { displayName: 'Ada', age: 36 }, a: 1 } }; + expect(documentService.prepareDeleteData(doc, 'profile')).toEqual({ a: 1 }); + }); + + it('removes a nested key and keeps sibling keys intact', () => { + const doc = { id: 'doc-1', data: { profile: { displayName: 'Ada', age: 36 }, tags: ['a'] } }; + expect(documentService.prepareDeleteData(doc, 'profile.displayName')).toEqual({ + profile: { age: 36 }, + tags: ['a'], + }); + }); + + it('removes a deeply nested key and keeps sibling keys intact', () => { + const doc = { id: 'doc-1', data: { a: { b: { c: 1, d: 2 } } } }; + expect(documentService.prepareDeleteData(doc, 'a.b.c')).toEqual({ a: { b: { d: 2 } } }); + }); + + it('is a no-op when a nested key lives under a missing parent', () => { + const doc = { id: 'doc-1', data: { a: 1 } }; + expect(documentService.prepareDeleteData(doc, 'profile.displayName')).toEqual({ a: 1 }); + }); + + it('is a no-op when an intermediate path is not a map', () => { + const withArray = { id: 'doc-1', data: { tags: ['x', 'y'] } }; + expect(documentService.prepareDeleteData(withArray, 'tags.0')).toEqual({ tags: ['x', 'y'] }); + + const withPrimitive = { id: 'doc-1', data: { count: 1 } }; + expect(documentService.prepareDeleteData(withPrimitive, 'count.total')).toEqual({ count: 1 }); + }); + + it('returns a new object and does not mutate the input data', () => { + const data = { profile: { displayName: 'Ada', age: 36 }, tags: ['a', 'b'] }; + const snapshot = JSON.stringify(data); + + const result = documentService.prepareDeleteData({ id: 'doc-1', data }, 'profile.displayName'); + expect(result).not.toBe(data); + expect(result.profile).not.toBe(data.profile); + expect(JSON.stringify(data)).toBe(snapshot); + + documentService.prepareDeleteData({ id: 'doc-1', data }, 'profile'); + documentService.prepareDeleteData({ id: 'doc-1', data }, 'tags.0'); + expect(JSON.stringify(data)).toBe(snapshot); + }); +}); diff --git a/src/features/collections/services/documentService.ts b/src/features/collections/services/documentService.ts index f833893..363ea8c 100644 --- a/src/features/collections/services/documentService.ts +++ b/src/features/collections/services/documentService.ts @@ -4,7 +4,11 @@ * Extracted from CollectionTab.jsx */ -import { FirestoreValue, FirestoreTimestamp as SharedFirestoreTimestamp } from '../../../shared/utils/firestoreUtils'; +import { + FirestoreValue, + FirestoreTimestamp as SharedFirestoreTimestamp, + getValueType, +} from '../../../shared/utils/firestoreUtils'; interface FirestoreTimestamp { _seconds: number; @@ -112,6 +116,79 @@ export const documentService = { const transformedValue = this.transformValueForSave(oldValue as FirestoreValue, newValue); return { ...doc.data, [field]: transformedValue }; }, + + /** + * Prepare document data after removing a field + * @param doc - Original document + * @param fieldPath - Dot notation path of the field to remove, relative to doc.data + * @returns New document data with the field removed. A missing intermediate path, + * or a non-Map intermediate (array/primitive), is a no-op returning the + * data unchanged + */ + prepareDeleteData(doc: DocumentData, fieldPath: string): Record { + const data: Record = { ...doc.data }; + const segments = fieldPath.split('.'); + let target = data; + for (let i = 0; i < segments.length - 1; i += 1) { + const child = target[segments[i]]; + if (getValueType(child) !== 'Map') { + return data; + } + const childCopy = { ...(child as Record) }; + target[segments[i]] = childCopy; + target = childCopy; + } + delete target[segments[segments.length - 1]]; + return data; + }, + + /** + * Prepare document data with a new field added at a nested dot-path. + * @param docData - Current document fields + * @param parentPath - Dot-path of the parent object relative to the doc ("" for doc root) + * @param fieldName - New field name (single segment, no dots) + * @param value - Value for the new field + * @returns { data } on success or { error } describing why the add was rejected + */ + prepareAddData( + docData: Record, + parentPath: string, + fieldName: string, + value: FirestoreValue, + ): { data: Record } | { error: string } { + const name = fieldName.trim(); + if (!name) return { error: 'Field name is required' }; + if (name.includes('.')) return { error: 'Field name cannot contain dots' }; + + let cloned: Record; + try { + cloned = + typeof structuredClone === 'function' + ? (structuredClone(docData ?? {}) as Record) + : (JSON.parse(JSON.stringify(docData ?? {})) as Record); + } catch { + try { + cloned = JSON.parse(JSON.stringify(docData ?? {})) as Record; + } catch { + return { error: 'Document data is not plain serializable data' }; + } + } + + let parent: unknown = cloned; + if (parentPath) { + for (const segment of parentPath.split('.')) { + if (Array.isArray(parent)) return { error: 'Cannot add fields inside an Array' }; + if (parent === null || typeof parent !== 'object') return { error: 'Parent path does not exist' }; + parent = (parent as Record)[segment]; + } + } + if (Array.isArray(parent)) return { error: 'Cannot add fields inside an Array' }; + if (parent === null || typeof parent !== 'object') return { error: 'Parent path does not exist' }; + const parentObj = parent as Record; + if (Object.prototype.hasOwnProperty.call(parentObj, name)) return { error: 'Field already exists' }; + parentObj[name] = value; + return { data: cloned }; + }, }; export default documentService;