fix(gui): stop Radio Setup nav relayout from crashing macOS accessibility queries. Principle XI.

[w5jwp]

Summary

Fixes #5837

On macOS, selecting a page in Radio Setup crashes the app (SIGSEGV at 0x8 in QAbstractScrollArea::viewport(), reached from QAccessibleTableCell::rect() ← state() ← QCocoaAccessible::shouldBeIgnored) when an accessibility client asks for the app's focused element. Full root-cause trace is in the issue.

Short version: RadioSetupDialog::updateRadioCapabilityVisibility() runs on every GPS / oscillator / capability / connection status message and called QTreeWidgetItem::setHidden() on navigation rows even when nothing changed. setHidden() → QTreeView::setRowHidden() can schedule a delayed QTreeView layout, so on a connected FLEX (GPS status arrives about once a second) the navigation tree almost always had a layout pending. When macOS then queries the focused cell, QAccessibleTableCell::rect() → QTreeView::visualRect() runs that pending doItemsLayout() from inside the cell's own method; the layout emits QAccessible::TableModelChanged, which frees every accessible cell including the running one, and the next view->viewport() dereferences null (Qt 6.8.3).

The change, all in RadioSetupDialog.cpp:

• setNavigationItemHidden() does nothing when the row's hidden state is unchanged (the steady-state case that was leaving the layout pending) and returns whether it changed anything.
• settleNavigationLayout() runs doItemsLayout() once per handler, and only if a row changed, on our own call stack, so no accessibility query can find a layout pending. It is called at the end of the handlers that can flip rows: updateRadioCapabilityVisibility(), the search filter, and the three connectionStateChanged / apdStateChanged lambdas.

All ten setHidden() calls in the dialog (Calibration, Droop Correction, APD, Filters, SmartLink, GPS, and the search filter) go through the helper. No behavior change is intended: the same rows are shown and hidden in the same conditions.

