[malibio]

Closes #3455

What changed

The dev-proxy forwarded collection changes under field names the proto doesn't have, so they never reached the daemon.

• Update (PATCH /api/nodes/:id) sent addToCollection / removeFromCollection. It now forwards UpdateNodeRequest's repeated addToCollections (paths), addToCollectionIds and removeFromCollectionIds.
• Create (POST /api/nodes) sent collection. It now forwards CreateNodeRequest's repeated collections (paths) and collectionIds.
• Both routes refuse a value that is not an array of strings with a 400, rather than dropping it.
• Browser mode can now change membership. HttpCollectionService.addNodeToCollection and removeNodeFromCollection did nothing; they now send the node update with addToCollectionIds / removeFromCollectionIds. addNodeToCollectionPath is still a stub: it must return the leaf collection id, which the update response does not identify.

The paths-or-ids rule is not duplicated in the proxy. The daemon already rejects a request carrying both, before it writes anything, and the proxy returns that as a 400.

Parity with the Tauri path

The issue asks to match what the Tauri path sends. The Tauri update_node command sends no collection fields: NodeUpdate has none, and the command always passes empty lists. The Tauri app changes membership through separate collection commands. Browser mode now reaches the same daemon code for add and remove by id, through the node update rather than those commands.

One difference: the update path looks the node up first, so a missing node returns NOT_FOUND.

Tests

• New e2e file node-collection-fields.e2e.ts, run against a real proxy and daemon (9 cases):
  • update: add by path, add by id then remove by id, a content change sent with a collection change
  • create: by path, by id
  • both routes: a path and an id on one request returns 400 INVALID_ARGUMENT and writes nothing; a non-array value returns 400
• collection-service.test.ts gains three cases for the two membership methods.
• The harness gains a baseUrl getter, because the adapter has no method that sends these fields.

The four cases from the first commit failed without the proxy change. bun run test:changed passed (frontend and browser tiers); bun run quality:fix made no changes. The collection UI was not driven in a browser.

Also: the usage examples in node.ts showed updateNode(..., { add_to_collections }), which works on neither transport; they now point at collectionService.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3456 (initial review)

Code Review Summary

Verdict: APPROVE. The fix is correct, small and proven by a real proxy + daemon test. Two non-blocking gaps are worth closing in this PR: the same bug exists on the create route, and the browser app still cannot change membership.

What I checked against the code rather than the description:

• Field mapping is right. UpdateNodeRequest declares add_to_collections = 6, remove_from_collection_ids = 7, add_to_collection_ids = 9, all repeated (packages/proto/proto/node_service.proto:407-418). The loader runs with keepCase: false (packages/dev-tools/src/grpc-client.ts:204), so the camelCase keys the handler now sets are the ones that serialize. The old singular keys matched nothing.
• The paths-or-ids rule is enforced once, before any write. node_ops::update_node returns InvalidParams as its first statement (packages/core/src/ops/node_ops.rs:271), ahead of the node update and the collection loop. Leaving it out of the proxy is the right call (DRY); the proxy maps INVALID_ARGUMENT to 400.
• I ran the new test file (bun run --cwd packages/desktop-app test:e2e -- src/tests/e2e/node-update-collections.e2e.ts): 4 of 4 passed in 2.6s against the worktree's sidecar daemon and the proxy from source.
• "All four fail without the proxy change" — not run against the old code (review only, no edits), but it follows from the code: the old handler dropped all three fields, so each PATCH returned 200 with no membership change.
• Parity claim is accurate, but incomplete. Tauri update_node does send three empty lists (packages/desktop-app/app-lib/src/commands/nodes.rs:721-723), NodeUpdate has no collection fields (packages/nodespace-types/src/node.rs:197-208), and the CLI sends all three (packages/cli/src/commands/node.rs:302-304). What the description leaves out is in the second finding below.

Node-type sequence: not applicable. No type, field, relationship or structural rule changes; member_of is already declared.

