feat: new selected list component for importing from git#7813
Open
prateek-bruno wants to merge 4 commits intousebruno:mainfrom
Open
feat: new selected list component for importing from git#7813prateek-bruno wants to merge 4 commits intousebruno:mainfrom
prateek-bruno wants to merge 4 commits intousebruno:mainfrom
Conversation
prateek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughAdds a reusable Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. 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: 3
🧹 Nitpick comments (1)
packages/bruno-app/src/components/SelectionList/index.js (1)
4-17: Add stable test IDs for the reusable checkboxes.This component owns the new import-selection controls, so Playwright tests need stable selectors; make the prefix configurable because some screens render multiple lists.
Suggested fix
const SelectionList = ({ title, items, selectedItems, onSelectAll, onItemToggle, getItemId, renderItemLabel, visibleRows = 8, rowHeight = 30, rowGap = 2, listPadding = 8, - emptyMessage = 'No items found' + emptyMessage = 'No items found', + testIdPrefix = 'selection-list' }) => {<input + data-testid={`${testIdPrefix}-select-all`} type="checkbox" checked={allSelected} onChange={onSelectAll} /><input + data-testid={`${testIdPrefix}-item-${itemId}`} type="checkbox" checked={isSelected} onChange={() => onItemToggle(itemId)} />As per coding guidelines, “Add
data-testidto testable elements for Playwright.”Also applies to: 29-34, 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/SelectionList/index.js` around lines 4 - 17, The SelectionList component should accept a configurable test id prefix (e.g., prop name testIdPrefix) and then apply stable data-testid attributes to the reusable checkbox elements: the master select-all checkbox (use something like `${testIdPrefix}-select-all-checkbox`) and each item checkbox and label (use `${testIdPrefix}-item-checkbox-${getItemId(item)}` and `${testIdPrefix}-item-label-${getItemId(item)}`). Update the component props list to include testIdPrefix (default to a sensible string), and add the corresponding data-testid attributes on the elements used in renderItemLabel/onItemToggle and the select-all control so Playwright can target them reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/SelectionList/index.js`:
- Line 18: The select-all boolean allSelected is currently based on array
lengths and can be true when selectedItems contains stale IDs; change its logic
in SelectionList to derive from actual item IDs by building a set of visible IDs
from items (e.g., items.map(i => i.id)) and then checking that every visible ID
is present in the selectedItems set (convert selectedItems to a Set for O(1)
lookups); ensure items.length === 0 yields false and account for missing id
fields defensively.
In `@packages/bruno-app/src/components/SelectionList/StyledWrapper.js`:
- Around line 44-56: The list currently inherits ul defaults and vertical
padding on .selection-item so getListHeight (used in max-height) undercounts
visible rows; make the list self-contained by resetting ul defaults on
.selection-list (e.g., list-style: none; margin: 0; padding: 0) and ensure each
row's height matches props.$rowHeight by removing vertical padding from
.selection-item and using either height: `${props.$rowHeight}px` or box-sizing
with explicit vertical padding accounted for in getListHeight; update references
in StyledWrapper to getListHeight, .selection-list, .selection-item,
props.$rowHeight and props.$listPadding accordingly.
In `@packages/bruno-app/src/components/Sidebar/CloneGitRespository/index.js`:
- Around line 346-355: The scanned collections are not being preselected because
selectedCollectionPaths stays empty after scan; update the place where scan
results are stored (where collectionPaths is assigned/updated) to also set
selectedCollectionPaths to the scanned collectionPaths so the SelectionList
(used with selectedCollectionPaths, getItemId, renderItemLabel) shows items
checked and enables the Open action immediately; ensure this update uses the
same state setter for selectedCollectionPaths as used by
handleSelectAllCollections/handleCollectionSelect so selection remains fully
controlled.
---
Nitpick comments:
In `@packages/bruno-app/src/components/SelectionList/index.js`:
- Around line 4-17: The SelectionList component should accept a configurable
test id prefix (e.g., prop name testIdPrefix) and then apply stable data-testid
attributes to the reusable checkbox elements: the master select-all checkbox
(use something like `${testIdPrefix}-select-all-checkbox`) and each item
checkbox and label (use `${testIdPrefix}-item-checkbox-${getItemId(item)}` and
`${testIdPrefix}-item-label-${getItemId(item)}`). Update the component props
list to include testIdPrefix (default to a sensible string), and add the
corresponding data-testid attributes on the elements used in
renderItemLabel/onItemToggle and the select-all control so Playwright can target
them reliably.
🪄 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: a6abb1d5-4487-42ca-96f9-5d6fe47b3fbe
📒 Files selected for processing (4)
packages/bruno-app/src/components/SelectionList/StyledWrapper.jspackages/bruno-app/src/components/SelectionList/index.jspackages/bruno-app/src/components/Sidebar/BulkImportCollectionLocation/index.jspackages/bruno-app/src/components/Sidebar/CloneGitRespository/index.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/import/bulk-import/003-selection-list-viewport.spec.ts (1)
10-10: Use stable test IDs for the SelectionList DOM hooks.This spec is coupled to styling classes (
.selection-list,.selection-item). Please expose/usedata-testidhooks onSelectionListand its rows so style refactors don’t break the e2e test.As per coding guidelines,
Add data-testid to testable elements for Playwright.Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/import/bulk-import/003-selection-list-viewport.spec.ts` at line 10, Test is fragile due to querying styling classes; update the SelectionList component to expose stable data-testid attributes (e.g., data-testid="selection-list" on the root and data-testid="selection-item" on each row) and change the spec to query those attributes instead of classes (replace node.querySelectorAll('.selection-item') with node.querySelectorAll('[data-testid="selection-item"]') and use '[data-testid="selection-list"]' for the root). Ensure the component adds those attributes in the render output for SelectionList and its row renderer so style refactors won't break tests.
🤖 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/import/bulk-import/003-selection-list-viewport.spec.ts`:
- Around line 36-42: The generated collection names and substring assertions are
not sort-safe; update the test data generation in
003-selection-list-viewport.spec.ts so names are zero-padded (e.g., Viewport
Collection 01..10) when creating files (the loop that builds
`filePath`/`fileContent`) and likewise in the later loop (lines ~67-81) so
sorting is stable, then change assertions against the
SelectionList/BulkImportCollectionLocation to assert the exact padded string (or
use a regex with word boundaries) instead of using toContain('Viewport
Collection 1') so "1" won't match "10"; reference `BulkImportCollectionLocation`
and `SelectionList` to locate where assertions need updating.
- Around line 77-82: The test's retry wrapper uses toPass() without an explicit
timeout; update the assertion block that calls toPass() (wrapping
getFullyVisibleRowNames(collectionList) and the expects) to pass a timeout
option of { timeout: 5000 } to toPass(), so the call becomes toPass({ timeout:
5000 }) and matches the codebase pattern for explicit retry timeouts.
---
Nitpick comments:
In `@tests/import/bulk-import/003-selection-list-viewport.spec.ts`:
- Line 10: Test is fragile due to querying styling classes; update the
SelectionList component to expose stable data-testid attributes (e.g.,
data-testid="selection-list" on the root and data-testid="selection-item" on
each row) and change the spec to query those attributes instead of classes
(replace node.querySelectorAll('.selection-item') with
node.querySelectorAll('[data-testid="selection-item"]') and use
'[data-testid="selection-list"]' for the root). Ensure the component adds those
attributes in the render output for SelectionList and its row renderer so style
refactors won't break tests.
🪄 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: 660960a4-1789-4ed9-84d8-9ae8711c3b0a
📒 Files selected for processing (1)
tests/import/bulk-import/003-selection-list-viewport.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/import/bulk-import/003-selection-list-viewport.spec.ts (1)
24-86: LGTM — viewport spec reads cleanly. 🐰Padded names, explicit
toPasstimeout, andtest.afterEachcleanup are all in place. Nice follow-through on the earlier review rounds.One tiny thought (take it or leave it): the bulk-import modal is left open when the test body ends, relying on
closeAllCollectionsto clean up. If you ever hit flakes on subsequent tests added to this describe, consider dismissing the dialog explicitly (e.g.,bulkImportModal.getByRole('button', { name: /cancel/i }).click()) beforeafterEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/import/bulk-import/003-selection-list-viewport.spec.ts` around lines 24 - 86, The test leaves the Bulk Import dialog open which may cause flakes; explicitly dismiss the dialog before the test ends by clicking its cancel/close button using the bulkImportModal handle (e.g., locate bulkImportModal.getByRole('button', { name: /cancel|close|dismiss/i }) and click it) so the modal is closed in-test instead of relying solely on test.afterEach/closeAllCollections to clean up.
🤖 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/import/bulk-import/003-selection-list-viewport.spec.ts`:
- Around line 24-86: The test leaves the Bulk Import dialog open which may cause
flakes; explicitly dismiss the dialog before the test ends by clicking its
cancel/close button using the bulkImportModal handle (e.g., locate
bulkImportModal.getByRole('button', { name: /cancel|close|dismiss/i }) and click
it) so the modal is closed in-test instead of relying solely on
test.afterEach/closeAllCollections to clean up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8729b99-6bd7-413f-a2ac-5e1cd21ce499
📒 Files selected for processing (1)
tests/import/bulk-import/003-selection-list-viewport.spec.ts
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
After:
Screen.Recording.2026-04-21.at.6.54.27.PM.mov
Before:
Screen.Recording.2026-04-21.at.6.53.15.PM.mov
Create a new component for selection list, use it for imports from collections. Used in git import, bulk import paths.
Will be used in github import later.
Contribution Checklist:
Summary by CodeRabbit
New Features
Style
Refactor
Tests