Skip to content

feat(collections): allow deleting fields from documents in tree view - #20

Closed
EMTumariscal wants to merge 3 commits into
Flowdesktech:masterfrom
EMTumariscal:feat/remove-fields
Closed

EMTumariscal wants to merge 3 commits into
Flowdesktech:masterfrom
EMTumariscal:feat/remove-fields

Conversation

@EMTumariscal

Copy link
Copy Markdown
Contributor

Description

Adds the ability to delete a field from a Firestore document directly in the Tree view, without hand-editing the document JSON.

Deleting a field was previously only possible in the JSON editor: the user had 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 a single 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 deletion target there.

Two work units:

  1. documentService.prepareDeleteData(doc, fieldPath) — dot-notation field removal relative to doc.data. Returns a new object (never mutates the input), no-op when an intermediate path is missing or is not a Map, never throws. Covered by 7 unit tests.
  2. Tree view affordance — document-relative fieldPath threaded through TreeNodeRow/TreeContext, a hover delete icon (fixed 20px slot so the row never shifts), a confirmation dialog naming the document id and the field path, and handleDeleteField in CollectionTab which reuses the existing updateDocument thunk.

Size note (PR Size policy): 421 authored changed lines across 7 files — 233 lines of product code + tests, 188 lines of the ODD planning document odd/tasks/tree-delete-fields.md. The maintainer explicitly approved size:exception for a single PR rather than a chained split. The two work-unit commits are 252 and 197 lines each and would individually fit under a 400-line budget if a split is preferred during review.

Type of Change

  • Feature
  • Bug fix
  • Docs
  • Refactor

AI Code Generation

This change was produced with AI assistance and is disclosed here as required.

  • Tool / model: OpenCode (opencode-go/mimo-v2.6-pro) driving the Gentle AI orchestrator
  • Human involvement: The human stated the requirement (delete fields from documents), chose the host view (Tree over Table) from a presented trade-off analysis, approved the delivery strategy (size:exception, single PR), and explicitly granted the native review candidate ("Revisar este cambio" -> granted).
  • Scope of AI work: Implementation, unit tests, and the planning document in this PR were authored by the AI under human direction.
  • Verification: a native review-reliability lens reviewed the frozen candidate and returned 0 findings before this PR was opened. Human review of this PR remains required.
  • Commit attribution: no Co-Authored-By or AI attribution was added to any commit.

Related Issue

N/A. No open or closed issue in Flowdesktech/firestudio covers field deletion. Issues #2, #3, #4 and #16 are unrelated (generic feature request, queries, local emulator, storage file limit).

Technical Context

  • Why Tree and not Table: in TableRow a "field" is a shared column across every document, so "delete this field" is ambiguous (one cell vs. the whole column — and hiddenColumns already covers "hide in the view", which must not be confused with "delete in the database"). TreeNodeRow already computes a per-document path, and nested Maps/Arrays are rendered as child nodes, making it the only view where profile.displayName is an addressable target. Table also only reads doc.data?.[f] (top-level), so nested keys are not addressable there at all.
  • Persistence: reuses updateDocument (src/features/collections/store/collectionSlice.ts), which writes the whole document data object via setDocument/googleSetDocument. Omitting a key therefore removes it from Firestore. No field-mask or FieldValue.delete plumbing was introduced, so both the google and service-account auth methods work unchanged. This mirrors the existing prepareUpdateData shape exactly.
  • Array vs Map: TreeNodeRow's nested recursion maps Object.entries(value) for both Maps and Arrays. fieldPath is passed only when !Array.isArray(value), which suppresses the affordance on array indices — deleting an array element is a splice, not a field removal. A Map/Array node itself remains deletable as one field (profile, tags). This matches prepareDeleteData, whose non-Map-intermediate rule already rejects tags.0-style paths.
  • Destructive-action safety: deletion is never one click. A confirmation dialog names the document id and the exact document-relative field path before anything is written.
  • Known pre-existing limitation (deliberately not addressed here, to keep the PR focused): TreeEditingCell.field carries only the leaf nodeKey, so editing a nested field writes to the top level of the document. Nested delete does not inherit this bug (it uses the real document-relative fieldPath). Fixing nested edit is a separate change.

Test Environment

  • Hardware: x86_64 Linux development machine
  • OS: Linux
  • Model / provider: opencode-go/mimo-v2.6-pro via OpenCode
  • Channels used for testing: the local pnpm script suite that .github/workflows/ci.yml runs on pull_request (lint, format:check, typecheck, test). No manual Electron UI session was exercised for this PR; the behaviour change is covered by the unit tests listed under Evidence.

Evidence

CI-equivalent checks, run locally on the exact commands from .github/workflows/ci.yml:

$ ./node_modules/.bin/eslint "src/**/*.{ts,tsx,js,jsx}" --report-unused-disable-directives --max-warnings 0
===CI_LINT_EXIT=0===

$ pnpm run format:check
Checking formatting...
All matched files use Prettier code style!

$ pnpm run typecheck
> tsc -p tsconfig.json --noEmit
(clean, no output)

$ pnpm run test
Test Files  13 passed (13)
     Tests  96 passed (96)
   Duration  5.61s

New unit tests in src/features/collections/services/documentService.test.ts (7 cases): top-level key removal, whole-map key removal, nested key removal with siblings preserved, deeply nested key removal, missing parent (no-op), non-Map intermediate (no-op, covering both tags.0 and a primitive parent), and non-mutation of the input object.

Native review: review-reliability lens on the frozen candidate -> 0 findings, state approved, acknowledged with authority: burned.

Process note: this repository has no Makefile, so make check cannot be run as literally specified. The four checks CI runs were executed individually instead (results above), and all pass. There is also no PULL_REQUEST_TEMPLATE in either EMTumariscal/firestudio or Flowdesktech/firestudio; this description follows the documented PR Template Sections verbatim instead.

Checklist

  • Self-review completed
  • Change is focused — no unrelated changes bundled
  • Tests added for new logic (prepareDeleteData, 7 cases)
  • Existing tests still pass (96/96)
  • Typecheck and lint pass locally on the same commands CI runs
  • Formatting passes (prettier --check .)
  • AI involvement disclosed above
  • No Co-Authored-By or AI attribution added to commits
  • Docs updated (odd/tasks/tree-delete-fields.md tracks scope, decisions, verification evidence and commit ids)

@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