fix(refactor): flaky shortcut test cases#7902
Open
shubh-bruno wants to merge 1 commit intousebruno:mainfrom
Open
fix(refactor): flaky shortcut test cases#7902shubh-bruno wants to merge 1 commit intousebruno:mainfrom
shubh-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughThe PR refactors keyboard shortcut test coverage by replacing a single 2013-line Playwright spec with five focused, dedicated test specs organized by feature area (developer tools, layout, requests, search, sidebar, tabs), improving test maintainability and organization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/shortcuts/bound-actions-developer-tools.spec.ts (1)
16-135: 🏗️ Heavy liftExtract the shared shortcut-test helpers into one utility.
This setup/remap helper block is effectively copy-pasted across the new
bound-actions-*.spec.tsfiles. Pulling it into a shared helper/fixture would make future shortcut changes much less error-prone and keep the specs aligned.Based on learnings: "Use consistent patterns and helper utilities where they improve clarity. Prefer shared test utilities over copy-pasted setup code, but only when it actually reduces complexity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shortcuts/bound-actions-developer-tools.spec.ts` around lines 16 - 135, The helper functions for shortcut tests (e.g., setupBoundActionsData, checkIfRequestExists, openRequest, openKeybindingsTab, closePreferencesTab, closeTabByName, openFolderSettingsTab, reopenClosedTab, remapKeybinding, getTabIndex) are duplicated across bound-actions-*.spec.ts files; extract them into a single shared test utility module (e.g., tests/helpers/shortcutHelpers.ts) and update the spec files to import and use these functions. Move all non-test-specific logic and fixtures into that module, export each function with the same names so callers need minimal changes, and ensure tests import the shared helpers and remove the copy-pasted blocks to keep behavior identical. Ensure any Page/fixture types are exported or imported so type checks continue to pass.tests/shortcuts/bound-actions-search.spec.ts (1)
163-166: ⚡ Quick winAssert that
Escapereally closes global search.Both tests send
Escapebut never verify the modal disappears, so a broken close handler would still pass here.Based on learnings: "Cover both the "happy path" and the realistically problematic paths. Validate expected success behaviour, but also validate error handling, edge cases, and degraded-mode behaviour when appropriate."Suggested fix
- await page.keyboard.down('Escape'); - await page.keyboard.up('Escape'); + await page.keyboard.press('Escape'); + await expect(page.getByTestId('global-search-input')).not.toBeVisible({ timeout: 2000 });Also applies to: 191-193
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/shortcuts/bound-actions-developer-tools.spec.ts`:
- Around line 180-184: Replace the hard sleep calls with locator-based waits:
after clicking the close button (e.g., page.getByTestId('session-close-1') or
'session-close-0'), wait for the corresponding session row/button locator to
become hidden/detached using Playwright's expect(...).not.toBeVisible or
locator.waitFor({ state: 'hidden' | 'detached' ) instead of
page.waitForTimeout(1000); do this for the occurrences around the session-close
clicks (the block using page.getByTestId('session-close-1') and the later block
at the other occurrence) so the test proceeds only after the UI element is
actually removed.
In `@tests/shortcuts/bound-actions-sidebar.spec.ts`:
- Around line 255-264: The failing assertion checks for 'req-1-rename' but the
test fills the input via requestNameInput with 'req-1-renamed' and clicks the
rename button via the 'rename-item-button' test id; update the assertion that
uses page.locator('.collection-item-name').filter({ hasText: ... }) to expect
'req-1-renamed' (matching the value filled) so the sidebar visibility check
aligns with the submitted rename.
- Around line 511-523: The test "customized Alt+D open clone item modal for
folder" assumes cloneItem was remapped to Alt+D by a previous test; instead,
explicitly remap the cloneItem keybinding to Alt+D at the start of this test
before sending Alt+D. Locate the helper or UI flow used elsewhere to change
shortcuts (the code that remaps cloneItem) and invoke it here (or
programmatically set the keybinding via the app API/UI) after opening the
collection and before issuing page.keyboard.down('Alt')/('KeyD'), so the test
does not depend on test order and will work in isolation.
In `@tests/shortcuts/bound-actions-tabs.spec.ts`:
- Around line 79-93: The helper reopenClosedTab uses fixed sleep waits causing
flakiness; replace the ad-hoc timeouts with deterministic waits on observable
locator state: before invoking shortcut() capture the current count or presence
of the target tab via page.locator('.request-tab').filter({ hasText:
expectedTabName }) (or its count), then call shortcut() and use expect(...) with
toHaveCount()/toBeVisible() and an appropriate timeout to await the tab
disappearance and reappearance (retrying by checking the count/state rather than
waiting 150/200ms). Ensure the loop uses these locator-based expectations to
break early when the tab is detected and only falls back to a final expect with
a longer timeout if all attempts fail.
- Around line 675-685: The >= assertions for moving the 'req-9' tab are too lax;
change them to require a single-step advance per shortcut press until the end by
computing maxIndex (query the current number of tabs, e.g., via a helper like
getTabsCount(page) or similar) and then assert each move equals
Math.min(prevIndex + 1, maxIndex); specifically replace the three
expect(...).toBeGreaterThanOrEqual(...) checks with expects that
indexAfterOneMove === Math.min(startIndex + 1, maxIndex), indexAfterTwoMoves ===
Math.min(indexAfterOneMove + 1, maxIndex), and indexAfterThreeMoves ===
Math.min(indexAfterTwoMoves + 1, maxIndex), using the existing getTabIndex(page,
'req-9') calls to obtain prevIndex.
---
Nitpick comments:
In `@tests/shortcuts/bound-actions-developer-tools.spec.ts`:
- Around line 16-135: The helper functions for shortcut tests (e.g.,
setupBoundActionsData, checkIfRequestExists, openRequest, openKeybindingsTab,
closePreferencesTab, closeTabByName, openFolderSettingsTab, reopenClosedTab,
remapKeybinding, getTabIndex) are duplicated across bound-actions-*.spec.ts
files; extract them into a single shared test utility module (e.g.,
tests/helpers/shortcutHelpers.ts) and update the spec files to import and use
these functions. Move all non-test-specific logic and fixtures into that module,
export each function with the same names so callers need minimal changes, and
ensure tests import the shared helpers and remove the copy-pasted blocks to keep
behavior identical. Ensure any Page/fixture types are exported or imported so
type checks continue to pass.
🪄 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: d2cbade5-7532-49a9-94e2-1748dcd7886b
📒 Files selected for processing (7)
tests/shortcuts/bound-actions-developer-tools.spec.tstests/shortcuts/bound-actions-layout.spec.tstests/shortcuts/bound-actions-requests.spec.tstests/shortcuts/bound-actions-search.spec.tstests/shortcuts/bound-actions-sidebar.spec.tstests/shortcuts/bound-actions-tabs.spec.tstests/shortcuts/bound-actions.spec.ts
💤 Files with no reviewable changes (1)
- tests/shortcuts/bound-actions.spec.ts
Comment on lines
+180
to
+184
| // Close all sessions with terminal tab | ||
| await page.getByTestId('session-close-1').click(); | ||
| await page.waitForTimeout(1000); | ||
| await page.getByTestId('session-close-0').click(); | ||
| await expect(page.getByTestId('session-close-0')).not.toBeVisible({ timeout: 3000 }); |
Contributor
There was a problem hiding this comment.
Replace these terminal-cleanup sleeps with locator-based waits.
Both waitForTimeout(1000) calls reintroduce the timing dependency this PR is trying to remove. Wait for the closed session row/button to disappear before moving to the next click.
As per coding guidelines "Try to reduce usage of page.waitForTimeout(); in code unless absolutely necessary and the locator cannot be found using existing expect() playwright calls" and "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
Also applies to: 235-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/bound-actions-developer-tools.spec.ts` around lines 180 -
184, Replace the hard sleep calls with locator-based waits: after clicking the
close button (e.g., page.getByTestId('session-close-1') or 'session-close-0'),
wait for the corresponding session row/button locator to become hidden/detached
using Playwright's expect(...).not.toBeVisible or locator.waitFor({ state:
'hidden' | 'detached' ) instead of page.waitForTimeout(1000); do this for the
occurrences around the session-close clicks (the block using
page.getByTestId('session-close-1') and the later block at the other occurrence)
so the test proceeds only after the UI element is actually removed.
Comment on lines
+255
to
+264
| // Fill in the rename req name | ||
| const requestNameInput = page.locator('#collection-item-name'); | ||
| await requestNameInput.fill('req-1-renamed'); | ||
|
|
||
| // Click the rename button | ||
| await page.getByTestId('rename-item-button').click(); | ||
|
|
||
| // Verify renamed request appears in sidebar | ||
| // await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1' })).toBeVisible({ timeout: 2000 }); | ||
| await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1-rename' })).toBeVisible({ timeout: 2000 }); |
Contributor
There was a problem hiding this comment.
Assert the name this test actually submits.
The modal is filled with req-1-renamed, but the assertion waits for req-1-rename, so this fails even when the rename flow works.
Suggested fix
- await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1-rename' })).toBeVisible({ timeout: 2000 });
+ await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1-renamed' })).toBeVisible({ timeout: 2000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fill in the rename req name | |
| const requestNameInput = page.locator('#collection-item-name'); | |
| await requestNameInput.fill('req-1-renamed'); | |
| // Click the rename button | |
| await page.getByTestId('rename-item-button').click(); | |
| // Verify renamed request appears in sidebar | |
| // await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1' })).toBeVisible({ timeout: 2000 }); | |
| await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1-rename' })).toBeVisible({ timeout: 2000 }); | |
| // Fill in the rename req name | |
| const requestNameInput = page.locator('#collection-item-name'); | |
| await requestNameInput.fill('req-1-renamed'); | |
| // Click the rename button | |
| await page.getByTestId('rename-item-button').click(); | |
| // Verify renamed request appears in sidebar | |
| // await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1' })).toBeVisible({ timeout: 2000 }); | |
| await expect(page.locator('.collection-item-name').filter({ hasText: 'req-1-renamed' })).toBeVisible({ timeout: 2000 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/bound-actions-sidebar.spec.ts` around lines 255 - 264, The
failing assertion checks for 'req-1-rename' but the test fills the input via
requestNameInput with 'req-1-renamed' and clicks the rename button via the
'rename-item-button' test id; update the assertion that uses
page.locator('.collection-item-name').filter({ hasText: ... }) to expect
'req-1-renamed' (matching the value filled) so the sidebar visibility check
aligns with the submitted rename.
Comment on lines
+511
to
+523
| test('customized Alt+D open clone item modal for folder', async ({ page, createTmpDir }) => { | ||
| await page.keyboard.down('Alt'); | ||
| await page.keyboard.down('KeyY'); | ||
| await page.keyboard.up('KeyY'); | ||
| await page.keyboard.up('Alt'); | ||
|
|
||
| await createFolder(page, 'kb-folder-clone-src', collectionName, true); | ||
| await openCollection(page, collectionName); | ||
| await page.locator('.collection-item-name').filter({ hasText: 'kb-folder-clone-src' }).first().click(); | ||
| await page.keyboard.down('Alt'); | ||
| await page.keyboard.down('KeyD'); | ||
| await page.keyboard.up('KeyD'); | ||
| await page.keyboard.up('Alt'); |
Contributor
There was a problem hiding this comment.
This test currently depends on a previous test's keybinding remap.
cloneItem still defaults to Cmd/Ctrl+D, but this case never remaps it before sending Alt+D. That makes the test order-dependent and it will fail in isolation or after any reset.
Suggested fix
test('customized Alt+D open clone item modal for folder', async ({ page, createTmpDir }) => {
await page.keyboard.down('Alt');
await page.keyboard.down('KeyY');
await page.keyboard.up('KeyY');
await page.keyboard.up('Alt');
+ await remapKeybinding(page, 'cloneItem', async () => {
+ await page.keyboard.press('Alt+KeyD');
+ });
+
await createFolder(page, 'kb-folder-clone-src', collectionName, true);
await openCollection(page, collectionName);
await page.locator('.collection-item-name').filter({ hasText: 'kb-folder-clone-src' }).first().click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/bound-actions-sidebar.spec.ts` around lines 511 - 523, The
test "customized Alt+D open clone item modal for folder" assumes cloneItem was
remapped to Alt+D by a previous test; instead, explicitly remap the cloneItem
keybinding to Alt+D at the start of this test before sending Alt+D. Locate the
helper or UI flow used elsewhere to change shortcuts (the code that remaps
cloneItem) and invoke it here (or programmatically set the keybinding via the
app API/UI) after opening the collection and before issuing
page.keyboard.down('Alt')/('KeyD'), so the test does not depend on test order
and will work in isolation.
Comment on lines
+79
to
+93
| const reopenClosedTab = async (page: Page, shortcut: () => Promise<void>, expectedTabName: string | RegExp) => { | ||
| for (let attempt = 0; attempt < 3; attempt++) { | ||
| await page.locator('.request-tab').first().click(); | ||
| await page.waitForTimeout(150); | ||
| await shortcut(); | ||
| const reopenedTab = page.locator('.request-tab').filter({ hasText: expectedTabName }); | ||
| if ((await reopenedTab.count()) > 0) { | ||
| await expect(reopenedTab).toBeVisible({ timeout: 3000 }); | ||
| return; | ||
| } | ||
| await page.waitForTimeout(200); | ||
| } | ||
|
|
||
| await expect(page.locator('.request-tab').filter({ hasText: expectedTabName })).toBeVisible({ timeout: 5000 }); | ||
| }; |
Contributor
There was a problem hiding this comment.
reopenClosedTab() is still sleep-driven.
This helper retries based on fixed 150/200ms sleeps instead of an observable tab state, so it can still flap under slower CI runs.
As per coding guidelines "Try to reduce usage of page.waitForTimeout(); in code unless absolutely necessary and the locator cannot be found using existing expect() playwright calls" and "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/bound-actions-tabs.spec.ts` around lines 79 - 93, The helper
reopenClosedTab uses fixed sleep waits causing flakiness; replace the ad-hoc
timeouts with deterministic waits on observable locator state: before invoking
shortcut() capture the current count or presence of the target tab via
page.locator('.request-tab').filter({ hasText: expectedTabName }) (or its
count), then call shortcut() and use expect(...) with
toHaveCount()/toBeVisible() and an appropriate timeout to await the tab
disappearance and reappearance (retrying by checking the count/state rather than
waiting 150/200ms). Ensure the loop uses these locator-based expectations to
break early when the tab is detected and only falls back to a final expect with
a longer timeout if all attempts fail.
Comment on lines
+675
to
+685
| await page.keyboard.press(`${modifier}+BracketRight`); | ||
| const indexAfterOneMove = await getTabIndex(page, 'req-9'); | ||
| expect(indexAfterOneMove).toBeGreaterThanOrEqual(startIndex); | ||
|
|
||
| await page.keyboard.press(`${modifier}+BracketRight`); | ||
| const indexAfterTwoMoves = await getTabIndex(page, 'req-9'); | ||
| expect(indexAfterTwoMoves).toBeGreaterThanOrEqual(indexAfterOneMove); | ||
|
|
||
| await page.keyboard.press(`${modifier}+BracketRight`); | ||
| const indexAfterThreeMoves = await getTabIndex(page, 'req-9'); | ||
| expect(indexAfterThreeMoves).toBeGreaterThanOrEqual(indexAfterTwoMoves); |
Contributor
There was a problem hiding this comment.
Tighten the move-right assertions so the shortcut has to move the tab.
The current >= checks can pass even when a key press stops moving the tab. For this setup, each press should advance the active tab by exactly one position until it reaches the end.
Suggested fix
- expect(indexAfterOneMove).toBeGreaterThanOrEqual(startIndex);
+ expect(indexAfterOneMove).toBe(startIndex + 1);
- expect(indexAfterTwoMoves).toBeGreaterThanOrEqual(indexAfterOneMove);
+ expect(indexAfterTwoMoves).toBe(indexAfterOneMove + 1);
- expect(indexAfterThreeMoves).toBeGreaterThanOrEqual(indexAfterTwoMoves);
+ expect(indexAfterThreeMoves).toBe(indexAfterTwoMoves + 1);Also applies to: 723-740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/shortcuts/bound-actions-tabs.spec.ts` around lines 675 - 685, The >=
assertions for moving the 'req-9' tab are too lax; change them to require a
single-step advance per shortcut press until the end by computing maxIndex
(query the current number of tabs, e.g., via a helper like getTabsCount(page) or
similar) and then assert each move equals Math.min(prevIndex + 1, maxIndex);
specifically replace the three expect(...).toBeGreaterThanOrEqual(...) checks
with expects that indexAfterOneMove === Math.min(startIndex + 1, maxIndex),
indexAfterTwoMoves === Math.min(indexAfterOneMove + 1, maxIndex), and
indexAfterThreeMoves === Math.min(indexAfterTwoMoves + 1, maxIndex), using the
existing getTabIndex(page, 'req-9') calls to obtain prevIndex.
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
1 participant
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.
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit