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..289003a 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'; @@ -611,6 +612,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 { @@ -859,6 +893,7 @@ const CollectionTab: React.FC = ({ handleCellSave(); }} // Explicitly call handleCellSave onCellKeyDown={handleCellKeyDown} + onDeleteField={handleDeleteField} getType={getType} getTypeColor={getColor} formatValue={formatValue} diff --git a/src/features/collections/components/TreeView.tsx b/src/features/collections/components/TreeView.tsx index 23bb5b4..2791542 100644 --- a/src/features/collections/components/TreeView.tsx +++ b/src/features/collections/components/TreeView.tsx @@ -22,6 +22,7 @@ interface TreeViewProps { ) => void; onCellSave: () => void; onCellKeyDown: (e: React.KeyboardEvent) => void; + onDeleteField: (docId: string, fieldPath: string, docData: DocumentData, docCollectionPath?: string) => void; getType: (value: FirestoreValue) => string; getTypeColor: (type: string, isDark: boolean) => string; formatValue: (value: FirestoreValue, type: string) => string; @@ -43,6 +44,7 @@ const TreeView: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, getType, getTypeColor, formatValue, @@ -121,6 +123,7 @@ const TreeView: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, getType, getTypeColor, formatValue, @@ -142,6 +145,7 @@ const TreeView: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, getType, getTypeColor, formatValue, diff --git a/src/features/collections/components/tree/TreeContext.ts b/src/features/collections/components/tree/TreeContext.ts index c360511..89c35f5 100644 --- a/src/features/collections/components/tree/TreeContext.ts +++ b/src/features/collections/components/tree/TreeContext.ts @@ -28,6 +28,11 @@ 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; 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..5a2e34c 100644 --- a/src/features/collections/components/tree/TreeNodeRow.tsx +++ b/src/features/collections/components/tree/TreeNodeRow.tsx @@ -1,10 +1,23 @@ -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, } from '@mui/icons-material'; import { FirestoreValue } from '../../../../shared/utils/firestoreUtils'; import { @@ -15,6 +28,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 +41,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 +60,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 +75,7 @@ const TreeNodeRow: React.FC = ({ onCellEdit, onCellSave, onCellKeyDown, + onDeleteField, getType, getTypeColor, formatValue, @@ -66,6 +88,9 @@ 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 isExpanded = expandedNodes[path]; const displayValue = isExpandable ? '' : formatValue(value, nodeType); const isEditing = @@ -78,6 +103,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 +126,9 @@ const TreeNodeRow: React.FC = ({ return ( <> - + = ({ )} - - {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 +302,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 +322,7 @@ const TreeNodeRow: React.FC = ({ docData={docData} docCollectionPath={docCollectionPath} depth={depth + 1} + fieldPath={k} /> ))} {(subcollectionIds ?? []).map((id) => ( @@ -287,6 +339,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..d53970e 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,31 @@ 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; + }, }; export default documentService;