Skip to content

TIDY-432 #520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thyaravind merged 5 commits into from
Mar 14, 2026
Merged

TIDY-432 #520

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3320b48
Comprehensive tests for bulk editors
Sriya-Mukkawar Mar 10, 2026
54f2ed5
Review fixes
Sriya-Mukkawar Mar 10, 2026
d2464e9
Review agents issues fixed
Sriya-Mukkawar Mar 11, 2026
8b2a8ce
#2 review issues fix
Sriya-Mukkawar Mar 11, 2026
c8b0b40
Merge branch 'dev' into TIDY-432
thyaravind Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
221 changes: 221 additions & 0 deletions apps/e2e-playwright/tests/shared/collection/bulk-editor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
import { test, expect } from "@playwright/test";
import {
ensureInAppOnHome,
openLibraryAndTab,
LibraryTab,
selectFirstTwoViaContextMenu,
createTwoCollections,
getBulkEditBar
} from "../../utils/helpers";

const runtimeEnv = (
globalThis as { process?: { env?: Record<string, string | undefined> } }
).process?.env;

test.skip(
runtimeEnv?.SKIP_E2E === "1",
"E2E suite disabled by environment"
);

test.describe("collection - bulk editor @regression", () => {
test.beforeEach(async ({ page }) => {
await page.route("**/*", (route) => {
const reqUrl = route.request().url();
if (/accounts\.google\.com/i.test(reqUrl)) route.abort();
else route.continue();
});
});

test("select multiple collections via context menu - bulk edit bar appears and shows count", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoCollections(page);
await openLibraryAndTab(page, LibraryTab.Collections);

await selectFirstTwoViaContextMenu(page, "records-container");

await expect(
page.getByText(/Selected: 2 collections?/i)
).toBeVisible({ timeout: 10_000 });
const bar = getBulkEditBar(page);
await expect(bar).toBeVisible({ timeout: 5_000 });
await expect(bar.getByRole("button", { name: /^Star$/i })).toBeVisible();
await expect(bar.getByRole("button", { name: /^Archive$/i })).toBeVisible();
await expect(bar.getByRole("button", { name: /^Delete$/i })).toBeVisible();
});

test("select multiple collections - clear selection hides bulk edit bar", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoCollections(page);
await openLibraryAndTab(page, LibraryTab.Collections);

await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 collections?/i)
).toBeVisible({ timeout: 10_000 });

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 });
Comment on lines +65 to +67
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

});

test("select multiple collections - Star shows success toast, clears selection, and collections are starred", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoCollections(page);
await openLibraryAndTab(page, LibraryTab.Collections);

const urlStarred = new URL(page.url());
urlStarred.searchParams.set("starred", "1");
await page.goto(urlStarred.toString(), { waitUntil: "domcontentloaded" });
await page.waitForTimeout(1_500);
const starredThumbnailsBefore = page.locator(
"#records-container div[id^='thumbnail-']"
);
await page.locator("#records-container").waitFor({ state: "visible", timeout: 10_000 });
const starredCountBefore = await starredThumbnailsBefore.count();
const urlCollections = new URL(page.url());
urlCollections.searchParams.delete("starred");
await page.goto(urlCollections.toString(), { waitUntil: "domcontentloaded" });
await page.waitForTimeout(1_500);

await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 collections?/i)
).toBeVisible({ timeout: 10_000 });

await getBulkEditBar(page)
.getByRole("button", { name: /^Star$/i })
.click({ 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 page.goto(urlStarred.toString(), { waitUntil: "domcontentloaded" });
await page.waitForTimeout(1_500);
const thumbnails = page.locator("#records-container div[id^='thumbnail-']");
await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 });
await expect(thumbnails).toHaveCount(starredCountBefore + 2, {
timeout: 5_000
});

const starIconsInStarredView = page.locator(
"#records-container div[id^='thumbnail-'] .text-yellow-400"
);
await expect(starIconsInStarredView).toHaveCount(starredCountBefore + 2, {
timeout: 5_000
});
});

test("select multiple collections - Select all keeps bar visible with count", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoCollections(page);
await openLibraryAndTab(page, LibraryTab.Collections);

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(/Selected: \d+ collections?/i)
).toBeVisible({ timeout: 5_000 });
Comment on lines +131 to +146
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

});

