fix: remove agterm's full screen menu item, duplicated by AppKit's - #412
Merged
Conversation
AppKit appends its own "Enter Full Screen" (toggleFullScreen:, Globe+F) to the View menu as it is prepared for display, so agterm's item sat beside it as a visible duplicate. Nothing suppresses the injection: stripping the injected item on NSMenu.didBeginTrackingNotification runs before it, stripping it one runloop turn later changes only the model because the displayed menu is already snapshotted, NSFullScreenMenuItemEverywhere is ignored on macOS 26, and giving agterm's own item the toggleFullScreen: selector does not stop it either. Drop the item and let AppKit's be the only one. toggle_fullscreen keeps its rebindable chord by matching in CustomCommandRunner's key monitor, behind the same text-field and window guards as custom commands, so ctrl+cmd+f works without a menu item to carry it. The palette entry and window.fullscreen are unchanged. testViewMenuHasSingleFullScreenItem now matches on title and asserts the count: matching by identifier never found the injected item, which is why it passed while the duplicate was on screen.
macOS adds its own "Enter Full Screen" to the View menu and nothing suppresses it, so agterm ships none and handles the binding directly. Explain why the menu entry shows Globe+F while ctrl+cmd+f is the rebindable default.
Cover the other side of the leader guard: every existing test started from a non-armed engine, so `!commandEngine.isArmed` was never exercised false. A custom leader whose tail is the full screen chord must fire the command, and nothing pinned that. Unregister the test window from the process-global WindowRegistry: the id was minted inline and discarded, so tearDown could not undo the registration and each test method leaked a live window into the rest of the hosted bundle. Correct the surfaces the removal made stale or self-contradictory. windows.md listed "View > Toggle Full Screen" as a GUI surface three lines above the bullet saying agterm ships no such item. keymap.md, README, the site mirror, the skill reference and troubleshooting all named undo_close as the single action a menu item does not carry; toggle_fullscreen is now a second. Two godocs still pointed at the deleted item and at a chokepoint the chord no longer routes through.
There was a problem hiding this comment.
Pull request overview
Removes agterm’s own “Toggle Full Screen” View-menu item to avoid a persistent duplicate created by AppKit’s injected full-screen item, while keeping toggle_fullscreen rebindable by handling its chord via the existing key-monitor path in CustomCommandRunner. Documentation and test coverage are updated to reflect/lock in the new behavior and the menu/keymap-list implications.
Changes:
- Remove the app-defined View ▸ Toggle Full Screen item and rely on AppKit’s injected full-screen menu item to avoid duplicates.
- Route the
toggle_fullscreenchord throughCustomCommandRunner’s key monitor (respecting existing guards like text-field focus and leader sequencing). - Update docs and tests to reflect the new menu/keybinding behavior and to prevent regressions.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| site/docs.html | Documents that toggle_fullscreen has no dedicated menu item and updates keymap-list guidance. |
| site/commands.html | Updates window.fullscreen documentation to reference the system View-menu item wording. |
| README.md | Mirrors the docs note about toggle_fullscreen lacking its own menu item and updates keymap-list exceptions. |
| plugins/agterm/skills/agterm/troubleshooting.md | Updates troubleshooting guidance for keymap list exceptions to include toggle_fullscreen. |
| plugins/agterm/skills/agterm/reference.md | Updates reference text for window fullscreen and keymap/menu expectations. |
| agtermUITests/MenuUITests.swift | Strengthens the UI test to assert the View menu has exactly one “*Full Screen” item (system-injected only). |
| agtermTests/FullScreenChordTests.swift | Adds hosted unit tests covering toggle_fullscreen chord handling via the key monitor, including leader/text-field/repeat guards. |
| agterm/Control/ControlServer+WindowCommands.swift | Updates docstring for windowFullscreen to reflect the system menu item and shortcut handling. |
| agterm/Commands/CustomCommandRunner.swift | Adds explicit toggle_fullscreen chord handling before matcher advance; exposes a window-injected helper for tests. |
| agterm/AppDelegate.swift | Removes native full-screen menu-item stripping logic and keeps menu-tracking for chord reconciliation. |
| agterm/AppActions.swift | Updates documentation to reflect fullscreen toggling no longer routes through AppActions.toggleFullscreen() for the chord/control path. |
| agterm/agtermApp+Menus.swift | Removes the View-menu full-screen Button and documents the AppKit injection/duplication rationale. |
| .claude/rules/windows.md | Updates internal rules to describe the new fullscreen/menu behavior and test expectations. |
| .claude/rules/menu-actions.md | Updates internal menu/action guidance to match the new fullscreen routing. |
| .claude/rules/keymap.md | Updates internal keymap guidance to reflect toggle_fullscreen being monitor-driven and absent from menu-equivalent reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+94
to
+96
| /// The half above the key-window lookup, so a test can supply the window: a hosted test's own window | ||
| /// never becomes `NSApp.keyWindow` (the app is not active), which would make every guard here pass | ||
| /// vacuously. Internal for that reason alone, like `ControlServer.collectKeyEquivalents`. |
A nil key window returns at that guard, so the logic below never runs; the comment said the guards would pass vacuously instead.
Deploying agterm with
|
| Latest commit: |
07d8f5d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://492decc1.agterm.pages.dev |
| Branch Preview URL: | https://fix-fullscreen-menu-duplicat.agterm.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
the View menu showed two full screen items: agterm's own "Toggle Full Screen" and AppKit's "Enter Full Screen" right below it. Both also carried the same icon as Toggle Terminal Zoom.
AppKit appends its item as the View menu is prepared for display, and nothing stops it. Four mechanisms were tried and measured on macOS 26.5, all fail:
NSMenu.didBeginTrackingNotification- that fires before the injection. This is what the code did before, and it never worked; the test that pinned it matched by AX identifier, which never matches the injected item, so it passed while the duplicate was on screenNSFullScreenMenuItemEverywherefalse, the documented opt-out - ignored on macOS 26. Verified with the default registered and the item injected anywaytoggleFullScreen:selector so AppKit finds an equivalent - it injects past that tooso agterm now ships no full screen menu item and AppKit's is the only one.
toggle_fullscreenkeeps its rebindable ⌃⌘F by matching inCustomCommandRunner's existing key monitor, behind the same text-field and window guards custom commands use. A half-typed leader still outranks it. The palette entry andwindow.fullscreenare untouched, and Globe+F works because the item is the system's own.the visible trade: the menu entry reads "Enter/Exit Full Screen" and advertises Globe+F rather than your chord, since the shortcut no longer lives on that item. Documented in README and the site mirror.
second commit is a revmux review sweep: a test for the leader-sequence side of the new guard, an unregister for a window the test leaked into the process-global
WindowRegistry, and the doc surfaces the removal made stale -windows.mdwas asserting both that the menu item exists and that it does not, and five places still namedundo_closeas the only action no menu item carries.