feat(collections): allow deleting fields from documents in tree view - #20
Closed
EMTumariscal wants to merge 3 commits into
Closed
EMTumariscal wants to merge 3 commits into
EMTumariscal wants to merge 3 commits into
Conversation
10 tasks
Contributor
Author
|
Superseded by #22 — consolidated PR from EMTumariscal:develop to master bundling this change with its original description preserved. Closing per maintainer request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 —
TableRowonly addresses top-level keys (doc.data?.[f]) and collapses nested Maps/Arrays into a singleJSON.stringifycell, so a nested key likeprofile.displayNameis 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:
documentService.prepareDeleteData(doc, fieldPath)— dot-notation field removal relative todoc.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.fieldPaththreaded throughTreeNodeRow/TreeContext, a hover delete icon (fixed 20px slot so the row never shifts), a confirmation dialog naming the document id and the field path, andhandleDeleteFieldinCollectionTabwhich reuses the existingupdateDocumentthunk.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 approvedsize:exceptionfor 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
AI Code Generation
This change was produced with AI assistance and is disclosed here as required.
opencode-go/mimo-v2.6-pro) driving the Gentle AI orchestratorsize:exception, single PR), and explicitly granted the native review candidate ("Revisar este cambio" ->granted).review-reliabilitylens reviewed the frozen candidate and returned 0 findings before this PR was opened. Human review of this PR remains required.Co-Authored-Byor AI attribution was added to any commit.Related Issue
N/A. No open or closed issue in
Flowdesktech/firestudiocovers field deletion. Issues #2, #3, #4 and #16 are unrelated (generic feature request, queries, local emulator, storage file limit).Technical Context
TableRowa "field" is a shared column across every document, so "delete this field" is ambiguous (one cell vs. the whole column — andhiddenColumnsalready covers "hide in the view", which must not be confused with "delete in the database").TreeNodeRowalready computes a per-document path, and nested Maps/Arrays are rendered as child nodes, making it the only view whereprofile.displayNameis an addressable target. Table also only readsdoc.data?.[f](top-level), so nested keys are not addressable there at all.updateDocument(src/features/collections/store/collectionSlice.ts), which writes the whole document data object viasetDocument/googleSetDocument. Omitting a key therefore removes it from Firestore. No field-mask orFieldValue.deleteplumbing was introduced, so both thegoogleand service-account auth methods work unchanged. This mirrors the existingprepareUpdateDatashape exactly.TreeNodeRow's nested recursion mapsObject.entries(value)for both Maps and Arrays.fieldPathis 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 matchesprepareDeleteData, whose non-Map-intermediate rule already rejectstags.0-style paths.TreeEditingCell.fieldcarries only the leafnodeKey, 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-relativefieldPath). Fixing nested edit is a separate change.Test Environment
opencode-go/mimo-v2.6-provia OpenCodepnpmscript suite that.github/workflows/ci.ymlruns onpull_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: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 bothtags.0and a primitive parent), and non-mutation of the input object.Native review:
review-reliabilitylens on the frozen candidate -> 0 findings, stateapproved, acknowledged withauthority: burned.Process note: this repository has no
Makefile, somake checkcannot be run as literally specified. The four checks CI runs were executed individually instead (results above), and all pass. There is also noPULL_REQUEST_TEMPLATEin eitherEMTumariscal/firestudioorFlowdesktech/firestudio; this description follows the documented PR Template Sections verbatim instead.Checklist
prepareDeleteData, 7 cases)prettier --check .)Co-Authored-Byor AI attribution added to commitsodd/tasks/tree-delete-fields.mdtracks scope, decisions, verification evidence and commit ids)