Requirements (issue #3455)

Criterion	Status
Add by path, add by id and remove by id take effect through the proxy (tested against the proxy)	✅ Tests 1 and 2; ran green
A path add and an id add on one request is rejected, as on the Tauri path	✅ 400, nothing written. "As on the Tauri path" is vacuous (Tauri cannot send these fields); the rule lives in the shared ops layer, so every client gets it
bun run test:changed passes; bun run quality:fix clean	✅ as reported by the author. Not re-run here: quality:fix rewrites files, and test:changed does not include the e2e tier, which I ran separately

Findings

Critical Issues

None.

Suggested Improvements

🟡 Important — the create route has the same bug
📁 packages/dev-tools/src/dev-proxy.ts:410 (and the comment at :395)

POST /api/nodes still does request.collection = body.collection. CreateNodeRequest has no collection field; it has repeated string collections = 5 and repeated string collection_ids = 11 (node_service.proto:228, :250). A collection passed on create is silently dropped, exactly as it was on update.

Recommendation: fix it here with the same validated loop over collections and collectionIds, and add a create case to the new e2e file. Principle: fix a defect class, not one instance; leaving the sibling means the proxy still disagrees with the proto one route above the fix.

🟡 Important — browser mode still cannot change membership, and the new fields have no production caller
📁 packages/desktop-app/src/lib/services/collection-service.ts:266-277

The description says the app changes membership "through the separate collection commands". That is true in Tauri only. In browser mode getCollectionService() returns HttpCollectionService, whose addNodeToCollection, addNodeToCollectionPath and removeNodeFromCollection are empty bodies marked "Not implemented in dev-proxy yet". So the user-visible problem in the issue ("in browser mode adding or removing a collection ... is silently dropped") is still true after this PR; only the proxy's HTTP contract is fixed, and the test is its only caller.

Recommendation, in order of preference:

1. Implement the two id-based methods on HttpCollectionService with the PATCH this PR just fixed (addToCollectionIds: [collectionId] / removeFromCollectionIds: [collectionId]). That is a few lines and gives the fields a real consumer. addNodeToCollectionPath must return the leaf collection id, which the PATCH response does not identify, so it can stay as is.
2. If that is out of scope, say so plainly in the PR description so the parity section does not read as "both clients behave the same".

Principle: correctness against the stated problem, and YAGNI — an endpoint capability nothing but a test calls.

🟢 Suggestion — pin why the "both" request is rejected
📁 packages/desktop-app/src/tests/e2e/node-update-collections.e2e.ts:101

The test asserts only status === 400. Assert code === 'INVALID_ARGUMENT' and that the message is the daemon's "paths or collection IDs, not both", as the fourth test does for its code. Otherwise any 400 (for example a future proxy-side validation) keeps the test green without the exclusivity rule being exercised.

🟢 Suggestion — cover a collection change together with a field change
📁 packages/desktop-app/src/tests/e2e/node-update-collections.e2e.ts

Every case sends a collection-only PATCH, which takes the update.is_empty() branch in node_ops::update_node and skips the core update. The issue is about a collection change "during an update"; one case sending content plus addToCollections and asserting both would cover the other branch.

🟢 Suggestion — stale usage examples now contradict the working contract
📁 packages/desktop-app/src/lib/types/node.ts:218, :221, :283

These doc comments show updateNode(id, version, { add_to_collections: [...] }). That works on neither transport: UpdateNodeInput has no such field, Tauri's NodeUpdate ignores it, and the proxy reads camelCase. Point the examples at collectionService instead (comments should not describe an API that does not exist).

Nitpicks

• Nit: node-update-collections.e2e.ts:61, :73, :91 — version: 1 has no effect on a collection-only PATCH (the version is never checked on that branch). Drop it, as the id-based calls already do, so it does not read as load-bearing.
• Nit: node-update-collections.e2e.ts:73, :91 — the seed PATCH's response is not asserted, so a failed seed surfaces later as memberIds(undefined). An expect(...status).toBe(200) gives a clearer failure.
• Nit: daemon-harness.ts:339 — sseUrl and the two new HttpAdapter(...) calls (:260, :297) build the same URL by hand; they could use baseUrl.

Security, performance, documentation

• Security: improved. The handler now refuses a non-array instead of forwarding an arbitrary value to the gRPC encoder. Dev-only local tool; no new exposure.
• Performance: no impact.
• Documentation: no nodespace-docs page references the old field names; nothing to update beyond the node.ts comments above.

Verdict

APPROVE — a net improvement that is correct and tested. The two Important items are recommended for this PR but do not block it.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review feedback addressed (commit 08085b3)

Addressed

• 🟡 Create route had the same bug. POST /api/nodes forwarded collection, which CreateNodeRequest does not have. It now forwards the repeated collections and collectionIds. Both routes share one helper, forwardStringLists, which refuses a value that is not an array of strings.
• 🟡 Browser mode could not change membership. HttpCollectionService.addNodeToCollection and removeNodeFromCollection now send the node update with addToCollectionIds / removeFromCollectionIds instead of doing nothing. addNodeToCollectionPath is unchanged: it must return the leaf collection id, which the update response does not identify.
• 🟢 Pin why the "both" request is rejected. The test now asserts INVALID_ARGUMENT and the daemon's "not both" message, on update and on create.
• 🟢 Collection change together with a field change. New case sends content with addToCollectionIds and checks both took effect.
• 🟢 Stale usage examples in node.ts now point at collectionService.
• Nits: dropped the version: 1 that had no effect; the seed request is asserted (in a seedCollection helper); sseUrl uses baseUrl.

Skipped

• Nit: the two new HttpAdapter(...) calls in the harness still build the URL from the spawned port. They run in static methods before an instance exists, so there is no baseUrl to use.

Tests

• The e2e file is renamed node-collection-fields.e2e.ts because it now covers create as well: 9 of 9 pass against a real proxy and daemon. watch-nodes.e2e.ts (the sseUrl user) passes.
• collection-service.test.ts: 36 pass, including three new cases for the two membership methods.
• bun run test:changed passed; bun run quality:fix made no changes.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: this commit adds behaviour the first review did not see: a second proxy route changed, and two service methods went from no-ops to real requests.

[malibio]

Pragmatic Code Review — PR #3456 (re-review)

Review Type: Re-Review of commit 08085b34 (diff 2d303fcc..08085b34)

Verdict: APPROVE. Both Important findings are fixed in the code, the fixes are tested against a real proxy and daemon, and the new commit introduces no Critical or Important problem.

Previous Review Summary

	Count
Previous recommendations	8 (2 Important, 3 Suggestions, 3 Nits)
Addressed	7
Partly addressed, remainder reasonably skipped	1
Not addressed	0
New issues found	0 Critical, 0 Important, 2 Suggestions, 2 Nits

Checked against the code, not the author's summary:

#	Recommendation	Status	Evidence
1	🟡 Create route had the same bug	✅ Addressed	packages/dev-tools/src/dev-proxy.ts:429 forwards collections and collectionIds, which are CreateNodeRequest fields 5 and 11. The daemon passes both to node_ops::create_node, which rejects a request carrying both before it writes (packages/core/src/ops/node_ops.rs:183). No body.collection reference remains, and no adapter ever sent that key.
2	🟡 Browser mode could not change membership	✅ Addressed (option 1)	HttpCollectionService.addNodeToCollection and removeNodeFromCollection send the PATCH (collection-service.ts:266-299). The field names match what the proxy reads. addNodeToCollectionPath stays a stub, as the first review allowed.
3	🟢 Pin why the "both" request is rejected	✅ Addressed	expectPathsAndIdsRefused asserts 400, INVALID_ARGUMENT and "not both", on update and on create.
4	🟢 Collection change together with a field change	✅ Addressed	New case sends content with addToCollectionIds and asserts content, version 2 and membership. It takes the non-empty-update branch of update_node.
5	🟢 Stale usage examples in node.ts	✅ Addressed	Lines 193, 218, 221 and 283 point at collectionService.
6	Nit: version: 1 with no effect	✅ Addressed	The only one left (node-collection-fields.e2e.ts:126) is on the content update, where the version is checked.
7	Nit: seed request not asserted	✅ Addressed	seedCollection asserts the 200 and that the collection id resolved.
8	Nit: harness URLs built by hand	◐ Partly	sseUrl uses baseUrl. The two new HttpAdapter(...) calls were skipped.

Was the skip reasonable? Yes. Both calls are in the static start() and startDeferred() (daemon-harness.ts:260, :297), before the instance that owns baseUrl exists. Sharing the URL would need a new module-level helper for one string template, which is not worth it.

What I verified

• Ran the e2e file against the worktree's target/debug/nodespaced and the proxy from source: 9 of 9 passed in 2.7s.
• Ran collection-service.test.ts: 36 of 36 passed.
• ESLint on the five changed frontend files: clean.
• The two transports do the same thing. Tauri calls the AddNodeToCollection / RemoveNodeFromCollection RPCs; browser mode now calls UpdateNode with only an id list. Both end in the same CollectionService::add_to_collection / remove_from_collection (collection_ops.rs:226, :254; node_ops.rs:365-377). A collection-only update skips the node write, so the version does not change on either path. The one difference is that the update path looks the node up first and returns NOT_FOUND for a missing node.
• Going from no-op to a real request is safe for the callers. All three call sites (collection-node-viewer.svelte:148, :233; collection-sub-panel.svelte:110; and createNodeInCollection, called inside a try) catch and log, so a rejected request cannot break the view.
• No leftovers. No reference to the renamed test file, to body.collection, or to the old singular addToCollection / removeFromCollection remains in the repo.

Not verified: I did not drive the collection UI in a browser. The service methods are covered by a unit test with a mocked fetch, and the request they send is covered by the e2e test, but no test runs the two together.

Node-type sequence: not applicable. No type, field, relationship or structural rule changes.

Requirements (issue #3455)

Criterion	Status
An update through the dev-proxy that adds a node to a collection (by path and by id) and removes one by id takes effect (test against the proxy)	✅ Update cases 1 and 2; ran green
Sending both a path and an id add on one request is rejected, as on the Tauri path	✅ 400 INVALID_ARGUMENT with the daemon's "not both" message, nothing written; now also asserted on create
bun run test:changed passes; bun run quality:fix clean	✅ as reported by the author; not re-run here (quality:fix rewrites files). I ran the e2e file, the unit file and ESLint on the changed files
Issue overview: browser-mode membership changes are silently dropped	✅ Now true for add and remove by id, which is everything the UI calls

Findings

Critical Issues

None.

Important

None.

Suggested Improvements

🟢 Suggestion — the PR title and description describe the first commit only
📁 PR #3456 title and body

The title says "node update" only. The body names the deleted node-update-collections.e2e.ts, counts four tests, and its parity section says nothing in the frontend sends these fields, which HttpCollectionService now does. Update both before merge so the squash commit describes what lands: the create route, the two service methods and the nine-case test file. Principle: the commit message is the durable record of the change.

🟢 Suggestion — patchMembership drops the proxy's reason
📁 packages/desktop-app/src/lib/services/collection-service.ts:296-298

The error is built from response.statusText, so a refusal reads "Failed to add node to collection: Not Found" and the proxy's { code, message } body is discarded. This matches the two existing methods in the class, so it is consistent and not a regression. These are the first writes in the class, though, and a write's refusal reason is what a developer needs from the log. Optional: read message from the JSON body and fall back to statusText. Principle: error messages should aid debugging.

Nitpicks

• Nit: packages/dev-tools/src/dev-proxy.ts:412-414 — the comment's list is now (parentId, lifecycleStatus), but it still ends "(empty parent lookup, empty path)". "Empty path" referred to the removed collection string.
• Nit: packages/desktop-app/src/lib/types/node.ts:218, :283 — the examples recommend addNodeToCollectionPath, which in browser mode still returns '' and does nothing. Nothing in production calls it, so this is harmless today; the stub's comment is the place a reader finds that out.

Security, performance, documentation

• Security: unchanged from the first review. The create route now refuses a non-array for its two list fields, as the update route does. nodeId is URL-encoded in the new request. Dev-only local tool.
• Performance: one request per membership change; no impact.
• Architecture: forwardStringLists removes the duplication the second route would otherwise have added, and the paths-or-ids rule still lives only in the ops layer. No Pro-boundary or node-type concerns.

Verdict

APPROVE — every prior recommendation is resolved or reasonably skipped, and the new code is correct and tested. The two suggestions are optional; the PR description update is the one worth doing before merge.

[malibio]

Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review feedback addressed

Addressed

• 🟢 PR title and description now describe both commits: the create route, the two service methods and the nine-case test file.
• Nit: the create handler's comment no longer mentions "empty path", which referred to the removed collection string.

Skipped

• 🟢 patchMembership drops the proxy's reason. It builds its error from statusText, as the other methods in the class do. Reading the JSON body is worth doing for the whole class at once rather than for two of its methods; the three callers only log the error.
• Nit: the node.ts examples recommend addNodeToCollectionPath, which is still a stub in browser mode. The example is correct for the Tauri app, and the stub's own comment says it is not implemented.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the only code change since the re-review is one comment line. The rest is the PR title and description.