Fix desktop processing banner overlapping conversation details - #11901
Conversation
Failure-Class: none
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
undivisible
left a comment
There was a problem hiding this comment.
Request changes
The code/test/changelog scope is otherwise focused, but this diff adds a new production comment and rewrites the existing deferred-processing comment, contrary to the repository rule to preserve comments verbatim and add no new comments.
Please
- Remove the new
ConversationDetailProcessingLayoutcomment. - Restore the existing
deferredProcessingSectioncomment verbatim; leave the layout, test, and changelog changes intact.
Failure-Class: none
|
Addressed in
Verification: |
|
Thanks @aryanorastar — verified on
Review of the three changed files:
Desktop Swift CI is green on this head (Build & Tests, Static & Test Contracts, Release Compile, Hygiene). From my side the code is in good shape — leaving the final call with @undivisible to re-confirm the outstanding change request against the new head before merge. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
ty @aryanorastar this really bugged me @Archit-lal look he's picking up after you :)) |
undivisible
left a comment
There was a problem hiding this comment.
looks good. banner sits in the layout now so it can’t cover the chips, the height test would fail if this went back to an overlay, and the comment nits are gone.
|
Thanks, glad this helped. I’ll keep picking up these macOS UX issues where I can. @undivisible @Archit-lal |
Summary
Problem
While a deferred conversation was being enriched, the Processing conversation… banner was attached as an overlay to the entire Conversation Details card. SwiftUI overlays do not participate in their parent’s size calculation, so the banner occupied the same vertical region as metadata that was already available from the local conversation record. The source and duration chips were consequently rendered underneath the banner.
This was reproduced in the macOS desktop app while verifying a separate conversation-deletion report.
Root cause
ConversationDetailViewrenderedsummaryContentin the card’s normal layout flow, then painteddeferredProcessingSectionover the card with.overlay(alignment: .top). Both layers used the same outer spacing token, but the overlay reserved no height for itself and did not account for the card header.Fix
Introduce
ConversationDetailProcessingLayout, a small production layout seam that places the conditional banner before the existing summary content in oneVStack:OmiSpacing.xxlprovides the existing 24-point section separationVisual proof
Before — processing banner overlaps metadata
After — banner reserves space above metadata
The patched state was physically verified in the named bundle
/Applications/omi-processing-layout.app, including resizing the desktop window.Regression coverage
testProcessingBannerReservesSpaceAboveConversationMetadatarenders the production layout seam in idle and processing states and verifies that enabling processing increases the measured height by the banner height plusOmiSpacing.xxl. This would fail if the banner were changed back to an overlay or another non-layout decoration.Verification
./scripts/dev-feedback.py --once swift 'ConversationDetailAutomationStateTests'— 9 tests passed, 0 failures./scripts/swift-format-wrapper.sh lint Desktop/Sources/MainWindow/Pages/ConversationDetailView.swift Desktop/Tests/ConversationDetailAutomationStateTests.swift— passedpython3 ../../.github/scripts/desktop-changelog.py validate— passedgit diff --check— passedmake preflight— passedScope and risk
The change is limited to conversation-detail presentation during deferred enrichment. Completed conversations and the underlying enrichment, persistence, transcript, deletion, and metadata behavior are unchanged. Risk is low and covered by the focused layout test plus physical verification.
Failure-Class: none
No product invariant citations are required for the changed paths.