Repository navigation
fix(macOS): restore B1 on saved-left panel startup - #6453
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes. #6443 is a deterministic macOS regression from #6198: buildUI() creates the B1 placeholder, then restores the saved dock side at MainWindow.cpp:5895, which fires refreshAfterLayoutShift() before MainWindow::show(). refreshAfterReparent() read isVisible() (false for a child of an unshown top level), called hide(), and never paired the show() — leaving Qt's explicit hide flag set, which showChildren() honours. Both halves of the triage ruling on #6443 are implemented, in the form it proposed: isHidden() in the helper (PanadapterStack.cpp:37) and a pre-show early return in refreshAfterLayoutShift() (:360-362). The reporter's own A/B (close with panel LEFT → blank B1, RIGHT → fine) matches the fixed path. Nothing in the issue is left untouched; the only requirement the thread named and the diff does not carry is the regression test (nit 1).
Scope
| File / hunk | What it changes | Claimed by title/body | Verdict |
|---|---|---|---|
src/gui/PanadapterStack.cpp:33-49 (refreshAfterReparent) |
isVisible() → isHidden() for the hide/show round trip |
Yes — the primary fix | In scope |
src/gui/PanadapterStack.cpp:356-362 (refreshAfterLayoutShift) |
early return when window() is not visible |
Yes — "skips the macOS native surface refresh while the stack's top-level window is hidden" | In scope |
docs/user/docs/panadapter-controls.md:164 |
two sentences on dock-side persistence and macOS startup | Yes — "The user guide documents startup with either saved dock side" | In scope (see nit 2) |
Everything in the diff is explained by #6443. No sibling copy of the visibility bookkeeping exists — refreshAfterReparent() is the single helper and all six call sites (:376, :667, :1021, :1093, :1164) share it. The body's "Windows/Linux rendering paths are unchanged" holds literally: both hunks sit inside #if defined(Q_OS_MACOS) && defined(AETHER_GPU_SPECTRUM), and the #else arm is still a bare resetGpuResources(). versioned_docs/ and CHANGELOG.md are correctly untouched. No new public surface, no settings key, no removed guard — AppletPanelDockedLeft persistence at MainWindow.cpp:10035 predates this PR, so the doc's first sentence documents existing behaviour rather than new behaviour.
Blockers
None.
Nits
- No regression test (non-blocking). The #6443 triage asked for one: build the stack, call
refreshAfterLayoutShift()beforeshow(), then assert!sw->isHidden()after the window is shown. The honest obstacle is that the whole seam is compiled out except on macOS withAETHER_GPU_SPECTRUM, so such a case added tonative_widget_topology_testwould pass vacuously on the Linux full suite — which is presumably why the triage said the bookkeeping would need hoisting out of the#if, i.e. a restructure this fix does not otherwise need.check-macosdoes runctest, but only-R '^mac_nr_filter_test$'(ci.yml:1174) andasr_gpu_probe_test(:1189), so a new macOS-only case would not gate PR CI without touching the frozen test gate. The three tests the body cites (native_widget_topology_test,floating_restore_policy_test,window_show_state_test) are neighbouring coverage and none of them would fail against the unfixed code — the body says as much, which is the right call. Reporting as a nit rather than a blocker because there is no deterministic policy-compliant seam here without that restructure. docs/user/docs/panadapter-controls.md:164— the second sentence documents the absence of a bug:On macOS, the primary panadapter's spectrum, waterfall and overlay controls remain visible with the panel saved on either side.
A reader with no memory of #6443 has no reason to be told this, and it will age badly in a cross-platform guide. Suggest keeping the first sentence and dropping the second. (I checked — there is no#6443known-issue line anywhere underdocs/user/docsthat needs removing in exchange.)src/gui/PanadapterStack.cpp:360—window()->isVisible()is safe (QWidget::window()returns the widget itself when unparented and never null), but:1156in the same file writesif (auto* w = window()). Purely a local-consistency note; no fix needed.- The early return also skips the trailing
update()sweep at:381, which was previously the one unconditional side effect of this function. On a non-visible windowupdate()is a no-op, so this is harmless — flagging only so it is a deliberate choice rather than an accident. - PR body, not code: the before/after screenshots are hosted on the fork's
fix-6443-assetsbranch, which the body asks reviewers to keep alive. Once it is deleted the merged history has dead image links. Re-uploading them as GitHub drag-and-drop attachments would make them permanent.
What I tried to break
- Every sibling call site of
refreshAfterReparent(), for a behaviour change outside the targeted path. None. At:667,:1021and:1093the caller follows the helper with an explicitsw->show(), so the inner decision is moot. At:1164(dockPanadapter)sw->show()precedes the deferred call, soisHidden()is false and it shows — identical to the oldisVisible()==truebranch. On the float/dock paths thathide()the widget first (:1000-1009,:1066,:1116-1119), oldisVisible()==falseand newisHidden()==trueboth skip the innershow(), and the caller's ownshow()still runs. The only divergence between old and new is "not explicitly hidden but inside a hidden ancestor" — exactly the pre-show case #6443 reports. I specifically probed the comment at:1089("Defersw->show()until afterrefreshAfterReparent()") for an intent the innershow()would now violate: it does not, because that path hid the widget explicitly at:1066. - Whether the two halves are redundant — that is, whether the early return makes the
isHidden()change dead code. They are not.MainWindow.cpp:3997notestoggleMinimalMode(true)can emit ashowEventmid-construction; Qt sets the visible state before dispatchingshowEvent, so a dock change inside that window would not take the early return, and theisHidden()half is what carries it. In the other direction,isHidden()alone would still destroy and re-create a native Metal view on an unshown top level — cost with nothing to fix. Both are load-bearing, and either alone would have produced the body's "Fixed, saved LEFT →visible: true" reading, so that table cannot discriminate between them. - A dock toggle while the top level is hidden, post-fix. If the main window is
hide()-ed (not merely minimised — Qt keepsisVisible()true for a minimised window) and the dock side then changes, the native re-realize is now skipped and nothing re-runs it on the next show:grep showEventfinds no refresh hook in eitherMainWindoworPanadapterStack. So in principle the #6198 stale-strip symptom could surface on re-show. I could not find a reachable path other than startup (the bridge handlers atMainWindow.cpp:4779-4804and the menu path atMainWindow_Menus.cpp:1125all run with the window up), and pre-PR that same path blanked B1 permanently, so this is strictly an improvement. Naming it, not asking for a change. - The CodeGuard findings — all four refuted (security-audit (CodeGuard)).
PanadapterStack.cpp:685,:1278,:1283and:1308are eachfor (const QString& id : …); the& idbyte sequence is read as a shell background operator followed by the Unixidcommand. The file contains noQProcess,system(,popen,exec*orstartDetachedsink at all, and:1263's structurally identicalconst QString& panIdis not flagged, which confirms the lexical trigger. None of the four intersect either hunk — the nearest is 300+ lines away, and the scan clearly ran a whole-file sweep on the PR head. (The${PR_DIFF}tokens in the diff I was handed are prompt-side mangling of&&; the checkout has&&.) - Claims in the body I could check. "Windows/Linux rendering paths are unchanged" — confirmed by the
#ifplacement. CI is green on all seven checks at88a1ad9, includingcheck-macos, so the changed macOS branch does compile. I could not check: the macOS/Metal runtime behaviour, or the bridgeget rhireadings in the proof table. Every statement above is reasoned from the head checkout at/tmp/aetherclaude/pr-6453, not reproduced at runtime — the bridge run in the PR body is the only runtime evidence this fix has, and its before/after discrimination (visible: false→visible: trueon the same saved-LEFT profile) is the right shape for the claim.
Recommendation
Approve with nits. This is a four-line fix that matches the diagnosed root cause exactly, is confined to the macOS GPU branch, changes nothing at any live-window call site, and is backed by a real before/after on the reported configuration. The nits are cosmetic or coverage-boundary notes; none of them should hold the merge, and given #6443 blanks the primary panadapter in the default single-pan layout on shipped v26.10.2, with downgrading as the only workaround, the case for landing promptly is strong. Concrete next step for the author: trim the second sentence in panadapter-controls.md, and re-host the two screenshots as attachments so the merged body does not depend on a fork branch staying alive. Thanks for the careful write-up — stating plainly which builds and suites were not run made this much faster to review.
🤖 aethersdr-agent · cost: $8.0920 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (4 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $8.2264 · model: claude-opus-5 |
|
Landing pass at e3e6a92: the documentation and attachment nits are already addressed. All four CodeGuard alerts are false positives on |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Verified exact head e3e6a92 against main 709093e.
The macOS/GPU-only repair preserves explicit widget hide state at src/gui/PanadapterStack.cpp:37 and skips native-surface refresh before the top level is shown at :360. All helper callers were inspected, including float, dock and canvas moves; existing explicit hides and caller show ordering remain intact. No new engine coupling, settings keys, protocol or TX behavior enters the diff.
Earlier review dispositions:
- Documentation nit fixed by e3e6a92: panadapter-controls.md documents only dock-side persistence. The PR body now uses permanent GitHub attachments for both proof images.
- The four CodeGuard command-injection alerts at PanadapterStack.cpp:685, :1278, :1283 and :1308 are false positives: each points to a C++ range-for reference (
const QString& id). There is no QProcess, system, popen, exec or startDetached command sink in the file. None is in a changed hunk. - The three existing tests are neighboring coverage, not a regression test for this path. The direct macOS/Metal bridge assertion was independently run against both implementations instead; no workflow or architectural restructure was needed.
Local evidence on this head: app build passed with pinned Qt 6.12.0 and GPU spectrum enabled; native_widget_topology_test, floating_restore_policy_test and window_show_state_test passed 3/3 with QT_QPA_PLATFORM=offscreen. The four aetherd static gates passed on merge base and head with byte-identical findings.
Runtime proof on Apple M2 Ultra using fresh settings profiles, explicit owned sockets and DEMO-0001 only, with TX disabled: both saved LEFT and RIGHT startups returned panIndex 0, visible=true, rendererFailed=false, renderer="GPU QRhi (Metal; Apple M2 Ultra)". Dock LEFT/RIGHT and panel hide/show preserved that result. Captures showed the spectrum, waterfall, VFO and overlay controls. get transmit returned transmitting=false. Both owned clients were disconnected and stopped.
Negative check: temporarily restoring pre-fix PanadapterStack.cpp produced visible=false, renderer="QRhi initializing", 100x100 B1 and failed the same startup assertion. Exact PR source was restored and rebuilt before this review.
Freshness: exact-head pull_request CI was created 2026-10-14T02:29:44Z, after main's tip timestamp 2026-10-13T23:37:44Z. All four required contexts (build, check-windows, check-macos, Static checks) are successful; signatures are verified; merge-tree is clean. No unresolved review threads or CHANGES_REQUESTED reviews exist.
The model-invocable automated code-review pass was unavailable; this was a manual adversarial pass. No live-radio or RF validation occurred. Post-merge main CI remains the independent verification and will be checked after landing.
Fixes #6443.
On macOS with GPU spectrum enabled, restarting with the applet panel saved on the left explicitly hid the startup B1 spectrum widget. B1 stayed blank after connecting, while a later-created B2 worked. The same build started normally with the panel saved on the right.
buildUI()creates the placeholder B1 before restoring the saved dock side. That restore callsrefreshAfterLayoutShift()before the main window is shown.refreshAfterReparent()savedisVisible(), temporarily hid the spectrum, then skippedshow()because the ancestor window was still hidden. Qt retained the spectrum's explicit hide flag after the main window appeared.This change preserves
isHidden()across the temporary hide/show cycle, and skips the macOS native surface refresh while the stack's top-level window is hidden. Live dock changes retain their existing Metal refresh. The user guide documents dock-side persistence. Windows/Linux rendering paths are unchanged.Proof
The agent automation bridge exercised the actual app startup with fresh settings profiles and the built-in demo source. Each profile seeded
AppletPanelDockedLeftbefore launch; this was not an in-session dock change standing in for startup. TX automation was pinned off, and no live radio was connected.get rhireadingpanIndex: 0,visible: false,gpu: true,renderer: "QRhi initializing"visible: true,renderer: "GPU QRhi (Metal; Apple M3 Max)"visible: true,rendererFailed: false,renderer: "GPU QRhi (Metal; Apple M3 Max)",1260 × 732visible: true,rendererFailed: falserendererFailed: falseapplet stateverified the actual dock side and splitter ordering.dumpTreeandgrab pan-visible 0captured the blank baseline and rendered fixed spectrum, waterfall scale, VFO flag and overlay controls.get transmitconfirmedtransmitting: false. Each owned app was stopped and the shared radio lock released after the run. The agent automation bridge was sufficient; no gaps found.Before — saved-left startup on unchanged upstream/main:
After — the same saved-left startup on the fixed build:
Proof images are GitHub attachments, independent of the fork's branches, and are absent from this PR's diff.
Validation
native_widget_topology_test,floating_restore_policy_testandwindow_show_state_test: 3/3 passed. These are neighboring coverage; the before/after startup regression proof is the real macOS GPU bridge run above.npm ci && npm run buildpassed. Generated reference check, log-rule checks (707 checks), strict test registration and accessibility checks passed. Existing log-rule line-number drift was reported without rule failures.Regression introduced by #6198; its broader platform follow-up is #6224. No visual design, UX defaults, protocol or transmit behavior changes.
Generated with OpenAI Codex.