Skip to content

LT-22848: Stop Delete Example from crashing the New UI detail view - #1193

Merged
thejambi merged 1 commit into
mainfrom
LT-22848-delete-example-crash
Oct 20, 2026
Merged

thejambi merged 1 commit into
mainfrom
LT-22848-delete-example-crash

Conversation

@thejambi

@thejambi thejambi commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Start here: Src/Common/Controls/DetailControls/DataTree.cs, SelectFirstPossibleSlice -- the one changed method.

Fixes LT-22848: in the New UI, deleting an Example from its row menu crashed FLEx with "Unable to find hvo ... in the object dictionary". After this change the Example is deleted and the detail view carries on.

Why a WinForms fix cures an Avalonia crash. The Example's row menu is not wholly answered natively, so all of its commands, Delete included, run on the hidden legacy DataTree the Avalonia view keeps for command routing. Slice.HandleDeleteCommand deletes the object, then calls SelectFirstPossibleSlice with the nearby slices, its own first. The delete postpones the tree's rebuild (the LT-21980 / LT-22011 guard), so that slice still exists. In the visible legacy view its window already exists, so focusing it is harmless and the rebuild discards it. The hidden tree was never shown, so focusing it creates the window, and StringSliceView.MakeRoot looks the deleted object up and throws. The fix skips a slice whose object has been deleted.

Where to look

  • Legacy UI focus after a delete. Shared code: the deleted slice no longer becomes current for a moment, so a neighbour can. Checked by hand -- the cursor lands in Lexeme Form.
  • Condition order. IsDisposed is tested before Slice.Object, which throws on a disposed slice.

Not here

  • The other row-menu commands (Insert, Swap, Convert, Merge) were not audited for stale hidden-tree slices; this fixes the delete-focus path.
  • No automated test of a shown tree; the legacy path rests on the manual check below.

Verification

  • DetailControlsTests: 125 passed, 1 skipped (NestedCollapsedPart, skipped on main too). The new test crashes the test host without the fix, with the same exception as the field report.
  • Manual, New UI: delete an existing Example, Undo, delete it again; delete an Allomorph and a Pronunciation. No crash.
  • Manual, legacy UI: Delete Example; the cursor lands in Lexeme Form.
  • Not run locally: the full xWorksTests suite on this branch -- CI runs it.

Next: review the one condition, or tell me the hidden command tree should instead be reset after every command.


Reading this a year from now -- start here

This PR is the record of why SelectFirstPossibleSlice skips slices of deleted objects. The crash only happens when the WinForms DataTree is used as a hidden command router behind the Avalonia detail view (RecordEditView.EnsureMenuCommandAdapter), a state the WinForms UI on its own never reaches. If that hidden tree is ever retired, this condition becomes defensive only.

