Add zoom controls to View menu with custom role menu item#48
Conversation
Releasing ⌘R (#44) replaced Menu::view() with a custom submenu that only kept devTools and fullscreen. That also dropped the viewMenu role's built-in zoom items, so ⌘+, ⌘-, and ⌘0 no longer did anything. NativePHP's RolesEnum doesn't expose zoomIn/zoomOut/resetZoom, but Electron accepts the role strings directly with their default accelerators attached. ZoomRoleMenuItem emits the raw role so we can reattach them to the custom View menu.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes introduce zoom role menu items (resetZoom, zoomIn, zoomOut) to the View submenu in the native app service provider, create a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Unit/Support/ZoomRoleMenuItemTest.php (2)
10-14: Consider tightening the positive assertion.
toMatchArrayonly checks the subset — extra/unexpected keys (e.g., a staleroleinherited from the parent, or unintended fields) would pass silently. If you want to lock the full payload down, switch totoEqualwith the exact shape, or add a key-count assertion. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Support/ZoomRoleMenuItemTest.php` around lines 10 - 14, The test currently uses toMatchArray on $array which only checks a subset and can miss extra/unexpected keys; update the assertion in ZoomRoleMenuItemTest (the expectation against $array) to assert the exact shape — either replace toMatchArray with toEqual using the full expected array ('type'=>'role','role'=>'zoomIn','label'=>'Zoom In') or add an explicit key-count/assertCount check to ensure no extra keys are present so the payload is fully locked down.
17-23: Optional: chain expectations on the same subject for readability.Per project guidelines, chain multiple expectations on the same subject when readable.
♻️ Suggested chain
test('omits label when none is provided', function () { $array = (new ZoomRoleMenuItem('resetZoom'))->toArray(); - expect($array['type'])->toBe('role'); - expect($array['role'])->toBe('resetZoom'); - expect($array)->not->toHaveKey('label'); + expect($array) + ->type->toBe('role') + ->role->toBe('resetZoom') + ->not->toHaveKey('label'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Support/ZoomRoleMenuItemTest.php` around lines 17 - 23, The test uses three separate expect(...) calls on the same $array subject; replace them with a chained assertion for readability — e.g. call expect($array)->toMatchArray(['type' => 'role', 'role' => 'resetZoom'])->not->toHaveKey('label') in the "omits label when none is provided" test that constructs the ZoomRoleMenuItem, so all checks target the same subject in one fluent chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Unit/Support/ZoomRoleMenuItemTest.php`:
- Around line 10-14: The test currently uses toMatchArray on $array which only
checks a subset and can miss extra/unexpected keys; update the assertion in
ZoomRoleMenuItemTest (the expectation against $array) to assert the exact shape
— either replace toMatchArray with toEqual using the full expected array
('type'=>'role','role'=>'zoomIn','label'=>'Zoom In') or add an explicit
key-count/assertCount check to ensure no extra keys are present so the payload
is fully locked down.
- Around line 17-23: The test uses three separate expect(...) calls on the same
$array subject; replace them with a chained assertion for readability — e.g.
call expect($array)->toMatchArray(['type' => 'role', 'role' =>
'resetZoom'])->not->toHaveKey('label') in the "omits label when none is
provided" test that constructs the ZoomRoleMenuItem, so all checks target the
same subject in one fluent chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9cd81700-831c-4538-ab76-6dd55799193d
📒 Files selected for processing (3)
app/Providers/NativeAppServiceProvider.phpapp/Support/ZoomRoleMenuItem.phptests/Unit/Support/ZoomRoleMenuItemTest.php
2 participants
Summary
This PR adds zoom in, zoom out, and reset zoom controls to the application's View menu. Since NativePHP's
RolesEnumdoesn't expose Electron's zoom roles, a newZoomRoleMenuItemclass was created to support these role-type menu items directly.Key Changes
ZoomRoleMenuItemclass: A custom menu item that extends NativePHP'sMenuItemto support Electron's zoom roles (zoomIn,zoomOut,resetZoom) with proper serializationZoomRoleMenuItemcorrectly serializes with and without labelsImplementation Details
The
ZoomRoleMenuItemclass bypasses NativePHP's limitations by directly setting thetypeto'role'and including the role string in the serialized output. This allows Electron to bind the standard zoom accelerators automatically. The zoom items are positioned at the top of the View menu for better UX, with separators organizing the menu logically.https://claude.ai/code/session_01XDyuWuYkRBcRS8MtLv7oQM
Summary by CodeRabbit