Repository navigation
fix(ai): show platform-correct modifier key in AI shortcut hints - #8430
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a platform-aware modifier helper and uses it in AI shortcut hint text. Updates Jest and Playwright coverage to assert the platform-specific modifier text, and adjusts a preferences fixture for the browser test. ChangesPlatform Modifier Key Utility and AI Shortcut Hints
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/components/AIAssist/index.js`:
- Line 224: The platform-specific shortcut hint in AIAssist now varies between ⌘
and Ctrl, but the existing tests do not cover either branch. Update the AIAssist
spec to assert the rendered hint text for both macOS and non-macOS cases, using
getPlatformModifierKey() behavior, and add matching coverage in the
AutocompletePane spec so both components validate the expected shortcut copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ab634ce-33cd-4fb6-bef3-59d70557592b
📒 Files selected for processing (3)
packages/bruno-app/src/components/AIAssist/index.jspackages/bruno-app/src/components/Preferences/AI/AutocompletePane.jspackages/bruno-app/src/utils/common/platform.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/bruno-app/src/components/Preferences/AI/AutocompletePane.spec.js (1)
51-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameterize the manual-trigger copy assertion.
Line 51 only exercises the
Ctrlbranch, so a macOS regression inOnly on ⌘+\would slip through. Mirror theit.eachpattern used above so both platform-specific strings are covered.Suggested change
- it('shows the platform modifier in the manual trigger description', () => { - renderPane('Ctrl'); - expect(screen.getByText('Only on Ctrl+\\')).toBeInTheDocument(); - }); + it.each([ + ['macOS', '⌘'], + ['non-macOS', 'Ctrl'] + ])('shows the %s modifier in the manual trigger description', (_platform, modifier) => { + renderPane(modifier); + expect(screen.getByText(`Only on ${modifier}+\\`)).toBeInTheDocument(); + });As per coding guidelines, “Add tests for any new functionality or meaningful changes” and “Cover both the ‘happy path’ and the realistically problematic paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/Preferences/AI/AutocompletePane.spec.js` around lines 51 - 54, The manual-trigger copy test in AutocompletePane.spec.js only covers the Ctrl case, so it can miss a macOS regression. Update the existing assertion around renderPane and screen.getByText to use the same it.each pattern as the other platform-dependent tests, covering both Ctrl and ⌘ variants of the manual-trigger description.Source: Coding guidelines
tests/preferences/ai/ai-shortcut-hints.spec.ts (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the manual-trigger hint in this flow too.
This spec only verifies the keymap row, but the same Preferences screen also changed the manual-trigger description. Adding that second visible assertion here keeps the E2E coverage aligned with the user-facing contract from this PR.
Suggested change
const keymap = page.locator('.autocomplete-keymap'); await expect(keymap).toBeVisible(); await expect(keymap.getByText(expectedModifier, { exact: true })).toBeVisible(); + await expect(page.getByText(`Only on ${expectedModifier}+\\`)).toBeVisible();As per coding guidelines, “Add tests for any new functionality or meaningful changes.” As per path instructions, E2E tests should verify user-visible behaviour and use multiple assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/preferences/ai/ai-shortcut-hints.spec.ts` around lines 13 - 15, The AI shortcut hints E2E test only checks the keymap row, so extend the existing assertion sequence in ai-shortcut-hints.spec.ts to also verify the manual-trigger hint is visible on the Preferences screen. Reuse the current test flow and add a second user-facing assertion alongside the existing locator checks, referencing the same page/locator setup so the coverage stays aligned with the changed manual-trigger description.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/bruno-app/src/components/Preferences/AI/AutocompletePane.spec.js`:
- Around line 51-54: The manual-trigger copy test in AutocompletePane.spec.js
only covers the Ctrl case, so it can miss a macOS regression. Update the
existing assertion around renderPane and screen.getByText to use the same
it.each pattern as the other platform-dependent tests, covering both Ctrl and ⌘
variants of the manual-trigger description.
In `@tests/preferences/ai/ai-shortcut-hints.spec.ts`:
- Around line 13-15: The AI shortcut hints E2E test only checks the keymap row,
so extend the existing assertion sequence in ai-shortcut-hints.spec.ts to also
verify the manual-trigger hint is visible on the Preferences screen. Reuse the
current test flow and add a second user-facing assertion alongside the existing
locator checks, referencing the same page/locator setup so the coverage stays
aligned with the changed manual-trigger description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dbbe231-c812-453b-a9a9-e71a17679823
📒 Files selected for processing (4)
packages/bruno-app/src/components/AIAssist/index.spec.jspackages/bruno-app/src/components/Preferences/AI/AutocompletePane.spec.jstests/preferences/ai/ai-shortcut-hints.spec.tstests/preferences/ai/init-user-data/preferences.json
✅ Files skipped from review due to trivial changes (1)
- tests/preferences/ai/init-user-data/preferences.json
utkarsh-bruno
left a comment
utkarsh-bruno
left a comment
There was a problem hiding this comment.
Looks good to me.
3 participants
Description
BRU3646
Contribution Checklist:
What is the Bug?
The AI feature keyboard shortcut hints always display the macOS Command symbol (⌘), even on Windows and Linux where the modifier key is Ctrl. This makes the hint text incorrect for non-macOS users.
Steps to Reproduce
Expected Behaviour
The hint shows the platform-appropriate modifier key:
Actual Behaviour
The hint hardcodes the macOS "⌘" symbol on all platforms, so Windows and Linux users see "⌘" instead of "Ctrl".
Summary by CodeRabbit
Bug Fixes
Tests