Merged
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
Contributor
Reviewer's GuideAdds shared Playwright helpers and data setup utilities for bulk editing, wires new test IDs into Svelte components for reliable selection, and implements comprehensive bulk editor E2E regression suites for nodes, goals, tasks, and collections. Sequence diagram for Playwright bulk editor tests using test IDssequenceDiagram
actor Tester
participant PlaywrightTest
participant TestHelpers
participant BrowserPage
participant BulkEditBar
participant Button
Tester->>PlaywrightTest: run bulk editor regression suite
PlaywrightTest->>TestHelpers: openBulkEditorForEntityType(entityType)
TestHelpers->>BrowserPage: navigateToListView(entityType)
BrowserPage-->>TestHelpers: list view loaded
TestHelpers->>BrowserPage: selectMultipleRows()
TestHelpers->>BrowserPage: getByTestId(bulk-edit-bar)
BrowserPage-->>BulkEditBar: resolve element with data-testid=bulk-edit-bar
TestHelpers->>BulkEditBar: click()
PlaywrightTest->>TestHelpers: performBulkAction(action)
TestHelpers->>BrowserPage: getByRole(button, name=actionLabel)
BrowserPage-->>Button: match aria-label for icon-only action
TestHelpers->>Button: click()
Button-->>BrowserPage: apply bulk edit action
BrowserPage-->>PlaywrightTest: UI updated
PlaywrightTest-->>Tester: assertions passed
Class diagram for updated Svelte button and bulk edit componentsclassDiagram
class Button {
+boolean isUnderlined
+string id
+string testId
+string ariaLabel
+boolean isHovering
+string shortcut
}
class BulkEditBar {
+boolean isExpandedMode
+string dataTestId_bulk_edit_bar
+Button[] leftActionsButtons
+Button[] rightActionsButtons
}
class ResourceThumbnailContextMenu {
+string dataTestId_thumbnail_context_menu_trigger
}
BulkEditBar "1" o-- "many" Button : uses
ResourceThumbnailContextMenu "1" o-- "1" Button : trigger_uses
Button : +render(id)
Button : +renderWithTestId(testId)
Button : +renderWithAriaLabel(ariaLabel)
BulkEditBar : +renderBulkEditBar()
BulkEditBar : +renderActionButtons(isExpandedMode)
ResourceThumbnailContextMenu : +renderTriggerButton()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoComprehensive E2E tests for bulk editors with accessibility improvements
WalkthroughsDescription• Add comprehensive E2E tests for bulk editor across collections, goals, tasks, and nodes • Implement test helper functions for creating test data and selecting items • Add accessibility attributes (aria-label, data-testid) to Button and UI components • Replace placeholder tests with full functional test coverage Diagramflowchart LR
A["Button Component"] -->|Add aria-label & testId| B["Accessibility Attributes"]
C["BulkEditBar Component"] -->|Add data-testid| B
D["ContextMenu Component"] -->|Add data-testid| B
E["Test Helpers"] -->|Create test data & selection| F["E2E Test Suites"]
G["Collection Tests"] -->|Implement full coverage| F
H["Goal Tests"] -->|Implement full coverage| F
I["Task Tests"] -->|Implement full coverage| F
J["Node Tests"] -->|Implement full coverage| F
File Changes1. client/elements/button/Button.svelte
|
Code Review by Qodo
1.
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds and enables Playwright E2E tests for bulk-editor flows (collections, goals, tasks, nodes), introduces reusable test helpers for library/selection interactions, and adds data-testid/aria-label hooks to UI buttons and context-menu elements for test targeting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The test names that say "via drag" are using
selectFirstTwoViaContextMenuunder the hood; consider updating these descriptions or wiring in thedragSelectFirstTwoThumbnailshelper so the behavior matches the name. - The
createTwoCollections/Goals/Tasks/Nodeshelpers share a lot of repetitive structure (command, wait for input, fill withDate.now(), confirm); consider extracting a small shared utility to reduce duplication and make future changes to this flow easier. - Instead of passing an arbitrary
RegExpintoopenLibraryAndTab, you could narrow this tokeyof typeof LibraryTaband resolve the regex inside the function, which would prevent callers from accidentally using a pattern that doesn't match a real tab.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test names that say "via drag" are using `selectFirstTwoViaContextMenu` under the hood; consider updating these descriptions or wiring in the `dragSelectFirstTwoThumbnails` helper so the behavior matches the name.
- The `createTwoCollections/Goals/Tasks/Nodes` helpers share a lot of repetitive structure (command, wait for input, fill with `Date.now()`, confirm); consider extracting a small shared utility to reduce duplication and make future changes to this flow easier.
- Instead of passing an arbitrary `RegExp` into `openLibraryAndTab`, you could narrow this to `keyof typeof LibraryTab` and resolve the regex inside the function, which would prevent callers from accidentally using a pattern that doesn't match a real tab.
## Individual Comments
### Comment 1
<location path="apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts" line_range="29" />
<code_context>
});
- test.skip("bulk edit goals (select multiple, apply action)", async ({ page }) => {
+ test("select multiple goals via drag → bulk edit bar appears and shows count", async ({
+ page
+ }) => {
</code_context>
<issue_to_address>
**issue (testing):** Test name mentions drag selection but uses context-menu selection, which is misleading
The implementation uses `selectFirstTwoViaContextMenu`, so this test doesn’t actually exercise drag selection despite the name. The same mismatch appears in the analogous task/node/collection specs. Please either update these tests to use `dragSelectFirstTwoThumbnails` so they truly cover drag, or rename them to reflect context-menu selection and add separate tests for the drag flow.
</issue_to_address>
### Comment 2
<location path="apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts" line_range="57" />
<code_context>
+ page.getByText(/Selected: 2 collections?/i)
+ ).toBeVisible({ timeout: 10_000 });
+
+ await getBulkEditBar(page)
+ .getByRole("button", { name: /Clear selection/i })
+ .click({ timeout: 5_000 });
</code_context>
<issue_to_address>
**issue (testing):** Select-all test asserts a fixed count of 2 after selecting all, which may not reflect the real behaviour
In the `"select multiple nodes → Select all updates count to all items"` test, the final assertion still hardcodes `"Selected: 2 nodes"` after clicking "Select all". If there are more than two nodes, this no longer proves that "Select all" updated the count. Please assert on a dynamic value instead (e.g. a regex like `Selected: \d+ nodes?` and verify the count increased, or compute the expected total from the DOM and assert that exact value).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
|
|
||
| test.skip("bulk edit goals (select multiple, apply action)", async ({ page }) => { | ||
| test("select multiple goals via drag → bulk edit bar appears and shows count", async ({ |
Contributor
There was a problem hiding this comment.
issue (testing): Test name mentions drag selection but uses context-menu selection, which is misleading
The implementation uses selectFirstTwoViaContextMenu, so this test doesn’t actually exercise drag selection despite the name. The same mismatch appears in the analogous task/node/collection specs. Please either update these tests to use dragSelectFirstTwoThumbnails so they truly cover drag, or rename them to reflect context-menu selection and add separate tests for the drag flow.
| page.getByText(/Selected: 2 nodes?/i) | ||
| ).toBeVisible({ timeout: 10_000 }); | ||
|
|
||
| await getBulkEditBar(page) |
Contributor
There was a problem hiding this comment.
issue (testing): Select-all test asserts a fixed count of 2 after selecting all, which may not reflect the real behaviour
In the "select multiple nodes → Select all updates count to all items" test, the final assertion still hardcodes "Selected: 2 nodes" after clicking "Select all". If there are more than two nodes, this no longer proves that "Select all" updated the count. Please assert on a dynamic value instead (e.g. a regex like Selected: \d+ nodes? and verify the count increased, or compute the expected total from the DOM and assert that exact value).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts:40">
P2: This test claims to verify that "Select all updates count to all items," but since exactly 2 nodes are created and both are already selected before clicking "Select all," the assertion `Selected: 2 nodes` is identical before and after the click. The test doesn't actually prove "Select all" changed anything. Assert a dynamic total (e.g., count all thumbnails in the container) or create more than 2 nodes so the count visibly changes after "Select all."</violation>
<violation number="2" location="apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts:102">
P2: The starred/archived checks use global `toHaveCount(2)` assertions, which makes the test brittle when the account already contains matching collections. Verify the two newly created collections specifically (or assert a count delta) instead of absolute totals.</violation>
</file>
<file name="apps/e2e-playwright/tests/utils/helpers.ts">
<violation number="1" location="apps/e2e-playwright/tests/utils/helpers.ts:256">
P2: Scope thumbnail lookup to the provided container; the current global locator can target the wrong thumbnails when multiple lists exist.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts:40">
P2: The Select all test does not validate Select all behavior: it checks `Selected: 2` both before and after clicking, so regressions in the button logic can slip through.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts:37">
P2: This test title says it verifies drag selection, but the implementation uses context-menu selection. That leaves drag behavior untested while reporting it as covered.</violation>
<violation number="2" location="apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts:105">
P2: After `Select all`, asserting exactly `Selected: 2 tasks` is brittle. Verify that a selection count is shown (or that count increases) instead of hard-coding 2.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts:29">
P2: Test name says "via drag" but the implementation uses `selectFirstTwoViaContextMenu`, so drag selection is never actually exercised. Either rename the test to reflect context-menu selection or switch to `dragSelectFirstTwoThumbnails`. The same mismatch exists in the analogous collection, task, and node specs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
![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 (1)
apps/e2e-playwright/tests/utils/helpers.ts (1)
146-161: Consider replacing hardcoded delay with a condition-based wait.The
waitForTimeout(1_500)on line 160 is a hardcoded delay that can lead to flaky tests or unnecessarily slow execution. Consider waiting for a specific condition instead, such as the tab content becoming visible or a loading indicator disappearing.Example improvement
await page.getByRole("button", { name: tabName }).first().click({ timeout: 5_000 }); - await page.waitForTimeout(1_500); + // Wait for tab content to stabilize (e.g., first thumbnail or empty state) + await page.locator('div[id^="thumbnail-"], [data-testid="empty-state"]') + .first() + .waitFor({ state: "visible", timeout: 10_000 }) + .catch(() => null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/utils/helpers.ts` around lines 146 - 161, The hardcoded wait in openLibraryAndTab (the final await page.waitForTimeout(1_500)) should be replaced with a condition-based wait: after clicking the tab button (the page.getByRole(...).first().click call), wait for the tab’s content or a loading indicator to reach the expected state (e.g., use a locator for the tab panel or a known element/text in that tab and call .waitFor({ state: "visible" }) or waitForSelector that it no longer exists). Use the tabName parameter to derive a stable locator (for example look up a heading, role="region"/role="tabpanel", or a specific element unique to that tab) and wait on that locator instead of an arbitrary timeout so tests are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts`:
- Around line 155-163: Remove the inline comment "// Verify collections are
actually archived: enable archived filter via URL (toggle has no accessible
name)" from the test block; keep the code that constructs URL, sets
searchParams, navigates (url, url.searchParams.set, page.goto,
page.waitForTimeout) and assertions (thumbnails locator and expect calls)
unchanged, and if you need to preserve context, move that explanation into the
test name or a surrounding describe/comment block outside the modified lines.
- Around line 192-196: Remove the inline comment line that starts with "//
Verify the selected collections are removed: count after delete should be
countBefore - 2" in the test around the assertions using recordsContainer,
thumbnailsAfter, and countBefore; leave the await page.waitForTimeout(1_500);
and the expect(thumbnailsAfter).toHaveCount(...) assertion intact and ensure no
new inline comments are added in that block.
- Around line 95-108: Remove the inline explanatory comments in the test block
that begin "// Verify collections are actually starred: enable starred filter
via URL (toggle has no accessible name)" and "// Verify star icon appears on
each of the selected (now starred) collections"; the test code using URL,
url.searchParams, page.goto, thumbnails (locator "#records-container
div[id^='thumbnail-']"), and starIconsInStarredView already expresses the
intent, so delete those two comment lines and leave the code using url,
page.goto(...), thumbnails, and starIconsInStarredView unchanged.
- Around line 39-48: Remove the inline explanatory comments in the test block so
the assertions remain self-documenting: delete the comment lines preceding the
Selected text assertion and the block describing the bar (the lines that start
with "// Only collections are selected; bar shows count" and "// Bar (box in top
nav) has Star, Archive, Delete options") and keep the existing expectations
using page.getByText(/Selected: 2 collections?/i), getBulkEditBar(page), and
bar.getByRole(...) unchanged; this will satisfy the guideline against adding
inline comments while preserving test behavior.
In `@client/elements/button/Button.svelte`:
- Around line 41-44: Remove the inline documentation comments above the exported
props to satisfy coding guidelines: delete the comment lines preceding export
let testId and export let ariaLabel so only the prop declarations remain; ensure
no other inline comments are added around the testId or ariaLabel declarations
and keep their types/initializers unchanged in Button.svelte.
---
Nitpick comments:
In `@apps/e2e-playwright/tests/utils/helpers.ts`:
- Around line 146-161: The hardcoded wait in openLibraryAndTab (the final await
page.waitForTimeout(1_500)) should be replaced with a condition-based wait:
after clicking the tab button (the page.getByRole(...).first().click call), wait
for the tab’s content or a loading indicator to reach the expected state (e.g.,
use a locator for the tab panel or a known element/text in that tab and call
.waitFor({ state: "visible" }) or waitForSelector that it no longer exists). Use
the tabName parameter to derive a stable locator (for example look up a heading,
role="region"/role="tabpanel", or a specific element unique to that tab) and
wait on that locator instead of an arbitrary timeout so tests are deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16d4fa26-5476-422b-9945-8e570fb4206a
📒 Files selected for processing (8)
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.tsapps/e2e-playwright/tests/utils/helpers.tsclient/components/record/BulkEditBar.svelteclient/components/record/thumbnail/ResourceThumbnailContextMenu.svelteclient/elements/button/Button.svelte
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
Contributor
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts:140">
P2: The post-"Select all" assertion is too permissive and can pass even when the button has no effect, so this test no longer validates the select-all behavior.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts:106">
P2: This assertion is too broad and can pass without confirming that "Select all" changed the selection count.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts`:
- Around line 102-107: The test's assertion using page.getByText(/Selected: \d+
goals?/i) is too permissive; update the post-click check to assert the exact
total selected count (e.g., use expect(page.getByText("Selected: X
goals")).toBeVisible() or toHaveText("Selected: X goals")) after calling
getBulkEditBar(page).getByRole("button", { name: /Select all/i }).click(...),
and adjust the test setup so the dataset yields a total X greater than the
initial 2 (ensure fixtures or seed data used by this spec produce that larger
total) so the assertion will fail if Select all does not increase the count.
In `@apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts`:
- Around line 102-107: The test currently only checks visibility after clicking
the "Select all" button; change it to assert the exact selected count by first
ensuring there are more than the seeded two tasks (seed an extra task or create
one in the test) or programmatically derive the current thumbnail/task count
(using the page locator used for task thumbnails) and then click
getBulkEditBar(page).getByRole("button", { name: /Select all/i }).click(...) and
assert that the bulk bar text equals "Selected: X tasks" (or "task" when X===1)
where X is the derived thumbnail count; reference getBulkEditBar and the page
locator you use to count thumbnails when implementing this assertion.
In `@apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts`:
- Around line 102-107: Before clicking "Select all" capture the number of
visible node thumbnails using a locator (e.g., page.locator(...) or an existing
thumbnail role/selector) and store its count; then click the Select all button
via getBulkEditBar(page).getByRole("button", { name: /Select all/i }).click(...)
and assert the bulk bar text exactly equals the captured count (e.g., "Selected:
X nodes" or singular as appropriate) instead of just asserting some count is
visible; use the same getBulkEditBar(page) helper to read the displayed count
and compare it to the pre-click thumbnail count.
In `@apps/e2e-playwright/tests/utils/helpers.ts`:
- Line 317: Remove the inline comment "// 3-dots trigger: inner button
(ContextMenuAction) opens the menu; outer has data-testid" and fold its content
into the existing block comment above or incorporate it into the helper's
name/JSdoc for the relevant helper that deals with the 3-dots trigger (the
helper referencing ContextMenuAction / data-testid in helpers.ts); ensure no new
inline // comments are added and update the helper's block comment or name to
convey the same information.
- Around line 277-287: The marquee start coordinates can fall inside a thumbnail
when a card is close to the container edge; update the startX/startY logic
(where startX/startY are computed from containerBox, minX, minY) to guarantee
they are strictly outside the thumbnails by checking after the initial clamp
and, if startX >= minX set startX = minX - 5 (and similarly if startY >= minY
set startY = minY - 5), ensuring the starting point is left/above the first card
so the helper performs marquee selection rather than starting a drag/click.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80f34a30-d545-4783-b6eb-130cb64a5aab
📒 Files selected for processing (8)
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.tsapps/e2e-playwright/tests/utils/helpers.tsapps/nucleus/vite.config.tsclient/elements/button/Button.svelteclient/products/memotron/capture/CaptureTopBar.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- client/elements/button/Button.svelte
- apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Comment on lines
+102
to
+107
| await getBulkEditBar(page) | ||
| .getByRole("button", { name: /Select all/i }) | ||
| .click({ timeout: 5_000 }); | ||
| await expect(page.getByText(/Selected: \d+ goals?/i)).toBeVisible({ | ||
| timeout: 5_000 | ||
| }); |
Contributor
There was a problem hiding this comment.
Make the Select all check prove the count changed.
This matcher is too weak to validate the bulk action. page.getByText(/Selected: \d+ goals?/i) still passes if the selection stays at 2, so the regression suite would miss a broken Select all. Please assert the exact total item count after the click, and make sure the test data guarantees that total is greater than the initial 2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts` around lines
102 - 107, The test's assertion using page.getByText(/Selected: \d+ goals?/i) is
too permissive; update the post-click check to assert the exact total selected
count (e.g., use expect(page.getByText("Selected: X goals")).toBeVisible() or
toHaveText("Selected: X goals")) after calling
getBulkEditBar(page).getByRole("button", { name: /Select all/i }).click(...),
and adjust the test setup so the dataset yields a total X greater than the
initial 2 (ensure fixtures or seed data used by this spec produce that larger
total) so the assertion will fail if Select all does not increase the count.
apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+277
to
+287
| const minX = Math.min(box1.x, box2.x); | ||
| const minY = Math.min(box1.y, box2.y); | ||
| const maxRight = Math.max(box1.x + box1.width, box2.x + box2.width); | ||
| const maxBottom = Math.max(box1.y + box1.height, box2.y + box2.height); | ||
| const startX = Math.max(containerBox.x + 5, minX - 20); | ||
| const startY = Math.max(containerBox.y + 5, minY - 10); | ||
| const endX = maxRight + 20; | ||
| const endY = maxBottom + 15; | ||
| await page.mouse.move(startX, startY); | ||
| await page.mouse.down(); | ||
| await page.mouse.move(endX, endY, { steps: 20 }); |
Contributor
There was a problem hiding this comment.
Do not let marquee selection start on top of a thumbnail.
When the first card is close to the container edge, these startX / startY calculations can land inside the thumbnail instead of in empty space. In that case the helper stops exercising marquee selection and starts a card drag/click instead.
Suggested fix
- const startX = Math.max(containerBox.x + 5, minX - 20);
- const startY = Math.max(containerBox.y + 5, minY - 10);
+ const startX = minX - 20;
+ const startY = minY - 10;
+ if (startX < containerBox.x + 1 || startY < containerBox.y + 1) {
+ throw new Error(
+ `No empty gutter before the first thumbnails in #${containerId}; cannot start marquee selection outside a card`
+ );
+ }
const endX = maxRight + 20;
const endY = maxBottom + 15;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/utils/helpers.ts` around lines 277 - 287, The
marquee start coordinates can fall inside a thumbnail when a card is close to
the container edge; update the startX/startY logic (where startX/startY are
computed from containerBox, minX, minY) to guarantee they are strictly outside
the thumbnails by checking after the initial clamp and, if startX >= minX set
startX = minX - 5 (and similarly if startY >= minY set startY = minY - 5),
ensuring the starting point is left/above the first card so the helper performs
marquee selection rather than starting a drag/click.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts (1)
97-112:⚠️ Potential issue | 🟡 MinorMake
Select allfail when it does nothing.In a clean run this view can still contain only the two seeded nodes, which makes
totalCount === 2and turns this into a no-op assertion. RequiretotalCount > 2or create another node first, then assert the exact bulk-bar count.Suggested tightening
const container = page.locator("#records-container"); const thumbnails = container.locator('div[id^="thumbnail-"]'); await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); const totalCount = await thumbnails.count(); + expect(totalCount).toBeGreaterThan(2); await selectFirstTwoViaContextMenu(page, "records-container"); await expect( page.getByText(/Selected: 2 nodes?/i) ).toBeVisible({ timeout: 10_000 }); await getBulkEditBar(page) .getByRole("button", { name: /Select all/i }) .click({ timeout: 5_000 }); - await expect( - page.getByText(new RegExp(`Selected: ${totalCount} nodes?`, "i")) - ).toBeVisible({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toContainText(`Selected: ${totalCount}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts` around lines 97 - 112, The test's "Select all" assertion can be a no-op when totalCount === 2; modify the test around the thumbnails/totalCount logic so it guarantees more than two nodes before asserting the bulk selection: either assert totalCount > 2 and fail the test if not, or create an additional node (using the same page helpers) after computing totalCount and before calling selectFirstTwoViaContextMenu/getBulkEditBar so the subsequent expect(page.getByText(new RegExp(`Selected: ${totalCount} nodes?`, "i"))) is meaningful; update references to totalCount, thumbnails, selectFirstTwoViaContextMenu, and getBulkEditBar accordingly.apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts (1)
97-112:⚠️ Potential issue | 🟡 MinorMake
Select allprove the selection actually grew.This is still vacuous when the library only contains the two seeded tasks:
totalCountstays2, so the test passes even ifSelect allis a no-op. Fail fast unless there are more than two tasks, or seed an extra task before clicking, then assert against the bulk bar text directly.Suggested tightening
const container = page.locator("#task-library"); const thumbnails = container.locator('div[id^="thumbnail-"]'); await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); const totalCount = await thumbnails.count(); + expect(totalCount).toBeGreaterThan(2); await selectFirstTwoViaContextMenu(page, "task-library"); await expect( page.getByText(/Selected: 2 tasks?/i) ).toBeVisible({ timeout: 10_000 }); await getBulkEditBar(page) .getByRole("button", { name: /Select all/i }) .click({ timeout: 5_000 }); - await expect( - page.getByText(new RegExp(`Selected: ${totalCount} tasks?`, "i")) - ).toBeVisible({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toContainText(`Selected: ${totalCount}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts` around lines 97 - 112, The test checks that "Select all" increases the selection but can falsely pass when the library only has the two seeded tasks; change the test so it proves selection grew by either (a) asserting totalCount > 2 and failing fast if not, or (b) seeding an extra task before calling selectFirstTwoViaContextMenu and clicking the "Select all" button, then asserting the bulk bar text equals `Selected: ${totalCount} task(s)`; update the block around totalCount, selectFirstTwoViaContextMenu(page, "task-library"), and getBulkEditBar(...).getByRole("button", { name: /Select all/i }) to implement one of these approaches so the assertion verifies an actual increase in selection.apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts (1)
97-112:⚠️ Potential issue | 🟡 MinorMake
Select allprove the selection actually increased.If this tab only contains the two seeded goals,
totalCountis still2, so the test passes on a no-op. Require more than two goals in the view, or seed another one, and then verify the exact count on the bulk-edit bar.Suggested tightening
const container = page.locator("#records-container"); const thumbnails = container.locator('div[id^="thumbnail-"]'); await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); const totalCount = await thumbnails.count(); + expect(totalCount).toBeGreaterThan(2); await selectFirstTwoViaContextMenu(page, "records-container"); await expect( page.getByText(/Selected: 2 goals?/i) ).toBeVisible({ timeout: 10_000 }); await getBulkEditBar(page) .getByRole("button", { name: /Select all/i }) .click({ timeout: 5_000 }); - await expect( - page.getByText(new RegExp(`Selected: ${totalCount} goals?`, "i")) - ).toBeVisible({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toContainText(`Selected: ${totalCount}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts` around lines 97 - 112, The test currently can pass when there are only two goals because totalCount may equal 2; make the test assert that the view contains more than two items before using "Select all" and verify the bulk-edit bar shows the full totalCount after clicking it: after computing totalCount from thumbnails (variable totalCount and locator thumbnails) add an assertion like expect(totalCount).toBeGreaterThan(2) (or seed one more item if needed), then click Select all via getBulkEditBar().getByRole("button", { name: /Select all/i }) and assert the bulk-edit text exactly matches `Selected: ${totalCount} goals?` to ensure the selection actually increased (use the existing selectFirstTwoViaContextMenu and getBulkEditBar locators to locate elements).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts`:
- Around line 65-67: The test currently only waits for the old "Selected: 2
collections" label to disappear; update the assertions so they verify the entire
bulk-edit bar is hidden by asserting getBulkEditBar(page) is hidden after each
Clear selection / Star / Archive / Delete action (use
expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000 }) or similar). Replace
or add to the existing expect(page.getByText(/Selected: 2
collections?/i)).toBeHidden(...) checks (also update the other occurrences
referenced) so the test fails if the bar remains visible with a different
label/state.
- Around line 203-205: The test captures countBefore too early because
Locator.count() doesn't wait; before calling count() wait for thumbnails to be
rendered by awaiting the first thumbnail (e.g., await
thumbnailsBefore.first().waitFor()) or use
page.waitForSelector("#records-container div[id^='thumbnail-']") or an
expect(toHaveCount) for a known minimum, then call countBefore = await
thumbnailsBefore.count(); update the code around recordsContainer /
thumbnailsBefore / countBefore to perform that wait before snapshotting the
baseline.
- Around line 131-146: The test wrongly allows "Select all" to pass when only
the two seeded collections exist; update the test in bulk-editor.spec.ts to
ensure more than two items are present before clicking the "Select all" button
by either creating an additional collection (use existing helper that seeds
collections or call the create function) or asserting totalCount > 2 after
computing const totalCount = await thumbnails.count(); then click
getBulkEditBar(...).getByRole("button", { name: /Select all/i }).click(...) and
assert the bulk-edit bar shows Selected: ${totalCount} collections? exactly;
reference the variables container, thumbnails, totalCount and the helper
selectFirstTwoViaContextMenu/getBulkEditBar to locate where to add the extra
seed or the additional assertion.
In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts`:
- Around line 57-62: After clicking the "Clear selection" button and after the
"Star" action, assert that the bulk-edit bar element itself (obtained via
getBulkEditBar(page)) is hidden, not just that the "Selected: 2 goals" label
disappeared; update the assertions following the click calls to call something
like expect(getBulkEditBar(page)).toBeHidden({ timeout: 5000 }) so the test
verifies the entire bar is gone; do the same change for the second instance
around the Star action (the other assertion block at lines ~84-86) to ensure
getBulkEditBar(page) is hidden after that interaction as well.
In `@apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts`:
- Around line 57-62: After clicking the "Clear selection" button (and similarly
after "Mark as completed"), the test must assert that the entire bulk-edit bar
is hidden, not just that the "Selected: 2 tasks" text disappeared; update the
assertions to call getBulkEditBar(page) and expect(...).toBeHidden({ timeout:
5_000 }) immediately after the click for the Clear selection case and the Mark
as completed case (referencing getBulkEditBar(page) and the role button names
"Clear selection" and "Mark as completed" to locate the two places to change).
In `@apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts`:
- Around line 57-62: After clicking the "Clear selection" button, assert that
the bulk-edit bar element itself is hidden rather than only checking the text;
replace the text-based expectation with an assertion that getBulkEditBar(page)
is hidden (e.g., await expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000
})). Do the same for the "Star" action: after triggering the Star control where
the test currently checks for text changes (lines corresponding to the second
case), assert that getBulkEditBar(page) is hidden using getBulkEditBar(page) to
ensure the entire bar is gone.
---
Duplicate comments:
In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts`:
- Around line 97-112: The test currently can pass when there are only two goals
because totalCount may equal 2; make the test assert that the view contains more
than two items before using "Select all" and verify the bulk-edit bar shows the
full totalCount after clicking it: after computing totalCount from thumbnails
(variable totalCount and locator thumbnails) add an assertion like
expect(totalCount).toBeGreaterThan(2) (or seed one more item if needed), then
click Select all via getBulkEditBar().getByRole("button", { name: /Select all/i
}) and assert the bulk-edit text exactly matches `Selected: ${totalCount}
goals?` to ensure the selection actually increased (use the existing
selectFirstTwoViaContextMenu and getBulkEditBar locators to locate elements).
In `@apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts`:
- Around line 97-112: The test checks that "Select all" increases the selection
but can falsely pass when the library only has the two seeded tasks; change the
test so it proves selection grew by either (a) asserting totalCount > 2 and
failing fast if not, or (b) seeding an extra task before calling
selectFirstTwoViaContextMenu and clicking the "Select all" button, then
asserting the bulk bar text equals `Selected: ${totalCount} task(s)`; update the
block around totalCount, selectFirstTwoViaContextMenu(page, "task-library"), and
getBulkEditBar(...).getByRole("button", { name: /Select all/i }) to implement
one of these approaches so the assertion verifies an actual increase in
selection.
In `@apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts`:
- Around line 97-112: The test's "Select all" assertion can be a no-op when
totalCount === 2; modify the test around the thumbnails/totalCount logic so it
guarantees more than two nodes before asserting the bulk selection: either
assert totalCount > 2 and fail the test if not, or create an additional node
(using the same page helpers) after computing totalCount and before calling
selectFirstTwoViaContextMenu/getBulkEditBar so the subsequent
expect(page.getByText(new RegExp(`Selected: ${totalCount} nodes?`, "i"))) is
meaningful; update references to totalCount, thumbnails,
selectFirstTwoViaContextMenu, and getBulkEditBar accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4e1c679-d131-4895-b6b5-4ed1431c0baa
📒 Files selected for processing (5)
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.tsapps/e2e-playwright/tests/utils/helpers.ts
Comment on lines
+65
to
+67
| await expect( | ||
| page.getByText(/Selected: 2 collections?/i) | ||
| ).toBeHidden({ timeout: 5_000 }); |
Contributor
There was a problem hiding this comment.
These tests should assert the bar disappears, not only the old label.
Each of these paths only waits for Selected: 2 collections to disappear. If the bulk-edit bar remains visible with another state, the tests still pass. Assert getBulkEditBar(page) is hidden after Clear selection, Star, Archive, and Delete.
Also applies to: 103-105, 182-184, 218-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts` around lines
65 - 67, The test currently only waits for the old "Selected: 2 collections"
label to disappear; update the assertions so they verify the entire bulk-edit
bar is hidden by asserting getBulkEditBar(page) is hidden after each Clear
selection / Star / Archive / Delete action (use
expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000 }) or similar). Replace
or add to the existing expect(page.getByText(/Selected: 2
collections?/i)).toBeHidden(...) checks (also update the other occurrences
referenced) so the test fails if the bar remains visible with a different
label/state.
Comment on lines
+131
to
+146
| const container = page.locator("#records-container"); | ||
| const thumbnails = container.locator('div[id^="thumbnail-"]'); | ||
| await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); | ||
| const totalCount = await thumbnails.count(); | ||
|
|
||
| await selectFirstTwoViaContextMenu(page, "records-container"); | ||
| await expect( | ||
| page.getByText(/Selected: 2 collections?/i) | ||
| ).toBeVisible({ timeout: 10_000 }); | ||
|
|
||
| await getBulkEditBar(page) | ||
| .getByRole("button", { name: /Select all/i }) | ||
| .click({ timeout: 5_000 }); | ||
| await expect( | ||
| page.getByText(new RegExp(`Selected: ${totalCount} collections?`, "i")) | ||
| ).toBeVisible({ timeout: 5_000 }); |
Contributor
There was a problem hiding this comment.
Select all is still vacuous when only two collections exist.
In a clean library totalCount can still equal the two seeded collections, so this passes even when Select all changes nothing. Require more than two collections in the view, or create one more before the click, then assert the exact count on the bulk-edit bar.
Suggested tightening
const container = page.locator("#records-container");
const thumbnails = container.locator('div[id^="thumbnail-"]');
await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 });
const totalCount = await thumbnails.count();
+ expect(totalCount).toBeGreaterThan(2);
await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 collections?/i)
).toBeVisible({ timeout: 10_000 });
await getBulkEditBar(page)
.getByRole("button", { name: /Select all/i })
.click({ timeout: 5_000 });
- await expect(
- page.getByText(new RegExp(`Selected: ${totalCount} collections?`, "i"))
- ).toBeVisible({ timeout: 5_000 });
+ await expect(getBulkEditBar(page)).toContainText(`Selected: ${totalCount}`);📝 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
| const container = page.locator("#records-container"); | |
| const thumbnails = container.locator('div[id^="thumbnail-"]'); | |
| await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); | |
| const totalCount = await thumbnails.count(); | |
| await selectFirstTwoViaContextMenu(page, "records-container"); | |
| await expect( | |
| page.getByText(/Selected: 2 collections?/i) | |
| ).toBeVisible({ timeout: 10_000 }); | |
| await getBulkEditBar(page) | |
| .getByRole("button", { name: /Select all/i }) | |
| .click({ timeout: 5_000 }); | |
| await expect( | |
| page.getByText(new RegExp(`Selected: ${totalCount} collections?`, "i")) | |
| ).toBeVisible({ timeout: 5_000 }); | |
| const container = page.locator("#records-container"); | |
| const thumbnails = container.locator('div[id^="thumbnail-"]'); | |
| await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); | |
| const totalCount = await thumbnails.count(); | |
| expect(totalCount).toBeGreaterThan(2); | |
| await selectFirstTwoViaContextMenu(page, "records-container"); | |
| await expect( | |
| page.getByText(/Selected: 2 collections?/i) | |
| ).toBeVisible({ timeout: 10_000 }); | |
| await getBulkEditBar(page) | |
| .getByRole("button", { name: /Select all/i }) | |
| .click({ timeout: 5_000 }); | |
| await expect(getBulkEditBar(page)).toContainText(`Selected: ${totalCount}`); |
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 144-144: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(Selected: ${totalCount} collections?, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts` around lines
131 - 146, The test wrongly allows "Select all" to pass when only the two seeded
collections exist; update the test in bulk-editor.spec.ts to ensure more than
two items are present before clicking the "Select all" button by either creating
an additional collection (use existing helper that seeds collections or call the
create function) or asserting totalCount > 2 after computing const totalCount =
await thumbnails.count(); then click getBulkEditBar(...).getByRole("button", {
name: /Select all/i }).click(...) and assert the bulk-edit bar shows Selected:
${totalCount} collections? exactly; reference the variables container,
thumbnails, totalCount and the helper
selectFirstTwoViaContextMenu/getBulkEditBar to locate where to add the extra
seed or the additional assertion.
Comment on lines
+57
to
+62
| await getBulkEditBar(page) | ||
| .getByRole("button", { name: /Clear selection/i }) | ||
| .click({ timeout: 5_000 }); | ||
| await expect(page.getByText(/Selected: 2 goals?/i)).toBeHidden({ | ||
| timeout: 5_000 | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert the bulk-edit bar is hidden after the action.
These checks only prove the Selected: 2 goals label went away. If the bar remains visible with another state, the tests still pass. Assert getBulkEditBar(page) is hidden after Clear selection and after Star.
Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts` around lines
57 - 62, After clicking the "Clear selection" button and after the "Star"
action, assert that the bulk-edit bar element itself (obtained via
getBulkEditBar(page)) is hidden, not just that the "Selected: 2 goals" label
disappeared; update the assertions following the click calls to call something
like expect(getBulkEditBar(page)).toBeHidden({ timeout: 5000 }) so the test
verifies the entire bar is gone; do the same change for the second instance
around the Star action (the other assertion block at lines ~84-86) to ensure
getBulkEditBar(page) is hidden after that interaction as well.
Comment on lines
+57
to
+62
| await getBulkEditBar(page) | ||
| .getByRole("button", { name: /Clear selection/i }) | ||
| .click({ timeout: 5_000 }); | ||
| await expect(page.getByText(/Selected: 2 tasks?/i)).toBeHidden({ | ||
| timeout: 5_000 | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert the bulk-edit bar is hidden after the action.
Both paths only verify that Selected: 2 tasks disappeared. If the bar stays visible with a different state, these tests still pass. Assert getBulkEditBar(page) is hidden after Clear selection and after Mark as completed.
Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts` around lines
57 - 62, After clicking the "Clear selection" button (and similarly after "Mark
as completed"), the test must assert that the entire bulk-edit bar is hidden,
not just that the "Selected: 2 tasks" text disappeared; update the assertions to
call getBulkEditBar(page) and expect(...).toBeHidden({ timeout: 5_000 })
immediately after the click for the Clear selection case and the Mark as
completed case (referencing getBulkEditBar(page) and the role button names
"Clear selection" and "Mark as completed" to locate the two places to change).
Comment on lines
+57
to
+62
| await getBulkEditBar(page) | ||
| .getByRole("button", { name: /Clear selection/i }) | ||
| .click({ timeout: 5_000 }); | ||
| await expect(page.getByText(/Selected: 2 nodes?/i)).toBeHidden({ | ||
| timeout: 5_000 | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert the bulk-edit bar is hidden after the action.
These assertions only check that Selected: 2 nodes disappeared. If the bar stays visible with a different state, the regression still passes. Assert getBulkEditBar(page) is hidden after Clear selection and after Star.
Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts` around
lines 57 - 62, After clicking the "Clear selection" button, assert that the
bulk-edit bar element itself is hidden rather than only checking the text;
replace the text-based expectation with an assertion that getBulkEditBar(page)
is hidden (e.g., await expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000
})). Do the same for the "Star" action: after triggering the Star control where
the test currently checks for text changes (lines corresponding to the second
case), assert that getBulkEditBar(page) is hidden using getBulkEditBar(page) to
ensure the entire bar is gone.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts (2)
62-67:⚠️ Potential issue | 🟡 MinorAssert the bulk-edit bar is hidden, not just the old label.
Lines 65-67, 103-105, 182-184, and 220-222 only wait for
Selected: 2 collectionsto disappear. These paths still pass if the bar remains visible with a different state. AssertgetBulkEditBar(page)is hidden after Clear selection, Star, Archive, and Delete.Suggested tightening
await getBulkEditBar(page) .getByRole("button", { name: /Clear selection/i }) .click({ timeout: 5_000 }); - await expect( - page.getByText(/Selected: 2 collections?/i) - ).toBeHidden({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000 }); @@ await expect( page.getByText(/Starred 2 collections? successfully/i) ).toBeVisible({ timeout: 10_000 }); - await expect( - page.getByText(/Selected: 2 collections?/i) - ).toBeHidden({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000 }); @@ await expect( page.getByText(/Archived 2 collections? successfully/i) ).toBeVisible({ timeout: 10_000 }); - await expect( - page.getByText(/Selected: 2 collections?/i) - ).toBeHidden({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000 }); @@ await expect( page.getByText(/Deleted 2 collections? successfully/i) ).toBeVisible({ timeout: 10_000 }); - await expect( - page.getByText(/Selected: 2 collections?/i) - ).toBeHidden({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toBeHidden({ timeout: 5_000 });Also applies to: 103-105, 182-184, 220-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts` around lines 62 - 67, The tests currently only wait for the old label text to disappear after actions (Clear selection / Star / Archive / Delete); instead assert the entire bulk-edit UI is hidden. After invoking getBulkEditBar(page).getByRole(...).click(...) (or performing Star/Archive/Delete flows), replace the expect(page.getByText(/Selected: \d+ collections?/i)).toBeHidden(...) checks with expect(getBulkEditBar(page)).toBeHidden(...), so the assertion targets the bulk-edit container (getBulkEditBar(page)) being hidden rather than just the label text.
131-146:⚠️ Potential issue | 🟡 Minor
Select allis still vacuous when only two collections are present.Line 134 can still produce
totalCount === 2on a clean library, so this test passes even ifSelect allis a no-op. Require more than two visible collections before the click, then assert the count from the bulk-edit bar.Suggested tightening
const container = page.locator("#records-container"); const thumbnails = container.locator('div[id^="thumbnail-"]'); await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 }); const totalCount = await thumbnails.count(); + expect(totalCount).toBeGreaterThan(2); await selectFirstTwoViaContextMenu(page, "records-container"); await expect( page.getByText(/Selected: 2 collections?/i) ).toBeVisible({ timeout: 10_000 }); await getBulkEditBar(page) .getByRole("button", { name: /Select all/i }) .click({ timeout: 5_000 }); - await expect( - page.getByText(new RegExp(`Selected: ${totalCount} collections?`, "i")) - ).toBeVisible({ timeout: 5_000 }); + await expect(getBulkEditBar(page)).toContainText(`Selected: ${totalCount}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts` around lines 131 - 146, The test currently allows totalCount === 2 which masks a no-op "Select all"; update the test around the thumbnails/count logic so it requires more than two visible collections before exercising the Select all flow: after computing totalCount from container/thumbnails ensure totalCount > 2 (fail or skip the test if not) then call selectFirstTwoViaContextMenu(page, "records-container"), click the Select all button obtained via getBulkEditBar(page).getByRole("button", { name: /Select all/i }), and assert the bulk-edit bar shows "Selected: {totalCount} collection(s)" (use the existing RegExp matching) to verify Select all actually selects all items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts`:
- Around line 62-67: The tests currently only wait for the old label text to
disappear after actions (Clear selection / Star / Archive / Delete); instead
assert the entire bulk-edit UI is hidden. After invoking
getBulkEditBar(page).getByRole(...).click(...) (or performing
Star/Archive/Delete flows), replace the expect(page.getByText(/Selected: \d+
collections?/i)).toBeHidden(...) checks with
expect(getBulkEditBar(page)).toBeHidden(...), so the assertion targets the
bulk-edit container (getBulkEditBar(page)) being hidden rather than just the
label text.
- Around line 131-146: The test currently allows totalCount === 2 which masks a
no-op "Select all"; update the test around the thumbnails/count logic so it
requires more than two visible collections before exercising the Select all
flow: after computing totalCount from container/thumbnails ensure totalCount > 2
(fail or skip the test if not) then call selectFirstTwoViaContextMenu(page,
"records-container"), click the Select all button obtained via
getBulkEditBar(page).getByRole("button", { name: /Select all/i }), and assert
the bulk-edit bar shows "Selected: {totalCount} collection(s)" (use the existing
RegExp matching) to verify Select all actually selects all items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 015775fc-7d93-444b-8d81-5544a04602fa
📒 Files selected for processing (1)
apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
Member
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Merged
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
2 participants
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.
Summary by Sourcery
Add end-to-end coverage for bulk editing across library entities and expose test-friendly hooks on bulk edit UI components.
New Features:
Enhancements:
Tests:
Summary by cubic
Adds Playwright E2E coverage for the bulk editor across Collections, Goals, Tasks, and Nodes. Tests cover context-menu selection, Select all, bulk actions with success toasts, and clearing selection; adds stable test hooks, small accessibility tweaks, and allows
local.nucleus.toin dev. Addresses TIDY-432.New Features
LibraryTab, create sample data (including nodes via Capture), select the first two via context menu or drag, and target the bulk edit bar.Refactors
data-testid="bulk-edit-bar"anddata-testid="thumbnail-context-menu-trigger".Buttonnow supportsariaLabel/testId; BulkEditBar sets aria-labels on icon-only actions; Capture save usestestId="capture-save-button"withariaLabel="Save".local.nucleus.toinapps/nucleusdev server.Written for commit c8b0b40. Summary will update on new commits.
Summary by CodeRabbit
Tests
Accessibility / UI