This works around a Qt bug rather than fixing it; the defect itself (a layout run re-entrantly from inside QAccessibleTableCell::rect()) should be reported upstream. Other item views that can have a layout pending when a focus query arrives are not fixed here; the same pattern appears in NetworkDiagnosticsDialog and ThemeEditorDialog (follow-up: #5839).

Approach notes:

• A first attempt that only flushed the layout in the currentItemChanged handler did not fix the crash, because the next GPS status message dirtied the tree again. It is not in this PR.
• The first version of this PR flushed after every flipped row. Review pointed out that this turns Qt's one coalesced layout into one synchronous layout per row (about 20 for a single search keystroke), each rebuilding every accessible cell interface. It now flushes once per handler.

Constitution principle honored

Principle XI — Fixes Are Demonstrated. The crash was reproduced on demand under lldb on unpatched main, the same repro was run on the patched build, and the failure mode of the rejected first attempt is documented above.

Test plan

• Local build passes (cmake --build build --target AetherSDR, Qt 6.8.3 arm64, macOS 27.0). Only the AetherSDR target was built locally; CI ran the rest.
• Behavior verified on a real radio: FLEX-8600, SmartSDR 4.2.20.41343, GPS locked.
• Existing tests pass (CI) — CI was green on the first version of this PR; re-running on this update.
• Reproduction steps documented (see the issue)

No regression test is added. The crash needs a real QTreeView, real focus and a real AXFocusedUIElement request on macOS, which the socket-free test layer cannot express.

Verification detail (real clicks, lldb breakpoints on QTreeWidgetItem::setHidden, QTreeView::doItemsLayout and RadioModel::gpsStatusChanged, tree focused, a separate process polling AXFocusedUIElement every 20 ms):

• Unpatched main (d3743a9): crashes on every attempt (Network click → Audio click), stack as above, confirmed with symbolicated Qt frames.
• First attempt (handler flush only): still crashes.
• This PR, steady state: in a 30 s idle window, 30 gpsStatusChanged emissions, 0 setHidden calls and 0 doItemsLayout calls.
• This PR, search filter: typing "audio" flipped 19 rows with 1 layout; "gps" flipped 4 with 1; clearing flipped 19 with 1. (The per-row version ran one layout per flipped row.)
• This PR, real state changes: disconnect and reconnect each made exactly one setHidden and one layout.
• No crash in any of these runs.
• Focus-query failures: in the runs with the poller, some queries returned an error instead of a focused element (54 of 3,357 with the debugger attached; about 50 of about 1,600 in a rerun without it, slowest query 0.09 s). The codes I printed were -25212 (kAXErrorNoValue, "no focused element"), starting when the search filter hid the currently selected row. I read that as expected, since a filtered-out row has no focused cell, and not as a hang or crash, but I only printed the first dozen codes.

Not covered:

• A real state flip through updateRadioCapabilityVisibility()'s own call sites (for example a GPSDO appearing): on this radio the Filters, SmartLink and GPS rows never changed state. Disconnect and reconnect exercised the helper through the connectionStateChanged lambda instead, and I did not record which row that toggled.
• Other macOS versions (this was all on macOS 27.0, build 26A428, a brand-new release, so I do not know whether the trigger is new to it), Intel Macs, other Qt versions, Linux/Windows (the change is platform-neutral, but the crash it addresses is in the macOS accessibility bridge).
• It is a timing-sensitive crash, so a clean run is evidence, not proof.

Checklist

• Commits are signed (docs/COMMIT-SIGNING.md) — OpenPGP-signed; GitHub shows the commit as Verified
• No new flat-key AppSettings calls — none touched (Principle V)
• Code is clean-room — not decompiled, disassembled, or reverse-engineered from a proprietary binary (Principle IV). Only Qt's own frames in the crash report and Qt's open-source behavior were used to locate the cause.
• All meter UI uses MeterSmoother — n/a, no meter UI touched
• Documentation updated if user-visible behavior changed — n/a, no user-visible change; CHANGELOG.md not touched
• Security-sensitive changes reference a GHSA if applicable — n/a

🤖 Generated with Claude Code

[w5jwp]

Following up on the triage note on #5837, which asked me to confirm the flush is reached on real state changes and not only on the no-op path. I ran that check on the PR head (01a0e453) and it holds, with one gap noted at the end.

Setup: patched build against Qt 6.8.3, macOS 27.0 (26A428), FLEX-8600 (GPS locked), run under lldb with breakpoints on QTreeWidgetItem::setHidden and RadioModel::gpsStatusChanged. Real keyboard focus was on the Radio Setup navigation tree, and a separate process polled AXFocusedUIElement every 20 ms for the whole run (2,031 queries, 0 failures). The app never crashed.

Steady state (the crash path): over a 60 s idle window, gpsStatusChanged fired 60 times (about once a second, so the "ticking time field" point in the triage is right) and setHidden was called 0 times. Before this change, each of those emissions went through setHidden on the Filters, SmartLink and GPS rows; a short earlier run on the pre-fix code recorded 15 setHidden calls reached via gpsStatusChanged.

Real state changes:

• Dialog construction and initial connect: 4 real setHidden calls, all through the helper.
• Disconnect: exactly one setHidden(true), from the connectionStateChanged lambda in the RadioSetupDialog constructor.
• Reconnect: exactly one setHidden(false), on the same row from the same lambda.

Each of those real changes went through the new flush path with an accessibility poller running and the tree focused, and nothing crashed.

Not covered:

• On this radio the Filters, SmartLink and GPS rows did not change state across disconnect/reconnect, so a real flip through updateRadioCapabilityVisibility()'s own call sites (for example a GPSDO appearing) was not exercised. I can't provoke that on this hardware.
• I did not record which row the disconnect/reconnect lambda toggled.
• No regression test yet, and I haven't verified the triage's note about setExpanded(true) in the search handler.
• It is a timing-sensitive crash, so a clean run is evidence rather than proof.

[aethersdr-agent]

1. Issue fit

Yes, for the surface the issue names. #5837's root-cause chain is: updateRadioCapabilityVisibility() fires on every GPS/oscillator/capability/connection message → setHidden() → doDelayedItemsLayout() → a layout is left pending almost permanently → an AXFocusedUIElement query runs it re-entrantly from inside QAccessibleTableCell::rect() → TableModelChanged frees the running cell → null viewport(). The diff attacks link 2/3: no-op when unchanged, and flush synchronously when it does change so nothing is left pending for the accessibility path to execute. I read all 13 setHidden sites in RadioSetupDialog.cpp at head — every one now routes through setNavigationItemHidden(), with none missed (grep -n setHidden src/gui/RadioSetupDialog.cpp shows only the one inside the helper at :287).

I could not reproduce the crash or the fix: headless, no build, no macOS accessibility client. Everything below is reasoned from source, not observed running. The issue's own evidence — six identical .ips reports, an lldb breakpoint on deleteAccessibleInterface, and a 25 s AXFocusedUIElement poll-while-clicking stress run after the patch — is the strongest evidence here and I cannot independently confirm any of it.

Test coverage: none added, and I am not asking for it. There is no deterministic socket-free seam for "a pending QTreeView layout is executed re-entrantly from an AppKit accessibility query" — that needs a real QTreeView, a real focus, and a real AXFocusedUIElement request on macOS. Per AGENTS.md's test-layer boundary this lands in the GUI/platform layer that has no registered lane. The reporter's stress run is the right evidence for this class. Noting it as a documented coverage boundary, not a nit.

2. Scope

File / group	What it changes	Claimed by title/body?	Verdict
src/gui/RadioSetupDialog.cpp — new static setNavigationItemHidden() (+ 15-line comment)	Adds the guard-and-flush helper	Yes	In scope
src/gui/RadioSetupDialog.cpp — 9 call-site conversions (ctor ×3, connect lambdas ×3, search lambda ×2, updateRadioCapabilityVisibility ×2)	Routes existing setHidden() through the helper	Yes	In scope
Reindentation at :1138 and :1147	Whitespace to fit the new call	Implied	In scope

Everything in the diff is explained by the issue. One file, no new public surface, no settings key, no capability field, no default changed, no CHANGELOG edit, no removed guard, no deleted comment naming a symptom. I diffed the two reformatted boolean expressions token-by-token against the base — !isCapabilityPageAvailable(item) || (!needle.isEmpty() && !haystack.contains(needle, Qt::CaseInsensitive)) is byte-identical in meaning; only leading whitespace moved.

No socket test added, modified, or removed — nothing in this diff touches tests/.

3. Blockers

None.

4. Nits (non-blocking)

• The same defect is live in NetworkDiagnosticsDialog, and there on a 1 Hz timer. NetworkDiagnosticsDialog::refresh() is wired to m_refreshTimer started at m_refreshTimer.start(1000) (NetworkDiagnosticsDialog.cpp:1002-1003) and calls m_digitalVoiceWaveformNavigationItem->setHidden(!waveformSeen) unconditionally at :2561. On any session without digital-voice waveform telemetry — the common case — that is setHidden(true) on an already-hidden row of an identical navigation QTreeWidget, once per second, for as long as the dialog is open. If the PR's mechanism is right, that dialog has a layout pending essentially 100% of the time, which is a stronger version of the Radio Setup condition, not a weaker one. :977 / :983 (search filter) and ThemeEditorDialog.cpp:987 are the same pattern on keystroke cadence. I am not asking to fix them here — #5837 is about Radio Setup and unbundling is correct — but the PR reads as "fixed" while a sibling surface still carries it. Worth a follow-up issue so the fix does not have to be rediscovered from a second crash report. See inline.
• Flushing per-row turns one coalesced layout into N synchronous ones. Before this change, N rows flipping in one handler scheduled N delayed layouts that Qt coalesced into exactly one at the next event-loop turn. After, each flip runs a full doItemsLayout() inline. The search-filter lambda is the sharp case: the tree has 19 pages + 4 categories, and the first typed character hides most of them, so one textChanged can now run ~20 full layouts, each emitting QAccessible::TableModelChanged and tearing down/rebuilding every cell interface. That is more accessibility-interface churn than before, on the exact mechanism that is the crash — safe now because it is on our own stack, but it is not free. A dirty-flag + single flush at the end of each handler keeps the fix and drops the churn back to one. See inline.
• The body says setHidden() "schedules a delayed layout even when the value is unchanged." I could not check this: no Qt sources on this machine to read QTreeView::setRowHidden. My recollection is that the unhide branch early-returns when the row is not in hiddenIndexes, so the unconditional scheduling would hold only for hidden→hidden. That does not affect the fix (the hidden rows are exactly the capability-gated ones cycling on status messages), but the claim is stated more broadly than I could verify.

5. What I tried to break

• Looked for the call site left behind. grep -n setHidden on the head checkout returns 13 hits in the file; 12 are setNavigationItemHidden calls and the 13th is inside the helper. None missed. Then widened the grep repo-wide, which is where the NetworkDiagnosticsDialog sibling above came from.
• Tried to desync the guard from the setter. item->isHidden() reads the row's own hidden flag via isRowHidden(row, parent), the same state setHidden() writes — not effective visibility — so a collapsed or hidden parent cannot make the guard skip a real state change. The null-item and no-tree-yet paths both degrade to a harmless no-op.
• Attacked the reindented booleans at :1138 and :1147. Compared against base token-by-token; ||/&& precedence and operand order are unchanged. This was the most likely place for a silent logic change to hide, and it is clean.
• Checked reentrancy of the new synchronous flush. Traced every caller: constructor, three connect lambdas on model signals, the textChanged search lambda, and updateRadioCapabilityVisibility(). None is reachable from inside an accessibility query, so doItemsLayout() never runs on the stack the crash needs. Also confirmed category->setExpanded(true) at :916 early-returns when already expanded, so it does not re-dirty the tree behind the flush.
• Checked the construction-order path. Lines 799/814/829 run while the tree is still being populated — doItemsLayout() there lays out a partial tree that later addPage() calls re-lay out. Harmless, just three extra layouts at dialog construction.
• CI: all 5 checks green on 01a0e45 (build, Static checks, check-macos, check-windows, Sanitizer option configures). Green here means it compiles on three platforms; no check exercises this code path, so it is not evidence about the fix.

6. Recommendation

Approve with nits. Small, single-file, every call site converted, no logic drift in the reformatted expressions, and the root-cause analysis in #5837 is unusually well-evidenced (symbolicated lldb trace, six matching crash reports, a stress run that reproduces before and not after). The two nits are a follow-up issue for the NetworkDiagnosticsDialog sibling and an optional batch-the-flush refactor — neither should hold the merge, since the crash is on a priority: high macOS path and this removes the window for it. Concrete next step: a maintainer merges after the signing fix below, and opens a follow-up for the NetworkDiagnosticsDialog:2561 sibling.

Nice find, and thank you for the trace — the deleteAccessibleInterface breakpoint is what makes this reviewable at all.

---

One more thing: commit signing

main requires verified signatures, and the 1 commit on this branch is unsigned — routine setup, not a code problem.

Quickest setup (SSH key signing, no GPG needed):

git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign true

(If you have no SSH key: ssh-keygen -t ed25519 first.) Then on GitHub: Settings → SSH and GPG keys → New SSH key → set the key type dropdown to Signing Key → paste the .pub.

Re-sign the commit already on this branch:

git rebase main --exec "git commit --amend --no-edit -n -S"
git push --force-with-lease

For GPG or troubleshooting: https://docs.github.com/authentication/managing-commit-signature-verification

---

_{🤖 aethersdr-agent · cost: $6.3337 · model: claude-opus-5}

[w5jwp]

Thanks for the review. Pushed an update (55cba3a4, still one commit) and updated the PR description.

Batching (the flush-per-row nit): taken. setNavigationItemHidden() now just reports whether it changed the row, and each handler that can flip rows calls settleNavigationLayout() once at its end (the three connectionStateChanged / apdStateChanged lambdas, the search filter, and updateRadioCapabilityVisibility()). Re-measured under lldb with the tree focused and an AXFocusedUIElement poller running:

• 30 s idle: 30 gpsStatusChanged, 0 setHidden, 0 doItemsLayout.
• search "audio": 19 rows flipped, 1 layout (previously one per row); "gps": 4 rows, 1 layout; clearing: 19 rows, 1 layout.
• disconnect and reconnect: one setHidden and one layout each.
• no crash.

NetworkDiagnosticsDialog / ThemeEditorDialog: filed as #5839, kept out of this PR as suggested. I checked your line references and they match main, but I have not reproduced a crash there, and the issue says so.

"Even when unchanged" wording: you were right to flag it. I could not read Qt's source either, and I don't know which direction each of those calls went. The PR description and the code comment now say setHidden() "can schedule" a layout, not that it always does. The fix covers both cases.

One thing I noticed while re-testing: with the poller running, some AXFocusedUIElement queries return an error instead of an element (about 3% in one run). The codes I printed were -25212 (kAXErrorNoValue, "no focused element") and started when the search filter hid the currently selected row, which I read as expected. I only printed the first dozen codes, so that reading isn't fully confirmed. Details are in the PR description.

Update: the commit is now OpenPGP-signed and GitHub shows it as Verified (head 358cc877).

[NF0T]

Thanks @w5jwp. This is a well-diagnosed fix, and you changed it in the direction the review asked and then measured the result. I reviewed head 358cc877 (signed, Verified) and tried to break it.

What's right, and how I checked it

The mechanism holds against Qt's own source, not just the crash trace. Qt 6.8.3, qtreeview.cpp:

• QTreeView::setRowHidden() ends with an unconditional d->doDelayedItemsLayout(). The isPersistent() early-out only guards the hiddenIndexes.remove(), not the scheduling. So setHidden() posts a layout in both directions, hidden→hidden and visible→visible.
• QTreeView::doItemsLayout() → QAbstractItemView::doItemsLayout() → interruptDelayedItemsLayout() stops the timer and clears delayedPendingLayout. So settleNavigationLayout() really does leave nothing pending.
• QTreeWidgetItem::isHidden() reads the view's own hiddenIndexes entry for that row, not effective visibility. A hidden parent cannot make the isHidden() == hidden guard skip a real change.
• QTreeView::expand() returns early when the index is already expanded. This closes the setExpanded(true) question from the triage note.

I also measured it with a standalone probe (a QTreeWidget subclass counting doItemsLayout() calls, using your two helpers verbatim; Qt 6.11.2, Windows, offscreen; 4 categories × 5 rows):

Case	Layouts
Nothing touched (control)	0
Pre-PR: setHidden(true) on an already-hidden row	1 posted
Pre-PR: setHidden(false) on an already-visible row	1 posted
Pre-PR: 20× same-state setHidden	1 (coalesced)
PR: 20× guarded same-state + settle	0
PR: 10 rows flipped in one handler	1 synchronous, 0 more after the event loop (no leftover pending layout)
Pre-PR: same 10-row flip	0 synchronous, 1 posted
setExpanded(true) on an expanded category, and on a hidden+expanded one	0

For the re-entrancy step, I took a QAccessibleTableCell id, dirtied the tree, then re-resolved the id and called state()/rect(), as a native client would. With the pre-PR pattern the pending layout ran inside the cell's state() (1 layout observed after the query). With the guard it did not (0). On Qt 6.11.2 the cell interface was not freed, so I could not reproduce the crash itself here (no AppKit, and a newer Qt). The macOS crash and the before/after lldb results rest on your evidence, which I have not independently confirmed.

Coverage: all 10 setHidden sites in the file now go through the helper. grep at head finds no other setHidden. updateRadioCapabilityVisibility() has no early return before its settle, and nothing else in the GPS-tick path mutates the navigation tree. The reformatted boolean expressions are equivalent to base. The merge is clean against current main (8f06ac87).

Scope

File / group	Claimed by title/body?	Verdict
RadioSetupDialog.cpp: two new file-static helpers plus mechanism comment	Yes	In scope
RadioSetupDialog.cpp: 10 call-site conversions, 5 settle calls	Yes	In scope

One file, one commit, no new public surface, no settings key, no default changed, no CHANGELOG edit, no deleted guard. This is a fix, not a preference change: the same rows show and hide under the same conditions. NetworkDiagnosticsDialog and ThemeEditorDialog were kept out and filed as #5839.

CI and process

• All 5 checks are green on the current head, and ci.yml runs on pull_request, so it tests the merge result. Nothing exercises this path (the crash needs an AppKit focus query), so green here means "compiles on three platforms", not "fixed". I don't count that against the PR.
• Fixes #5837 is present, the commit is signed, and the PR title cites Principle XI. I read XI: it locates the demonstration in the squash-merge process, so the citation is right regardless of the PR-body demonstration.
• No regression test, and I'm not asking for one. The crash needs a real QTreeView, real focus and an AppKit request. See the note below on a seam for #5839.
• The formal review from aethersdr-agent was written against 01a0e453. Both of its non-blocking nits (flush-per-row, and the sibling dialogs) are addressed in the current head.
• Code owners: only src/gui/RadioSetupDialog.cpp is touched, which is Tier 3 (@aethersdr/reviewers).

Verdict: approve, no blockers

Forward guidance (not blocking, and not moved goalposts)

• For #5839: lifting the helpers into a header makes a small socket-free test possible: a QTreeWidget subclass that counts doItemsLayout(), asserting 0 layouts on repeated same-state calls and exactly 1 synchronous layout per changed handler. My probe is about 60 lines of this shape, and I'm happy to attach it. It would pin the guard without needing macOS.
• A correction to the #5839 triage note, which also matters for its design: it says the early return is on the unhide branch and that un-hiding a non-hidden row doesn't schedule a layout. In 6.8.3 both directions schedule (see above, and the probe row for setHidden(false) on a visible row). The guard is therefore needed on both directions, not just hide.
• Your wording is right. "can schedule" is what the source supports. Keep it.

Questions, where you have context I lack

1. On the -25212 (kAXErrorNoValue) results: when the search filter hides the selected row, does focus land somewhere sensible afterward? The tree keeps focus but has no current cell. That was true before this PR too, so I'm asking only whether it's worth a note in #5839.
2. You couldn't flip Filters/SmartLink/GPS on your radio. If someone with a GPSDO or a different family can, that would close the one "Not covered" item, but I would not hold the merge for it.

What I tried to break

• The claim that unchanged setHidden() posts a layout: measured, both directions.
• The claim that a per-handler doItemsLayout() leaves nothing pending: measured, plus Qt source for interruptDelayedItemsLayout().
• A hidden parent making the isHidden() guard skip a real flip: read QTreeWidgetItem::isHidden(), which is own-row state.
• A site left behind, and other tree mutations in the GPS-tick path: grep of the file at head.
• The reformatted boolean expressions: compared against base.
• Re-entrancy with the old pattern versus the guard: reproduced (layout runs inside state()), crash not reproduced (Qt 6.11.2 on Windows).
• "No behavior change", driven against the app: I built the merge base (d3743a9b) and this head (358cc877) on Windows (MinGW, Qt 6.11.2, ENABLE_ASR=OFF; the two binaries differ only in RadioSetupDialog.cpp). I ran both offscreen against the demo simulator (DEMO-0001, isolated settings, AETHER_AUTOMATION_NO_TX=1) and drove the same script through the bridge over a socket. The script opens Radio Setup, searches audio, gps, calib, a no-match string and filter, clears the search, disconnects, searches and clears again, reconnects, and searches gps and clears again. I grabbed radioSetupNavigation at each of the 13 states. All 13 screenshots are byte-identical between base and head. The comparison does discriminate: the states differ from each other (connected vs disconnected shows Filters and GPS rows appearing and disappearing, so those flips go through updateRadioCapabilityVisibility()), and clearing the search returns to the initial image on both builds.
• Not done: the macOS crash and the Qt 6.8.3 behaviour are not verified (nothing here reproduces an AppKit focus query). I did not count layouts inside the real app, only in the standalone probe. The demo backend only exercises one capability set.

Suggested squash subject: fix(gui): stop Radio Setup nav relayout from crashing macOS accessibility queries. Principle XI. (#5838)

Nice find, and thanks for measuring rather than asserting.

73,
Ryan NF0T

---

👨🏼‍💻 Co-authored by Claude Sonnet 5