CODAP-1485: support incremental case append for hierarchical collections

[kswenson]

Summary

DataSet has an optimized incremental-append path for new items, but it only fully applied to flat datasets. For hierarchical ones — including the Sampler plugin, which builds three collections (experiments → samples → items) — every append did an O(total cases) rebuild per child collection, making streamed item creation O(N²).

The append fast path in Collection.completeCaseGroups was gated on !parentCases, which is never true for a collection that has a parent. The earlier stages were already additive and correct for hierarchies; only the completion stage regressed.

Streamed item creation through the plugin path is 7.4× faster at the tail — 7.50 ms per request down to 1.01 ms at 4000 items.

Results

Appending 400 samples of 10 items to a three-collection dataset — ms per append, by quartile:

	Q1	Q2	Q3	Q4
flat, before	0.52	0.84	1.17	1.50
flat, after	0.40	0.53	0.71	0.86
hierarchical, before	1.12	2.70	4.34	6.04
hierarchical, after	0.49	0.65	0.78	0.95

End-to-end through the plugin path (createItemsInSegments), ms per request:

	Q1	Q4
before	1.55	7.50
after	0.61	1.01

7.4× faster at the tail. This reduces the O(N²) cost of streamed creation rather than removing it — each append is still O(N), since the cached case arrays are rebuilt by spreading and completeCaseGroups rehashes every case id on both branches. Making the path genuinely O(new cases) is CODAP-1487.

What changed

The append path itself — a child collection completes additively when the new cases provably sort to the end of the grouped order; inserts, additions under an older parent, and regrouping still rebuild. Streaming into an existing sample of an existing experiment no longer re-sorts either parent collection, which was O(accumulated parent cases) on every item.

The lookup maps — caseInfoMap was re-registered wholesale and itemIdChildCaseMap cleared and rebuilt on every append; both are now incremental.

What a create request reports — createItemsInSegments no longer diffs every collection's case ids before and after. The add reports the case ids it minted, read through a consume-once accessor. The request brings grouping up to date first so the additive pass can run, since deleteItem, deleteCaseBy and itemSearch.delete all removeCases without revalidating, so delete-then-create arrives with grouping invalid.

Correctness fixes found along the way — an appended item that un-hides a set-aside case is restored correctly (main dropped it from both the middle and childmost collections); setLength growing in place no longer loses the MobX notification that reassignment provided, and isNumeric reads changeCount like length and type; getAfterPosition no longer treats item index 0 as "no position".

Review

Approved by @dougmartin after two rounds of requested changes, alongside three multi-agent review passes. Between them they found roughly twenty defects, all addressed. Two behaviours were kept deliberately and are called out in the thread: a set-aside case made visible again is reported as created, matching main; and the five per-index attribute accessors stay non-reactive, since making them read changeCount would re-render the case table on every value write.

Testing

Twenty-three tests across data-set-hierarchical-append.test.ts, data-set.test.ts and item-handler.test.ts. Each assertion is mutation-verified — reverting the production change it covers fails at least one test. Several exist because mutation showed an earlier version constrained nothing, and the reporting tests derive ground truth independently (a case belongs to a request exactly when every item in it came from that request) rather than from the dataset's own case ids.

Follow-ups

• CODAP-1486 — items appended while a value-only invalidation is pending are never grouped. Pre-existing; reproduces on main.
• CODAP-1487 — the remaining O(total cases) work per append: five full-length passes in completeCaseGroups, and caseIdToIndexMap encoding a hidden case two different ways.

Fixes CODAP-1485

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 98.63014% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.86%. Comparing base (f3d4193) to head (5644369).

Files with missing lines	Patch %	Lines
v3/src/models/data/collection.ts	97.26%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2687       +/-   ##
===========================================
+ Coverage   73.02%   87.86%   +14.83%     
===========================================
  Files         818      818               
  Lines       47121    47223      +102     
  Branches    12034    12069       +35     
===========================================
+ Hits        34412    41493     +7081     
+ Misses      12694     5714     -6980     
- Partials       15       16        +1     

Flag	Coverage Δ
cypress	69.72% <79.02%> (+31.68%)	⬆️
jest	63.02% <98.63%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cypress]

codap-v3    Run #12168

Run Properties:   Passed #12168  •  496cfbec4b: null