test("select multiple collections - Archive shows success toast, clears selection, and collections are archived", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoCollections(page);
await openLibraryAndTab(page, LibraryTab.Collections);

const urlArchived = new URL(page.url());
urlArchived.searchParams.set("archived", "1");
await page.goto(urlArchived.toString(), { waitUntil: "domcontentloaded" });
await page.waitForTimeout(1_500);
const archivedThumbnailsBefore = page.locator(
"#records-container div[id^='thumbnail-']"
);
await page.locator("#records-container").waitFor({ state: "visible", timeout: 10_000 });
const archivedCountBefore = await archivedThumbnailsBefore.count();
const urlCollections = new URL(page.url());
urlCollections.searchParams.delete("archived");
await page.goto(urlCollections.toString(), { waitUntil: "domcontentloaded" });
await page.waitForTimeout(1_500);

await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 collections?/i)
).toBeVisible({ timeout: 10_000 });

await getBulkEditBar(page)
.getByRole("button", { name: /^Archive$/i })
.click({ 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 page.goto(urlArchived.toString(), { waitUntil: "domcontentloaded" });
await page.waitForTimeout(1_500);
const thumbnails = page.locator("#records-container div[id^='thumbnail-']");
await expect(thumbnails.first()).toBeVisible({ timeout: 10_000 });
await expect(thumbnails).toHaveCount(archivedCountBefore + 2, {
timeout: 5_000
});
});

test("select multiple collections - Delete shows success toast and clears selection", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoCollections(page);
await openLibraryAndTab(page, LibraryTab.Collections);

const recordsContainer = page.locator("#records-container");
const thumbnailsBefore = recordsContainer.locator("div[id^='thumbnail-']");
const countBefore = await thumbnailsBefore.count();

await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 collections?/i)
).toBeVisible({ timeout: 10_000 });

await getBulkEditBar(page)
.getByRole("button", { name: /^Delete$/i })
.click({ 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 page.waitForTimeout(1_500);
const thumbnailsAfter = recordsContainer.locator("div[id^='thumbnail-']");
await expect(thumbnailsAfter).toHaveCount(countBefore - 2, { timeout: 10_000 });
});
});
91 changes: 87 additions & 4 deletions apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import { test } from "@playwright/test";
import { ensureInAppOnHome } from "../../../utils/helpers";
import { test, expect } from "@playwright/test";
import {
ensureInAppOnHome,
openLibraryAndTab,
LibraryTab,
selectFirstTwoViaContextMenu,
createTwoGoals,
getBulkEditBar
} from "../../../utils/helpers";

const runtimeEnv = (
globalThis as { process?: { env?: Record<string, string | undefined> } }
Expand All @@ -19,8 +26,84 @@ test.describe("goal - bulk editor @regression", () => {
});
});

test.skip("bulk edit goals (select multiple, apply action)", async ({ page }) => {
test("select multiple goals via context menu - bulk edit bar appears and shows count", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
// TODO: select multiple goals in library, open bulk editor, apply action and assert
await createTwoGoals(page);
await openLibraryAndTab(page, LibraryTab.Goals);

await selectFirstTwoViaContextMenu(page, "records-container");

await expect(
page.getByText(/Selected: 2 goals?/i)
).toBeVisible({ timeout: 10_000 });
});

test("select multiple goals - clear selection hides bulk edit bar", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoGoals(page);
await openLibraryAndTab(page, LibraryTab.Goals);

await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 goals?/i)
).toBeVisible({ timeout: 10_000 });

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
});
Comment on lines +57 to +62
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

});

test("select multiple goals - Star shows success toast and clears selection", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoGoals(page);
await openLibraryAndTab(page, LibraryTab.Goals);

await selectFirstTwoViaContextMenu(page, "records-container");
await expect(
page.getByText(/Selected: 2 goals?/i)
).toBeVisible({ timeout: 10_000 });

await getBulkEditBar(page)
.getByRole("button", { name: /^Star$/i })
.click({ timeout: 5_000 });
await expect(
page.getByText(/Starred 2 goals? successfully/i)
).toBeVisible({ timeout: 10_000 });
await expect(page.getByText(/Selected: 2 goals?/i)).toBeHidden({
timeout: 5_000
});
});

test("select multiple goals - Select all keeps bar visible with count", async ({
page
}) => {
test.setTimeout(90_000);
await ensureInAppOnHome(page);
await createTwoGoals(page);
await openLibraryAndTab(page, LibraryTab.Goals);

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(/Selected: \d+ goals?/i)).toBeVisible({
timeout: 5_000
});
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

});
});
Loading
Loading