Repository navigation
Fix overlapping buttons in the wide permission dialog - #10
Conversation
NSAlert lays its buttons out in a horizontal row, which overlaps once the dialog is widened to fit a long command and the larger dialog font. Add a stackAlertButtonsVertically pass that reparents the buttons onto the content view, grows the window to make room, and lays them out as a single vertical column centered horizontally and vertically. Each button keeps its own fitted width so the layout survives the relayout NSAlert runs while presenting, and the existing reverse add-order keeps the top-to-bottom order matching the button list and the return-code mapping intact.
|
/gemini review |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new AppleScript handler stackAlertButtonsVertically to lay out dialog buttons in a single vertical column, preventing overlap when the dialog is widened. The review feedback highlights three key areas for improvement: robustly extracting the content view's height, defensively checking if buttonContainer is missing value before removing it from its superview to prevent runtime errors, and ignoring full-span background or visual effect views when calculating the content floor to ensure correct vertical centering on all macOS versions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| set theWindow to alert's |window|() | ||
| set contentView to theWindow's contentView() | ||
| set contentWidth to (item 1 of item 2 of (contentView's frame())) as real |
There was a problem hiding this comment.
To support robustly calculating the content floor and ignoring full-height background or visual effect views, we should extract the content view's height as well.
set theWindow to alert's |window|()
set contentView to theWindow's contentView()
set contentViewFrame to contentView's frame()
set contentWidth to (item 1 of item 2 of contentViewFrame) as real
set contentHeight to (item 2 of item 2 of contentViewFrame) as real
| set buttonContainer to (item 1 of buttonViews)'s superview() | ||
| if buttonContainer is not contentView then | ||
| repeat with theButton in buttonViews | ||
| theButton's removeFromSuperview() | ||
| contentView's addSubview:theButton | ||
| end repeat | ||
| buttonContainer's removeFromSuperview() | ||
| end if |
There was a problem hiding this comment.
If buttonContainer is missing value, calling removeFromSuperview() on it will result in a runtime error. We should defensively check that buttonContainer is not missing value before attempting to remove it.
set buttonContainer to (item 1 of buttonViews)'s superview()
if buttonContainer is not missing value and buttonContainer is not contentView then
repeat with theButton in buttonViews
theButton's removeFromSuperview()
contentView's addSubview:theButton
end repeat
buttonContainer's removeFromSuperview()
end if
| set contentFloor to missing value | ||
| repeat with childView in (contentView's subviews()) | ||
| if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then | ||
| set childBottom to (item 2 of item 1 of (childView's frame())) as real | ||
| if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
In some macOS versions, the content view contains full-span background or visual effect views that are not buttons. If we don't ignore them, contentFloor will be incorrectly set to 0, which breaks the vertical centering of the button column. We should ignore any subviews that span the full height of the content view.
set contentFloor to missing value
repeat with childView in (contentView's subviews())
if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then
set childFrame to childView's frame()
set childHeight to (item 2 of item 2 of childFrame) as real
if childHeight is greater than 1 and childHeight is less than (contentHeight - 5) then
set childBottom to (item 2 of item 1 of childFrame) as real
if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom
end if
end if
end repeat
There was a problem hiding this comment.
Code Review
This pull request introduces a new AppleScript handler, stackAlertButtonsVertically, to lay out NSAlert buttons in a single vertical column, preventing overlapping issues when the dialog is widened. The review feedback highlights two critical issues with this implementation: first, modifying the autoresizing mask of all subviews indiscriminately prevents the background container view from resizing, leading to visual defects; second, on modern macOS versions, the direct subviews of the content view are wrapped in an NSVisualEffectView, which breaks the coordinate calculations for centering the buttons. Code suggestions are provided to resolve both issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if extraHeight is greater than 0 then | ||
| repeat with childView in (contentView's subviews()) | ||
| childView's setAutoresizingMask:8 -- NSViewMinYMargin: stay anchored to the top | ||
| end repeat | ||
| set windowFrame to theWindow's frame() | ||
| set grownFrame to current application's NSMakeRect((item 1 of item 1 of windowFrame), ((item 2 of item 1 of windowFrame) - extraHeight), (item 1 of item 2 of windowFrame), ((item 2 of item 2 of windowFrame) + extraHeight)) | ||
| theWindow's setFrame:grownFrame display:false | ||
| end if |
There was a problem hiding this comment.
Overwriting the autoresizing mask of the background container view (such as NSVisualEffectView) to NSViewMinYMargin (8) prevents it from resizing vertically when the window grows. This causes a visual defect where the newly added bottom area of the dialog lacks the vibrant background effect.
We should only apply the NSViewMinYMargin mask to actual content views whose height is less than the content view's height, allowing the background view to resize and fill the window correctly.
if extraHeight is greater than 0 then
set contentHeight to (item 2 of item 2 of (contentView's frame())) as real
repeat with childView in (contentView's subviews())
set childHeight to (item 2 of item 2 of (childView's frame())) as real
if childHeight < contentHeight then
childView's setAutoresizingMask:8 -- NSViewMinYMargin: stay anchored to the top
end if
end repeat
set windowFrame to theWindow's frame()
set grownFrame to current application's NSMakeRect((item 1 of item 1 of windowFrame), ((item 2 of item 1 of windowFrame) - extraHeight), (item 1 of item 2 of windowFrame), ((item 2 of item 2 of windowFrame) + extraHeight))
theWindow's setFrame:grownFrame display:false
end if
| set contentFloor to missing value | ||
| repeat with childView in (contentView's subviews()) | ||
| if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then | ||
| set childBottom to (item 2 of item 1 of (childView's frame())) as real | ||
| if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
On modern macOS versions, NSAlert wraps its content views (text fields, icon, etc.) inside an NSVisualEffectView container. Because the current loop only inspects the direct subviews of contentView and does not traverse recursively, it will match the NSVisualEffectView (which is not an NSButton and has a bottom coordinate of 0). This sets contentFloor to 0, completely bypassing the vertical centering logic and pinning the buttons to the bottom.
To fix this, we should inspect the subviews of the NSVisualEffectView container if it exists, and convert the coordinates of the child views to the contentView coordinate space to find the true lowest content view.
set targetContainer to contentView
repeat with childView in (contentView's subviews())
if ((childView's isKindOfClass:(current application's NSVisualEffectView)) as boolean) then
set targetContainer to childView
exit repeat
end if
end repeat
set contentFloor to missing value
repeat with childView in (targetContainer's subviews())
if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then
set childRect to targetContainer's convertRect:(childView's frame()) toView:contentView
set childBottom to (item 2 of item 1 of childRect) as real
if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom
end if
end repeat
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- Guard buttonContainer against missing value before removeFromSuperview so a button with no superview can never trigger a runtime error. - Capture the content view height and skip any near-full-height background or visual-effect view when computing the content floor, so a full-span view (present on some macOS versions) cannot drag the floor to 0 and collapse the vertical centering. Hairline dividers are kept as a valid floor.
|
Round 1 review addressed in 96d55c2: Gemini (all 3, valid):
Codex: no findings (reacted on the trigger). Verified: |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AppleScript handler, stackAlertButtonsVertically, to lay out dialog buttons in a single vertical column to prevent overlapping issues when the dialog is widened. A critical runtime issue was identified in the new handler where alert's buttons() returns an NSArray, which will cause a runtime error when accessed with AppleScript list operations unless explicitly coerced to a native AppleScript list.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| -- packs buttons into a horizontal row by default, which overlaps once the dialog | ||
| -- is widened to fit a long message and the larger dialog font. Stacking top to | ||
| -- bottom removes that failure mode regardless of dialog width or button count. | ||
| set buttonViews to alert's buttons() |
There was a problem hiding this comment.
In AppleScript-ObjC, alert's buttons() returns an NSArray. Attempting to use AppleScript list operations (such as item 1 of buttonViews or item buttonIndex of buttonViews) directly on an NSArray without explicit coercion will result in a runtime error (e.g., Can’t get item 1 of ...).
Coerce the returned NSArray to a native AppleScript list to ensure that list indexing and counting work reliably.
set buttonViews to (alert's buttons()) as list
- Exclude near-full-height views from the top-pinning autoresize pass so a full-height background/visual-effect view keeps resizing and fills the grown window instead of leaving a blank strip. - When NSAlert nests the icon and text inside an NSVisualEffectView, descend into that container and measure the content floor in content-view coordinates, so the wrapper itself no longer collapses the floor to 0 and the column stays vertically centered.
|
Round 2 review addressed in b700d69: Gemini (both HIGH, valid — defensive cross-version hardening):
Note: on the target macOS (26) I verified by direct measurement that NSAlert does not wrap content in an Codex: no major issues ("Chef's kiss"). Verified: |
|
/gemini review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AppleScript handler, stackAlertButtonsVertically, to lay out NSAlert buttons in a single vertical column, preventing layout overlap issues when the dialog is widened. The review feedback identifies two important improvement opportunities: first, preserving existing autoresizing mask flags rather than overwriting them entirely with NSViewMinYMargin (8); and second, using a more precise check (containsObject:) to exclude only the alert's action buttons when calculating the content floor, rather than ignoring all NSButton instances.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| repeat with childView in (contentView's subviews()) | ||
| if ((item 2 of item 2 of (childView's frame())) as real) < (contentHeight - 5) then | ||
| childView's setAutoresizingMask:8 -- NSViewMinYMargin: stay anchored to the top | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
Overwriting the autoresizing mask entirely to 8 (NSViewMinYMargin) discards other critical resizing behaviors (such as NSViewWidthSizable / 2), which can cause layout issues or text truncation when the window is presented or resized. Instead, preserve the existing mask flags and only add the NSViewMinYMargin (8) flag if it is not already set.
repeat with childView in (contentView's subviews())
if ((item 2 of item 2 of (childView's frame())) as real) < (contentHeight - 5) then
set currentMask to childView's autoresizingMask() as integer
if (currentMask div 8) mod 2 is 0 then
childView's setAutoresizingMask:(currentMask + 8)
end if
end if
end repeat
| repeat with childView in (floorContainer's subviews()) | ||
| if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then | ||
| set childRect to floorContainer's convertRect:(childView's frame()) toView:contentView | ||
| if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then | ||
| set childBottom to (item 2 of item 1 of childRect) as real | ||
| if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom | ||
| end if | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
Skipping all NSButton views can lead to layout issues if the dialog contains other buttons (such as a help button or custom checkboxes in an accessory view), as they will be ignored when calculating the content floor. Checking if the view is one of the alert's actual buttons using containsObject: is much safer and more precise.
repeat with childView in (floorContainer's subviews())
if not (buttonViews's containsObject:childView) then
set childRect to floorContainer's convertRect:(childView's frame()) toView:contentView
if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then
set childBottom to (item 2 of item 1 of childRect) as real
if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom
end if
end if
end repeat
alert's buttons() returns an NSArray; coerce it to an AppleScript list so the count and item-index operations in stackAlertButtonsVertically are reliable across AppleScript-ObjC versions.
|
Round 3 review addressed in 18f8ca0: Gemini (CRITICAL, line 48 — adopted): coerced
Codex: no major issues on the prior commits ("Chef's kiss" / "Breezy!"). Verified: |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function stackAlertButtonsVertically in dialog.applescript to arrange alert buttons in a vertical column, preventing overlap in wider dialogs. The review feedback highlights a critical AppleScript-ObjC issue where loop variables in repeat with ... in ... blocks are treated as references rather than direct values. Failing to explicitly dereference these variables using contents of can cause runtime coercion errors or silent failures, particularly when passing them as arguments to Objective-C methods like addSubview:. Applying the suggested dereferencing across all five loops will ensure robust and crash-free execution across different macOS versions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| repeat with theButton in buttonViews | ||
| theButton's sizeToFit() | ||
| set bandRect to (theButton's superview())'s convertRect:(theButton's frame()) toView:contentView | ||
| set rectBottom to (item 2 of item 1 of bandRect) as real | ||
| set rectHeight to (item 2 of item 2 of bandRect) as real | ||
| set rectTop to rectBottom + rectHeight | ||
| if bandBottom is missing value or rectBottom < bandBottom then set bandBottom to rectBottom | ||
| if bandTop is missing value or rectTop > bandTop then set bandTop to rectTop | ||
| if rectHeight > buttonHeight then set buttonHeight to rectHeight | ||
| end repeat |
There was a problem hiding this comment.
In AppleScript-ObjC, when using a repeat with x in list loop, the loop variable x is an AppleScript reference (e.g., item 1 of list) rather than the direct value. While some target method calls can be resolved, passing these references directly as arguments or using them in complex expressions can sometimes lead to runtime coercion errors or crashes depending on the macOS version. It is safer and more robust to explicitly dereference the loop variable using contents of.
repeat with theButtonRef in buttonViews
set theButton to contents of theButtonRef
theButton's sizeToFit()
set bandRect to (theButton's superview())'s convertRect:(theButton's frame()) toView:contentView
set rectBottom to (item 2 of item 1 of bandRect) as real
set rectHeight to (item 2 of item 2 of bandRect) as real
set rectTop to rectBottom + rectHeight
if bandBottom is missing value or rectBottom < bandBottom then set bandBottom to rectBottom
if bandTop is missing value or rectTop > bandTop then set bandTop to rectTop
if rectHeight > buttonHeight then set buttonHeight to rectHeight
end repeat
| repeat with theButton in buttonViews | ||
| theButton's removeFromSuperview() | ||
| contentView's addSubview:theButton | ||
| end repeat |
There was a problem hiding this comment.
This is particularly critical here because theButton is passed directly as an argument to addSubview:. In AppleScript-ObjC, passing a loop reference variable directly as an argument to an Objective-C method can cause a runtime crash or fail silently because the bridge receives the reference wrapper instead of the actual NSView object. Explicitly dereferencing it using contents of ensures the actual NSButton object is passed.
repeat with theButtonRef in buttonViews
set theButton to contents of theButtonRef
theButton's removeFromSuperview()
contentView's addSubview:theButton
end repeat
| repeat with childView in (contentView's subviews()) | ||
| if ((item 2 of item 2 of (childView's frame())) as real) < (contentHeight - 5) then | ||
| childView's setAutoresizingMask:8 -- NSViewMinYMargin: stay anchored to the top | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
Explicitly dereference the loop variable childView using contents of to ensure robust behavior across different macOS versions.
repeat with childViewRef in (contentView's subviews())
set childView to contents of childViewRef
if ((item 2 of item 2 of (childView's frame())) as real) < (contentHeight - 5) then
childView's setAutoresizingMask:8 -- NSViewMinYMargin: stay anchored to the top
end if
end repeat
| repeat with childView in (contentView's subviews()) | ||
| if ((childView's isKindOfClass:(current application's NSVisualEffectView)) as boolean) then | ||
| set floorContainer to childView | ||
| exit repeat | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
Explicitly dereference the loop variable childView using contents of to ensure robust behavior across different macOS versions.
repeat with childViewRef in (contentView's subviews())
set childView to contents of childViewRef
if ((childView's isKindOfClass:(current application's NSVisualEffectView)) as boolean) then
set floorContainer to childView
exit repeat
end if
end repeat
| repeat with childView in (floorContainer's subviews()) | ||
| if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then | ||
| set childRect to floorContainer's convertRect:(childView's frame()) toView:contentView | ||
| if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then | ||
| set childBottom to (item 2 of item 1 of childRect) as real | ||
| if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom | ||
| end if | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
Explicitly dereference the loop variable childView using contents of to ensure robust behavior across different macOS versions.
repeat with childViewRef in (floorContainer's subviews())
set childView to contents of childViewRef
if not ((childView's isKindOfClass:(current application's NSButton)) as boolean) then
set childRect to floorContainer's convertRect:(childView's frame()) toView:contentView
if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then
set childBottom to (item 2 of item 1 of childRect) as real
if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom
end if
end if
end repeat
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- When anchoring content to the top, add the NSViewMinYMargin flag to each view's existing mask instead of overwriting it, so flags such as NSViewWidthSizable are retained. - Use the alert's buttons() NSArray with containsObject: to exclude only the alert's own action buttons from the content-floor scan, so any other buttons (e.g. a help button or accessory checkboxes) still count as content.
|
Round 4 review addressed in 57debdd: Gemini (both valid):
Codex: no major issues ("Breezy!"). Verified: |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function stackAlertButtonsVertically in dialog.applescript to arrange alert buttons in a single vertical column, preventing overlapping issues when the dialog is widened. The review feedback highlights a critical issue where comparing Objective-C objects using AppleScript's native is not operator is unreliable and could inadvertently remove the main content view; using isEqual: is recommended instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| -- Reparent the buttons directly onto the content view so they can be positioned | ||
| -- in absolute coordinates, then drop the now-empty native button container. | ||
| set buttonContainer to (item 1 of buttonViews)'s superview() | ||
| if buttonContainer is not missing value and buttonContainer is not contentView then |
There was a problem hiding this comment.
In AppleScriptObjC, comparing two Objective-C objects (such as NSView instances) using the native AppleScript is or is not operators is unreliable. AppleScriptObjC wraps Objective-C objects in AppleScript wrappers, and two different wrapper instances pointing to the same underlying Objective-C object may not be considered equal by AppleScript's identity operators.
If buttonContainer and contentView refer to the same underlying view, buttonContainer is not contentView can evaluate to true. This would lead to calling buttonContainer's removeFromSuperview(), which would remove the entire contentView from the window, resulting in a blank dialog.
To safely compare Objective-C objects for equality, use the isEqual: method.
if buttonContainer is not missing value and not (buttonContainer's isEqual:contentView) then
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- Compare the button container against the content view with isEqual: instead of the AppleScript identity operator, which is unreliable for ObjC object wrappers and could otherwise remove the content view and blank the dialog. - Dereference each repeat-loop variable with 'contents of' before using it, so the underlying NSView is passed to ObjC methods (notably addSubview:) rather than an AppleScript reference wrapper.
|
Round 5 review addressed in b9e62b4: Gemini (all valid — AppleScript-ObjC correctness):
Codex: no major issues on Verified end-to-end, including the stack-view reparent path (exercised only by wide dialogs, which is where |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a vertical button layout mechanism (stackAlertButtonsVertically) in the AppleScript dialog implementation to prevent button overlap when dialogs are widened. The reviewer suggests a more robust and safer calculation of the content floor to avoid potential overlaps with other subviews and prevent runtime exceptions by checking the class name as a string instead of using isKindOfClass:.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| set floorContainer to contentView | ||
| repeat with childViewRef in (contentView's subviews()) | ||
| set childView to contents of childViewRef | ||
| if ((childView's isKindOfClass:(current application's NSVisualEffectView)) as boolean) then | ||
| set floorContainer to childView | ||
| exit repeat | ||
| end if | ||
| end repeat | ||
| set contentFloor to missing value | ||
| repeat with childViewRef in (floorContainer's subviews()) | ||
| set childView to contents of childViewRef | ||
| if not ((buttonArray's containsObject:childView) as boolean) then | ||
| set childRect to floorContainer's convertRect:(childView's frame()) toView:contentView | ||
| if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then | ||
| set childBottom to (item 2 of item 1 of childRect) as real | ||
| if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom | ||
| end if | ||
| end if | ||
| end repeat |
There was a problem hiding this comment.
Improvement: Robust and Safe Content Floor Calculation
Currently, the script descends strictly into the NSVisualEffectView if found, which means any other direct subviews of contentView (such as the accessory view/spacer view, or potential future custom accessory views) are completely ignored when calculating the content floor. This can lead to buttons overlapping with those views.
Additionally, using isKindOfClass: with current application's NSVisualEffectView can potentially fail or throw a runtime exception if the class cannot be resolved dynamically in the AppleScript-ObjC context. Checking the class name as a string via className() is much safer.
We can refactor this to check both direct subviews of contentView and nested subviews of NSVisualEffectView in a single, robust pass.
set contentFloor to missing value
repeat with childViewRef in (contentView's subviews())
set childView to contents of childViewRef
if not ((buttonArray's containsObject:childView) as boolean) then
set childRect to childView's frame()
if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then
set childBottom to (item 2 of item 1 of childRect) as real
if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom
end if
if (childView's className() as text) is "NSVisualEffectView" then
repeat with nestedViewRef in (childView's subviews())
set nestedView to contents of nestedViewRef
set childRect to childView's convertRect:(nestedView's frame()) toView:contentView
if ((item 2 of item 2 of childRect) as real) < (contentHeight - 5) then
set childBottom to (item 2 of item 1 of childRect) as real
if contentFloor is missing value or childBottom < contentFloor then set contentFloor to childBottom
end if
end repeat
end if
end if
end repeat
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Gather the content view's own subviews together with any NSVisualEffectView container's subviews into a single candidate list, measuring each via its own superview, so a direct accessory view is no longer ignored when the icon/text are nested inside an effect view. Keeps isKindOfClass: for the container check (subclass-aware, unlike a className string match).
|
Round 6 review addressed in af6b2aa: Gemini (line 141, MEDIUM):
Codex: no major issues. Verified: |
Fix overlapping buttons in the wide permission dialog
Why
NSAlertlays its buttons out in a single horizontal row, which overlaps once the permission dialog is widened to fit a long command and the larger dialog font.How
stackAlertButtonsVerticallypass that runs after the existing width/font layout, just beforerunModal, and takes over button placement fromNSAlert.NSAlertperforms while presenting the dialog.What
src/agent_hooks/assets/dialog.applescript:stackAlertButtonsVerticallyhandler implementing the centered vertical column.on run.Verification
Allow Once,Always Allow,Deny).osacompileparses the script; all 25tests/test_transport.pytests pass.