Project	codap-v3
Branch Review	main
Run status	Passed #12168
Run duration	08m 16s
Commit	496cfbec4b: null
Committer	null
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	82
Skipped	0
Passing	384
View all changes introduced in this branch ↗︎

[Copilot]

Pull request overview

Improves DataSet/Collection incremental append performance for hierarchical collections (e.g., Sampler-style experiment → sample → item), avoiding per-append O(total cases) rebuild work and bringing hierarchical streaming performance close to flat-dataset append costs.

Changes:

• Extend Collection.completeCaseGroups()’s incremental append fast path to child collections when appended cases provably sort to the end; fall back to rebuild when ordering/hidden-case conditions require it.
• Make DataSet.validateCasesForNewItems() update caseInfoMap and itemIdChildCaseMap incrementally rather than clearing/rebuilding them each append.
• Reduce append overhead in related paths (createItemsInSegments() case discovery and Attribute.setLength() growth strategy) and add focused regression tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
v3/src/models/data/data-set.ts	Incrementally registers only newly created cases and maps only newly appended items to child cases during append validation.
v3/src/models/data/collection.ts	Enables additive completion for hierarchical appends when safe (ordering + non-hidden), with rebuild fallback for hidden/ordering-unsafe appends; simplifies updateCaseGroups() signature.
v3/src/models/data/attribute.ts	Changes setLength() to grow value arrays in place to avoid per-append reallocation.
v3/src/data-interactive/handlers/item-handler.ts	Derives newly created case IDs from added items rather than scanning all case IDs, keeping work proportional to appended item count.
v3/src/models/data/data-set-hierarchical-append.test.ts	Adds hierarchical append tests covering non-regroup reads, incremental map updates, indexing correctness, hidden/empty-case behavior, and ordering across parents.
v3/src/data-interactive/handlers/item-handler.test.ts	Adds tests for hidden-case exclusion and reporting new case IDs in collection order.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

v3/src/models/data/collection.ts:595

• completeCaseGroups() treats an additive invalidation with newCaseIds=[] the same as “no append info” and falls through to the REBUILD path. That means parent collections (where self.child is set) will do an O(total cases) rebuild even when appending items only extended existing cases (i.e. no new cases were created), which undercuts the incremental-append optimization.