Paths not taken
  • Resetting the hidden tree in RecordEditView.DeleteDetailObject. The first diagnosis assumed Delete Example ran through the native delete added in LT-22691 (LT-22691: Answer the Delete leaf of the Lexicon detail menus natively #1182). It does not: a row menu is answered natively only when every leaf under it is, and mnuDataTree-Example has leaves that are not, so the whole menu runs through DTMenuHandler.OnDataTreeDelete on the hidden tree. A reset there would never have run. A captured stack of the test thread showed the real path; the fix moved to where the stale slice is actually focused.
  • Resetting the hidden tree after every menu command. Would also clear the stale slice, but it changes how every adapter-routed command is targeted, well beyond this crash. Not attempted.
Surprising findings
  • Native ownership of a row menu is all-or-nothing per menu id (LexiconMenuAuthority.Owns). Delete Example looks native in the code but is not, because its menu carries other leaves.
  • The WinForms UI runs the same postponed-rebuild sequence on every delete and survives only because its slices already have windows.
Evidence

Field report (New UI, FLEx 9.3.12 developer build), and the identical failure from the new test without the fix:

KeyNotFoundException: Unable to find hvo 11574 in the object dictionary. 11575 is the next available hvo
   at SIL.LCModel.Infrastructure.Impl.IdentityMap.GetObject(Int32 hvo)
   at SIL.FieldWorks.Common.Framework.DetailControls.StringSlice.StringSliceView.MakeRoot()
   at SIL.FieldWorks.Common.RootSites.SimpleRootSite.OnHandleCreated(EventArgs e)

The test, DataTreeTests.SelectFirstPossibleSlice_SkipsTheSliceOfADeletedObject_InATreeNeverShown, shows an entry in a parented but never-shown tree, deletes a sense whose ScientificName has a StringSlice inside the open unit of work (so the rebuild has not run), and asks for the next slice to focus.

Preflight review details

Code Review Summary

Branch: LT-22848-delete-example-crash
Base: origin/main (620cd99)
Date: 2026-10-20
Review model: Claude Opus 5.5
Files changed: 2

Overview

Fixes a crash when deleting an Example from its row menu in the New UI (Avalonia)
Lexicon Edit detail view. That menu is not wholly answered natively, so its
commands, Delete included, run on the hidden WinForms DataTree the view keeps for
command routing. Slice.HandleDeleteCommand deletes the object, then asks
DataTree.SelectFirstPossibleSlice to focus a neighbor, starting with its own
slice. The delete postpones the tree's rebuild, so that slice still exists; in a
tree never shown, focusing it creates its window, and StringSliceView.MakeRoot
looks the deleted object up and throws KeyNotFoundException.

SelectFirstPossibleSlice now skips a slice whose object has been deleted. A
DataTreeTests case reproduces the exact crash on a never-shown tree and passes
with the fix.

Contract/API Changes

None. DataTree.SelectFirstPossibleSlice is internal; its behavior changes only
for slices whose object is no longer valid.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

  • WinForms UI focus after a delete may change. The method is shared with
    the classic detail view. Previously the deleted object's slice briefly became
    current; now a neighboring slice does, and the rebuild can restore it.

    (Author checked the classic UI: after Delete Example the cursor lands in the
    Lexeme Form field, which is acceptable.)

Minor - Consider

  • No automated test covers a shown tree. The test exercises a tree that is
    never shown, the state that crashed; the classic UI path is covered by the
    author's manual check only.
  • Reference comparison in the test. slice.Object == deletedSense is an
    intentional identity check; the analyzer flags it.

Required Validation / Evidence

  • ./test.ps1 -CommentHygiene -TokenHygiene -TestProject DetailControlsTests:
    125 passed, 1 skipped (NestedCollapsedPart, skipped on main too).
  • New test fails without the fix: the test host crashes with the same
    KeyNotFoundException from StringSliceView.MakeRoot.
  • Manual, by the author (New UI): Delete Example on an existing Example, Undo
    restores it, delete again, and delete an Allomorph and a Pronunciation; no crash.
  • Manual, by the author (classic UI): Delete Example; cursor lands in Lexeme Form.

Positive Observations

  • One condition; IsDisposed is checked first, which matters because
    Slice.Object throws on a disposed slice.
  • The regression test reproduces the field crash exactly, with no app or desktop.
  • The second caller (HandleShowHiddenFields) only passes slices that survive a
    rebuild, so it is unaffected.

Interview Notes

  • Classic UI focus after delete: lands in Lexeme Form; acceptable.
  • Ticket: LT-22848 (filed during this work; analysis comment plus a correction
    comment giving the verified path).
  • Nothing further to flag.

Suggested Review Focus

  • Whether skipping a deleted object's slice in SelectFirstPossibleSlice is
    the right layer, as opposed to resetting the hidden command tree after commands.

🤖 Generated with Claude Code


This change is Reviewable

Deleting an Example from its row menu in the Avalonia detail view
crashed FieldWorks. That menu runs on the hidden WinForms DataTree,
whose rebuild after the delete is postponed, so the deleted object's
slice was still listed when the next slice to focus was chosen. The
hidden tree was never shown, so focusing that slice built its view,
which looked the deleted object up and threw.

SelectFirstPossibleSlice now skips a slice whose object has been
deleted. A DataTreeTests case reproduces the crash on a tree that is
never shown and passes with the fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   14m 10s ⏱️ + 4m 23s
6 725 tests +1  6 640 ✅ +1  85 💤 ±0  0 ❌ ±0 
6 734 runs  +1  6 649 ✅ +1  85 💤 ±0  0 ❌ ±0 

Results for commit 8239498. ± Comparison against base commit 620cd99.

@codecov-commenter

codecov-commenter commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.74%. Comparing base (620cd99) to head (8239498).

Files with missing lines Patch % Lines
Src/Common/Controls/DetailControls/DataTree.cs 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1193   +/-   ##
=======================================
  Coverage   39.74%   39.74%           
=======================================
  Files        1544     1544           
  Lines      355085   355089    +4     
  Branches    41247    41248    +1     
=======================================
+ Hits       141121   141147   +26     
+ Misses     184600   184581   -19     
+ Partials    29364    29361    -3     
Files with missing lines Coverage Δ
Src/Common/Controls/DetailControls/DataTree.cs 47.57% <60.00%> (+0.31%) ⬆️

... and 5 files with indirect coverage changes

🚀 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.

@thejambi
thejambi marked this pull request as ready for review October 20, 2026 16:52

@mark-sil mark-sil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thejambi
thejambi merged commit 5ccdba7 into main Oct 20, 2026
8 checks passed
@thejambi
thejambi deleted the LT-22848-delete-example-crash branch October 20, 2026 20:53
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.

3 participants