Describe the bug
Rejecting an invalid customMetadata value passed to TreeViewAlpha.runTransaction / runTransactionAsync puts the entire checkout into a broken state, rather than being a recoverable usage error. Every subsequent operation on the view then fails with Invalid use of ... after it was put into an invalid state by another error.
This is unfortunate because a malformed customMetadata is documented user input error — the same class of mistake as passing a bad value to any other API — and the value is validated before any state is mutated, so nothing is actually left inconsistent.
To Reproduce
Steps to reproduce the behavior:
- Create a
TreeViewAlpha.
- Call
view.runTransaction(() => { ... }, { customMetadata: cyclic }) where cyclic is an object containing a cycle (or any non-JSON-serializable value).
- Observe the expected
UsageError: Transaction "customMetadata" must be JSON-serializable: Converting circular structure to JSON.
- Perform any further valid edit on
view — e.g. view.runTransaction(() => view.root.insertAtEnd("x")).
- See
UsageError: Invalid use of SchematizingSimpleTreeView after it was put into an invalid state by another error.
Expected behavior
The invalid value is rejected with a UsageError and the view remains fully usable, the same way a rejected direct edit throws recoverably.
Cause and suggested fix
snapshotCustomMetadata runs inside TreeCheckout.mountTransaction, which is reached through @breakingMethod-decorated entry points. Any throw inside a breaking method marks the shared Breakable as broken.
Fixing it at the TreeCheckout layer alone is not sufficient: SchematizingSimpleTreeView is decorated with @breakingClass, so all of its methods break on throw, and it shares the checkout's Breakable (this.breaker = checkout.breaker). The view is what applications actually call.
The likely fix is to have the view's two runTransaction methods opt out of automatic breaking with @throwIfBroken (which breakingClass deliberately skips) and validate the metadata before delegating to the checkout. That appears safe for the transaction path itself, since the checkout's inner method remains @breakingMethod and still breaks on genuine failures — but it also means calling runTransaction on a disposed view would no longer poison the shared breaker, so the change needs its own review and test pass.
This was deliberately deferred out of PR #28064 to keep that change scoped to the custom commit metadata feature; the limitation is noted in a comment on TreeCheckout.mountTransaction.
Describe the bug
Rejecting an invalid
customMetadatavalue passed toTreeViewAlpha.runTransaction/runTransactionAsyncputs the entire checkout into a broken state, rather than being a recoverable usage error. Every subsequent operation on the view then fails withInvalid use of ... after it was put into an invalid state by another error.This is unfortunate because a malformed
customMetadatais documented user input error — the same class of mistake as passing a bad value to any other API — and the value is validated before any state is mutated, so nothing is actually left inconsistent.To Reproduce
Steps to reproduce the behavior:
TreeViewAlpha.view.runTransaction(() => { ... }, { customMetadata: cyclic })wherecyclicis an object containing a cycle (or any non-JSON-serializable value).UsageError: Transaction "customMetadata" must be JSON-serializable: Converting circular structure to JSON.view— e.g.view.runTransaction(() => view.root.insertAtEnd("x")).UsageError: Invalid use of SchematizingSimpleTreeView after it was put into an invalid state by another error.Expected behavior
The invalid value is rejected with a
UsageErrorand the view remains fully usable, the same way a rejected direct edit throws recoverably.Cause and suggested fix
snapshotCustomMetadataruns insideTreeCheckout.mountTransaction, which is reached through@breakingMethod-decorated entry points. Any throw inside a breaking method marks the sharedBreakableas broken.Fixing it at the
TreeCheckoutlayer alone is not sufficient:SchematizingSimpleTreeViewis decorated with@breakingClass, so all of its methods break on throw, and it shares the checkout'sBreakable(this.breaker = checkout.breaker). The view is what applications actually call.The likely fix is to have the view's two
runTransactionmethods opt out of automatic breaking with@throwIfBroken(whichbreakingClassdeliberately skips) and validate the metadata before delegating to the checkout. That appears safe for the transaction path itself, since the checkout's inner method remains@breakingMethodand still breaks on genuine failures — but it also means callingrunTransactionon a disposed view would no longer poison the shared breaker, so the change needs its own review and test pass.This was deliberately deferred out of PR #28064 to keep that change scoped to the custom commit metadata feature; the limitation is noted in a comment on
TreeCheckout.mountTransaction.