Since parent collections’ cached arrays (_caseGroups/_cases/_nonEmptyCases) are still valid when no new cases were added, this branch can skip the rebuild and just bump _groupingChangeVersion so observers notice any in-place mutations (e.g. updated childItemIds).

      completeCaseGroups(parentCases: Maybe<CaseInfo[]>) {
        const newCaseIds = _pendingNewCaseIds
        _pendingNewCaseIds = undefined

        const newGroups = !_needsFullRebuild && newCaseIds?.length
          ? newCaseIds
              .map(caseId => self.getCaseGroup(caseId))
              .filter((group): group is CaseInfo => !!group)
          : undefined

[kswenson]

Copilot's latest pass reported no new comments but included one suppressed comment: completeCaseGroups treats an additive invalidation with newCaseIds = [] the same as "no append info" and falls through to REBUILD, so parent collections rebuild even when an append created no case in them.

That was worth acting on — an independent multi-agent review flagged the same spot — and it's now fixed in dcdd525b1. Measured on a three-collection dataset with items streaming into the newest parent, which is the shape a long Sampler session produces:

parent cases	before	after
100	0.52 ms	0.44 ms
400	0.98 ms	0.74 ms
1600	3.20 ms	1.96 ms

The suggestion's scoping to parent collections (self.child set) was the right call and I kept it. The childmost collection can't take the shortcut: emptiness there is derived from item values, and re-sending an item id writes new values into a case that already exists — adding no case while still changing which cases are empty. There's a test for exactly that now.

The residual growth is case-id rehashing, which happens on both branches and is tracked separately as CODAP-1487.

[dougmartin]

Nice work on the performance win, and the mutation-verified test suite is a real strength. Two correctness regressions need fixing before merge: a genuinely-created case can go unreported to plugins, and setLength growing in place silently stops isNumeric from ever invalidating.

Changes requested

• v3/src/data-interactive/handlers/item-handler.ts (lines 51-55, 70, 79): re-sending an item id the data context already holds suppresses reporting of cases the request genuinely created. resentItemIds strips every already-held id from creatingItemIds, so a case whose first item is a re-sent id is treated as pre-existing. But re-sending an id with new grouping values moves that item to a new group and forms brand-new parent cases. Reproduced against setupTestDataset(): createItemsInSegments(dataset, [[{ id: toV2Id(existingItemId), values: { a1: "brandNewA1", a2: "brandNewA2", a3: 42 } }]]) creates one new case in collections[0] and one in collections[1], yet results[0].caseIDs comes back []. On main the same call reports both. The plugin gets no caseIDs and no createCases notification for cases that really exist, so plugin-side state silently diverges from CODAP, and that is precisely the Collaborative-plugin scenario resentItemIds was added to serve. Fix: decide newness from whether the case id existed before the add rather than from the item. Capture the known case ids before addCases (or better, have validateCasesForNewItems expose the case ids it registered, since data-set.ts:807 already iterates exactly that set) and replace the creatingItemIds.has(firstItemId) test with !knownCaseIds.has(caseId); that also makes resentItemIds unnecessary. Note the new test "does not report pre-existing cases when an item id is re-sent" re-sends values that form no new group at any level, so it passes on main too and does not constrain the new code, which is why this slipped through; please add a case that re-sends an existing id with grouping values that are new at a parent collection.

• v3/src/models/data/attribute.ts (lines 514-523, with the stale view at lines 151-154): growing the volatile arrays in place removes the only thing that invalidated isNumeric. get isNumeric reads self.numValues contents and never reads self.changeCount, and volatile array contents are not observable (the file documents this itself at lines 298-300), so the old self.numValues = self.numValues.concat(...) reassignment was what invalidated the computed, with MobX deferring the reaction to the end of the enclosing action, after addCases had written values. incChangeCount() does not help a view that never reads changeCount. Reproduced on a fresh dataset with a reaction(() => attr.isNumeric, ...), then data.addCases([{ __id__: "i1", nId: 5 }]); data.validateCases(): the PR branch observes [false] and attr.isNumeric stays false permanently; temporarily restoring main's setLength in the same tree observes [false, true]. So an attribute that becomes numeric through streamed or appended items keeps reporting isNumeric === false to every active observer: case-card-model.ts:92 branches on it for min/max vs. categorical summaries, and cell-text-editor.tsx:38 uses it to pick editor behavior. Fix: make isNumeric follow the same convention as get length and get type and read self.changeCount before touching numValues. Worth a reaction-based test too; the new setLength test only covers a.length, which already read changeCount and so could not have caught this.

Non-blocking

• v3/src/models/data/collection.ts (lines 597-603 and 669-705), with the test asserting the new behavior at v3/src/models/data/data-set-hierarchical-append.test.ts:78: when mustRecompute is false, _caseGroups/_cases/_nonEmptyCases keep their previous array identities while the CaseInfo objects inside them are mutated (addChildCase grows childCaseIds/childItemIds). _groupingChangeVersion still bumps so get caseGroups re-evaluates, but MobX propagates nothing downstream because the value is === its previous one. That contradicts the REBUILD path, which deliberately documents identity change as the signal at lines 694-697 ("A shallow copy preserves reference-identity semantics... so identity-comparing reactions on nonEmptyCases still fire"). Observers deriving per-parent child counts or child lists from collection.caseGroups (attribute-formula-adapter.getCaseChildrenCountMap, data-set-metadata.isFirstCaseOfAncestor, the case-table spacer geometry) won't be re-notified for a parent collection that gained no case of its own, and validateCasesForNewItems never bumps _caseValidationVersion, so the version counters are the only append signal. Either refresh the array identities when a descendant gained cases (_caseGroups = [..._caseGroups] and likewise, still far cheaper than the re-sort it replaces), or state explicitly in the comment at lines 597-602 that consumers must depend on _groupingChangeVersion and drop the identity-preserving copy at line 699, since the two paths currently contradict each other.

• v3/src/models/data/collection.ts (lines 385-394) and v3/src/models/data/data-set.ts (lines 785-791): an un-hidden case in a middle collection still ends up ordered differently from a full regroup. The new else-branch registers it via addChildCase, which appends to childCaseIds, whereas a full regroup walks parentChildIdMap in item order and lands it in its item-order position among its siblings. Two collections (g then m), items i1{g:1,m:1} and i2{g:1,m:2}, hide i1, append i3{g:1,m:1}: incremental gives the middle collection caseIds = [m2case, m1case], a full regroup gives [m1case, m2case], with symParent/caseIdToIndexMap differing correspondingly; the PR's own groupingOf(data) === regroupFromScratch(data) assertion fails for this shape. Not a regression (main drops the un-hidden case entirely), but the commit message claims both paths now match a full regroup, and the new tests at lines 270-325 only cover shapes where the two orders coincide. The visible symptom is case-table rows re-ordering spontaneously on a later reload, hierarchy change, or undo. Given how rare the shape is (set-aside cases plus streamed append), the simplest fix is to prefer correctness over speed: when any collection reports unhiddenCaseIds, fall back to a whole-dataset self.invalidateCases() rather than the per-collection collection.invalidateCaseGroups().

• v3/src/models/data/collection.ts (lines 670-682, 707-708): appends are still O(N) each, so streamed creation remains O(N²) asymptotically. _caseGroups, _cases and _nonEmptyCases are each rebuilt by spreading the full existing array, and hashStringSet/hashOrderedStringSet run unconditionally over every case id, five O(N) passes per append in the childmost collection. Measured on the three-collection Sampler shape on this branch: 0.82 ms/append at 1k items, 1.41 at 2k, 3.18 at 5k, 6.43 at 8k, still linear in N per append (main couldn't finish 8k within a 2-minute budget, so this is a large constant-factor win). No change needed here if CODAP-1487 covers it, but the PR description frames the O(N²) streamed-creation cost as fixed when it's reduced rather than removed, and a long Sampler session can accumulate well past 8k items, so it's worth adjusting the wording so it isn't lost.

• v3/src/models/data/collection.ts (lines 307-309): "Insertions currently take the rebuild path. Handling them incrementally is the opportunity still open here, and it would want the insert position rather than a flag..." describes work that doesn't exist rather than the code that does, so it will go stale independently of the code and duplicates what the tracking ticket should hold. Suggest deleting those three sentences and moving the insert-position note to CODAP-1487; the first sentence about itemIds semantics and the "appending items does not imply appending cases" paragraph document real invariants and are worth keeping.

• v3/src/models/data/collection.ts (line 394) and v3/src/models/data/data-set.ts (lines 785-791): a case hidden and then un-hidden within one append pass forces an unnecessary full rebuild. unhiddenCaseIds.push(caseId) runs in both arms of the if (parentChildInfo) branch, including when the case was created earlier in the same pass and so is already in newCaseIds with its parent already informed. A coalesced batch containing a hidden item followed by a visible item with the same group key therefore drops back to O(cases) for that collection. Moving the push into the else arm (the branch that mints the parentChildIdMap entry) confines it to the case it's meant for. Moot if the whole-dataset-regroup fix above is taken instead.

• v3/src/models/data/collection.ts (lines 389-392): the un-hide branch can mint a phantom parent case id. self.parent?.groupKeyCaseId(self.parentGroupKey(itemId)) creates and stores a new case id when the group key isn't already mapped (see groupKeyCaseId, lines 214-222), so if the parent group key is ever unknown here the code registers a parentCaseId with no case group; addChildCase then logs "missing parent case" and silently drops the child, while the minted id pollutes groupKeyCaseIds, which is serialized. A non-creating lookup would be safer: read self.parent?.groupKeyCaseIds.get(...) or add a peekGroupKeyCaseId view, and skip registration when it's absent.

One note on CI: the only non-green check is cypress/flake (1 flaky test, 0 failures), which doesn't look related to this change.

[kswenson]

Thanks — this was a genuinely valuable review. I reproduced all four substantive claims before acting on any of them, and every one held up.

Both blocking items are fixed.

The new-case reporting was wrong in exactly the way you describe: I confirmed 2 cases created and 0 reported. Taking your preferred fix — addCases now records the case ids it minted and callers read that, so newness is decided by case id rather than by item, and resentItemIds is gone. Your diagnosis of why it slipped through was also right: my test re-sent values forming no new group, so it passed on main too. Added the parent-collection case you asked for.

isNumeric likewise: reaction observed [false] while the value was true. It now reads changeCount like length and type. I audited the rest of the file rather than patching the one view — the cached scanners (isInferredNumericType and friends) are fine because incChangeCount invalidates them explicitly, and the per-index accessors never reacted to in-place writes, so that predates this branch. Added the reaction-based test.

Non-blocking, also taken:

The un-hide ordering was real — I reproduced [m2, m1] vs [m1, m2], so my commit message overstated it. Took your suggested whole-dataset regroup, which also made two other items moot: the unhiddenCaseIds.push now only fires for the case it's meant for, and the registration that called groupKeyCaseId — which does mint and store an id, as you noted — is gone entirely.

On array identity: you're right that the two paths contradicted each other, and my test asserted the wrong thing. Collections now hand out fresh arrays when they reuse their contents. Cost is ~8% of that optimization's win (1.96 → 2.12 ms/append at 1600 parent cases, against 3.20 before it), which seems a fair price for not silently dropping notifications.

Dropped the insert-position comment; that note is on CODAP-1487 now. And I've reworded the description so it says reduced-not-removed, with your per-N numbers.

Every fix is mutation-verified — each one, reverted, fails at least one test.

[dougmartin]

Thanks for the thorough follow-up. All eight items from the last round are addressed, and I confirmed each one: resentItemIds/creatingItemIds are gone and newness is decided by case id, isNumeric reads changeCount with a reaction-based test, the UNCHANGED branch hands out fresh array identities, the un-hide case falls back to a whole-dataset regroup, unhiddenCaseIds.push is confined to the else arm, the groupKeyCaseId un-hide path is gone entirely, the speculative insert-position comment is deleted, and the description now says reduced rather than removed.

One blocking item remains, and it is the same failure mode as last round's blocking report arriving through a different door: when the fast path cannot say what it created, the handler reports that nothing was created. One of the two triggers is introduced by the fix for the un-hide note.

Changes requested

• v3/src/data-interactive/handlers/item-handler.ts (lines 57-70), with the two paths that produce it at v3/src/models/data/data-set.ts (line 789) and (lines 795-804): const created = dataContext.newCaseIdsForLastAppend ?? {} treats "the append could not say what it created" as "the append created nothing", so the request returns caseIDs: [] and emits no createCases notification while cases really were created. Neither fallback populates the field: validateCasesForNewItems returns before writing it, and the full rebuild inside validateCases() never writes it at all. Reproduced against setupTestDataset(), both failing on this branch and correct on main. (a) Dataset invalid when the create request arrives: dataset.removeCases([dataset.items[0].__id__]) then createItemsInSegments(dataset, [[{ a1: "qq", a2: "qq", a3: 300 }]]) gives 3 cases created, 0 reported. This is reachable from ordinary plugin traffic because deleteItem and deleteCaseBy in handler-functions.ts (lines 20-22, 33-36) and itemSearch.delete in item-search-handler.ts (lines 16-19) all call removeCases inside applyModelChange with no following validateCases, so isValidCases stays false until something else validates. A plugin that deletes and then creates hits it. (b) The new un-hide fallback: hide the three a1: "b" items, then createItemsInSegments(dataset, [[{ a1: "b", a2: "y", a3: 100 }, { a1: "zz", a2: "zz", a3: 200 }]]) gives 6 cases created, 0 reported, including the whole new a1: "zz" branch that has nothing to do with the un-hidden case. The consequence is the one from last round: plugin-side state silently diverges from CODAP because CODAP reports no creation for cases that exist, and this reaches the Collaborative and Sampler flows (set-aside items, delete-then-repopulate). Fix: make the fallback paths report as well, rather than letting undefined mean "nothing". For (a) nothing has been grouped yet at line 789, so the collections' current caseIds are a valid baseline: snapshot them there and diff after the caller's validateCases(). For (b) the baseline is still recoverable even though the collections ahead of the bail were already mutated: for each collection processed so far it is current caseIds minus newCaseIds minus unhiddenCaseIds, and for the rest it is current caseIds. Both are O(total cases), but only on paths that are already doing a full regroup, so the streaming fast path keeps its win. Please add a test for each reproduction; the suite has neither today, which is why this passed.

Non-blocking

• v3/src/models/data/data-set.ts (lines 795-804): the un-hide fallback returns after self.invalidateCases() without revalidating, so addCases can now hand back a dataset that is invalid and internally inconsistent. The collections processed before the bail already pushed to self.caseIds and caseIdToIndexMap (collection.ts lines 346-348, 376-379), but no collection ran completeCaseGroups, so the cached _caseGroups/_cases/_nonEmptyCases still describe the pre-append state and no version counter moved. Measured on two collections with the b branch set aside: before addCases, isValidCases true with caseIds.length === 1 and cases.length === 1; after, isValidCases false with caseIds.length === 2 and cases.length === 1. Grouping is correct again after the next validateCases(), and every in-repo caller does call it, so I am not blocking on this; the insert path already leaves the dataset invalid on main in a comparable way. Still, an append flipping a valid dataset to invalid is new, and the mixed state is exactly what commit #606bb55b3 set out to eliminate. Calling self.validateCases() right after self.invalidateCases() in the fallback would restore addCases' old contract at no cost on a path that is rare by construction, and it would give the fix above a validated dataset to derive the created ids from.

• v3/src/models/data/data-set.ts (line 192, with the write at line 835 and the only reset at line 1314): newCaseIdsForLastAppend is documented as "the cases created by the most recent addCases", but nothing except the next addCases clears it, so after removeCases, setCaseValues, a full regroup or an undo it still advertises the previous append's ids, and those ids can name cases that no longer exist. Today's only consumer reads it in the same statement sequence and filters by getCaseIndex(...) ?? -1, so nothing is broken now, but it is a public volatile prop whose stated contract does not hold. Either narrow it so it cannot be misused (return the record from addCases, or expose a consume-and-clear accessor) or clear it in invalidateCases() and removeCases(). If it stays as-is, the comment should say it is valid only until the next mutation.

• v3/src/models/data/attribute.ts (lines 517-526), affecting the accessors at lines 404-420: the isNumeric fix covers the view that was reported, but growing the arrays in place narrows notification for the per-index accessors too (value, isValueNumeric, numValue, strValue, boolean). Your audit note is right that these never reacted to writes at existing indices, but main's self.strValues = self.strValues.concat(...) was a reassignment and so did notify on growth: reaction(() => a.strValue(2), ...) followed by a.setLength(3) observes nothing on this branch. I could not substantiate a broken consumer, since the dataset-level wrappers are equally non-reactive on main and any realistic reader of a new index also reads items/itemIds/case ids, which do change observably. Reporting the mechanism rather than a symptom: either give those five the same one-line void self.changeCount treatment so the file follows one convention, or note in setLength's comment that growth is now observable only through changeCount.

• v3/src/models/data/collection.ts (lines 322-324, 383-390, 461): unhiddenCaseIds is only ever tested for emptiness (data-set.ts line 795), yet it is returned as a list, which implies a completeness contract it does not have, since the caller stops at the first collection that reports one. Either return { newCaseIds, hasUnhiddenCases } or document that the contents are informational only.

• v3/src/models/data/collection.ts (lines 634-640): const childCaseIds = self.parent?.getCaseGroup(parentCaseId)?.childCaseIds ?? [] followed by firstNewIndex = childCaseIds.length - groups.length writes negative symIndex values with no warning if the parent case group is missing. The codebase already treats "parent case id with no case group" as worth a warning (collection.ts lines 499-502), which is the same condition that would empty childCaseIds here. I could not construct a reachable input, so treat this as defensive: skip the group or fall back to the rebuild branch when the parent group is undefined, rather than computing an index from a defaulted empty array.

• v3/src/models/data/collection.ts (line 599): newCaseIds?.length !== 0 reads as "is non-empty" but is also carrying the undefined case, which is only explained by the comment above it. newCaseIds == null || newCaseIds.length > 0 would put that at the expression. Related nit: the new comments at item-handler.ts lines 57-60 and data-set.ts lines 189-191 use -- where the surrounding comments in both files use a dash character.

On CI: the only non-green check is still cypress/flake (1 flaky test, 0 failures), which does not look related.

[kswenson]

Thanks — the blocking item was real and is fixed. Both triggers reproduced exactly as you described: 0 of 3 with an invalid data context on arrival, 0 of 6 through the un-hide fallback. Your reachability claim holds too — deleteItem, deleteCaseBy and itemSearch.delete all removeCases inside applyModelChange with no revalidation, so delete-then-create reaches it from ordinary plugin traffic.

On your suggested fix. Your (b) is implemented as you described and holds up: that path enters with grouping valid, so subtracting what the pass pushed does reconstruct what each collection held beforehand.

Your (a) I couldn't make work. It rests on the collections' current caseIds being a valid baseline, but they hold the last successfully grouped state — so when the dataset arrives invalid they're stale or empty, which is exactly when that path runs. Built as specified it reported 1 case of 3 after a delete-then-recreate, because the case id is regenerated from the retained groupKeyCaseIds and was still sitting in the stale baseline; and 11 of 3 after a document snapshot, because afterApplySnapshot → clearCases had emptied caseIds so everything looked new.

So (a) is handled by validating before the add instead. With grouping current, the additive pass has a consistent base and reports the ids it mints directly, and no baseline is involved.

Where that validate lives. In createItemsInSegments, not addCases. I tried it in addCases first, justified by "the caller validates straight afterwards anyway" — enumerating the call sites disproved that. There are ten-plus callers and only the item handler does; the v2 importer calls addCases on a still-invalid dataset and would have additively grouped every imported item into collections it discards two statements later. Validating is the request's concern, so addCases is unchanged, inserts still defer, and the accessor reports that it can't say rather than saying nothing was created.

Your non-blocking items. All taken except two:

• unhiddenCaseIds stays a list rather than a boolean. The fix now uses its contents to compute the un-hide baseline, so it is load-bearing and complete for the collection that reports it. Your premise held when you wrote it.
• The five per-index accessors keep their current behaviour. Making them read changeCount would make them react to every value write, and the case table reads them per cell, so it risks far more re-rendering than it fixes against a gap you couldn't tie to a consumer. setLength's comment now records that growth is observable only through changeCount.

Un-hidden cases count as created. A case that was set aside and is made visible again by an appended item is reported as created. That matches main, which diffs caseIds around the request and so reports it too, and it's what your un-hide test asserted. The alternative reading — a duplicate notification for a case the plugin already knows — is defensible, but the API doesn't specify this edge, and matching main is the safer default.

Also included, separately. getAfterPosition tested a resolved item index for truthiness, so after: <case whose last item is item 0> silently appended instead of inserting. Pre-existing and reachable only through the plugin API, since no in-repo caller passes after, but it's a one-word fix immediately beside code this PR already touches — it's its own commit rather than a follow-up story.

The new tests derive ground truth independently — a case belongs to a request exactly when every item in it came from that request — rather than from the dataset's own caseIds, and each assertion is mutation-verified.

[dougmartin]

Looks good 👍

All six items from the last round are addressed, and I confirmed each against the code. The blocking one is fixed at both triggers: an invalid data context on arrival is handled by validating before the add (item-handler.ts:55), and the un-hide fallback records a baseline and recovers the answer by diffing (data-set.ts:817-828, data-set.ts:889-904). I mutation-verified the baseline reconstruction: collapsing it to [...processed.caseIds] fails both new un-hide tests. newCaseIdsForLastAppend is now behind a consume-and-clear accessor and cleared in three places, so it can't hand a later reader stale ids. The two declined non-blocking items both hold up: unhiddenCaseIds' contents became load-bearing when the baseline started using them, so the completeness objection no longer applies, and leaving the five per-index accessors non-reactive is the right call now that setLength's comment records the mechanism. The getAfterPosition truthiness fix is correct and constrained by its test.

A few non-blocking notes (take or leave):

• v3/src/models/data/data-set.ts (lines 829-831): the self.validateCases() that closes last round's invalid-and-inconsistent note isn't constrained by any test. I deleted the line and all 60 suites under src/models/data and src/data-interactive still passed, because every test reaching this path calls validateCases() itself right afterwards, and takeCaseIdsCreatedByLastAppend re-validates too. Given the PR states every branch of the new code is mutation-verified, worth closing: in "restores a case that an appended item un-hides, in a middle collection", assert expect(data.isValidCases).toBe(true) and data.collections.forEach(c => expect(c.cases.length).toBe(c.caseIds.length)) immediately after the un-hiding addCases and before the explicit validateCases(). Both hold on the current code.

• v3/src/data-interactive/handlers/item-handler.ts (line 55): if (!dataContext.isValidCases) dataContext.validateCases() duplicates the guard validateCases() already opens with (data-set.ts:737), and reads an observable getter to do it. Calling dataContext.validateCases() unconditionally is equivalent; the comment above it is the part that earns its keep.

• v3/src/models/data/data-set.ts (lines 186-196): newCaseIdsForLastAppend and appendBaselineCaseIds are public volatile props, so the consume-and-clear contract is advisory. The whole point of the accessor is that "couldn't tell" must never be read as "created nothing", but anything can read dataSet.newCaseIdsForLastAppend directly, repeatedly, and find undefined, which walks straight back into the failure mode this PR has now fixed twice. A _ prefix would at least signal it; moving both into the .extend(self => ...) closure alongside _isValidCases, exposed only through the setter validateCasesForNewItems uses and the take accessor, would make it structural.

• v3/src/models/data/data-set.ts (lines 817-822): the baseline loop identifies the bailing collection two different ways, index < newCaseIdsByCollection.length and then the object identity test processed === collection, when both are saying "index k" with k === newCaseIdsByCollection.length by construction. The identity comparison also assumes self.collections returns the same MST node instances across two accesses, which is true but is an unnecessary dependency for what is arithmetic. Hoisting const bailIndex = newCaseIdsByCollection.length and branching on index < bailIndex / index === bailIndex / else would drop it. The logic itself is correct: I checked k = 0, middle, and last.

• v3/src/models/data/collection.ts (lines 636-648): the guard added for last round's note is if (!childCaseIds), but the failure it prevents (a negative symIndex from childCaseIds.length - groups.length) also occurs whenever childCaseIds.length < groups.length with a non-empty array. I could not construct a reachable input, since addChildCase and getCaseGroup resolve the parent through the same map pair, so this stays defensive-only, same standing as the original note. if (!childCaseIds || childCaseIds.length < groups.length) with the same warn-and-skip would cover it.

• v3/src/models/data/data-set.ts (line 897): the self.validateCases() inside the take accessor can't do work on any reachable path, since the fallback validates at line 831 and the only caller validates again at item-handler.ts:62. Harmless, but it makes the accessor look callable against an invalid dataset when the design requires the opposite. Either drop it or add a line saying it's belt-and-braces.

• v3/src/data-interactive/handlers/item-handler.test.ts (line 161), v3/src/models/data/data-set.test.ts (lines 1367, 1406), v3/src/models/data/data-set-hierarchical-append.test.ts (line 63): the -- comment style was fixed in the source files last round but reappears in the test comments added by the last two commits. Same one-character change as before, to match the dash character the surrounding comments use.

• v3/src/models/data/data-set.test.ts (line 1341): describe("caseIdsCreatedByLastAppend") names something that doesn't exist; the method is takeCaseIdsCreatedByLastAppend, and the take semantics are exactly what four of the six tests assert.

• v3/src/models/data/data-set.test.ts (lines 1350-1351): two consecutive blank lines after makeGrouped(). ESLint doesn't flag it, but nothing else in the file does it.

CI is green (27 passing), and lint is clean on all eight changed files.

[kswenson]

Thanks for the approval. All nine notes are in, pushed as 5644369.

Two are worth flagging because they changed code rather than comments, and landed after your approval:

The two fields are encapsulated now. You were right that the consume-and-clear contract was advisory while they were public props. They're closure variables alongside _isValidCases, reachable only through a setter pair and a take that consumes, so the contract holds by construction. It also reads better at the call sites, and they stop being observable writes that nothing observed.

The validateCases() you couldn't find a test for really had none — I reproduced your result, deleting it passed all 476. The un-hide middle-collection test now asserts isValidCases and caseIds/cases agreement immediately after the append and before the explicit validate, and fails without the line. That also corrects the PR description's claim that every branch was mutation-verified, which wasn't true for that one.

The rest as you suggested: the handler calls validateCases() unconditionally; the missing-parent guard covers childCaseIds.length < groups.length rather than only the empty case; the bailing collection is found by index off a hoisted bailIndex instead of by object identity; the take accessor's validateCases() says why it's there; and the describe name, the doubled blank line, and the comment dashes are fixed.

On the take accessor I kept the call rather than dropping it. You're right nothing reachable needs it, but the failure it guards against is diffing a baseline against grouping that hasn't caught up, which is the defect this branch fixed twice — and the last thing I removed as unreachable turned out to be load-bearing as soon as the design shifted. It's commented as belt-and-braces.

Full suite green locally (3757); CI is running on the push.