Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds extensive test infrastructure, Playwright E2E suites and helpers, many Vitest frontend/backend tests and factories, accessibility tweaks, a Zustand map-view store refactor, and a revamped GitHub Actions workflow with a shared pnpm install/cache and new test jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Setup as Setup Job (pnpm install)
participant Cache as Cache (actions/cache)
participant Jobs as Downstream Jobs (typecheck / lint / build / tests)
participant E2E as Playwright E2E
participant Artifacts as Artifacts (playwright-report)
CI->>Setup: start `setup` job (uses `NODE_VERSION`, `STORE_PATH_KEY`)
Setup->>Cache: restore pnpm store & node_modules (rgba(0,0,255,0.5))
Setup->>Setup: run `pnpm install`
Setup->>Cache: save pnpm store & node_modules (rgba(0,128,0,0.5))
CI->>Jobs: downstream jobs depend on `setup`
Jobs->>Cache: restore cache before execution (rgba(0,0,255,0.5))
Jobs->>Jobs: run typecheck / lint / frontend-test / backend-test / build
Jobs->>E2E: `e2e-test` runs after `build` (with secrets)
E2E->>E2E: install browsers, run Playwright tests
E2E->>Artifacts: upload `playwright-report/` artifact when applicable (rgba(128,0,128,0.5))
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/shadcn/components/collapsible.tsx (1)
125-131:⚠️ Potential issue | 🟡 MinorPrevent caller props from overriding collapsed-state accessibility attributes.
{...props}is currently last, so a consumer can overridearia-hiddenorinertand accidentally make closed content accessible/focusable again.🔧 Suggested fix
- aria-hidden={!isOpen} - inert={!isOpen ? true : undefined} - style={{ - overflow: 'hidden', - ...style, - }} - {...props} + {...props} + aria-hidden={!isOpen} + inert={!isOpen ? true : undefined} + style={{ + overflow: 'hidden', + ...style, + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/shadcn/components/collapsible.tsx` around lines 125 - 131, The spread of caller props comes after the component's accessibility attributes so consumers can override aria-hidden and inert; in the Collapsible component move {...props} before the explicit attributes or ensure aria-hidden and inert are set after props (so they win) — update the JSX in collapsible.tsx where aria-hidden, inert, style and {...props} are merged (the element rendering the collapsible content) so the component's aria-hidden={!isOpen} and inert={!isOpen ? true : undefined} cannot be overridden by user props.
🟡 Minor comments (14)
src/features/campaigns/components/campaign-dialog.tsx-87-109 (1)
87-109:⚠️ Potential issue | 🟡 MinorHandle edge case when
mode === 'edit'butcampaignis undefined.If
isOpenistrueandmode === 'edit'butcampaignisundefined, the effect does nothing and leaves potentially stale form data from a previous session. Consider adding defensive handling:} else if (campaign) { formRef.current.reset({ name: campaign.name, description: campaign.description || '', slug: campaign.slug, }) + } else { + // Edit mode without campaign - reset to defaults as fallback + formRef.current.reset({ ...DEFAULT_CAMPAIGN_FORM_VALUES }) }Alternatively, enforce the invariant at the type level by making
campaignrequired whenmode === 'edit'using discriminated unions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/campaigns/components/campaign-dialog.tsx` around lines 87 - 109, When the dialog opens in useEffect and mode === 'edit' but campaign is undefined, the form can retain stale values; update the effect to defensively handle that case by checking if (isOpen && mode === 'edit' && !campaign) and then calling formRef.current.reset(...) to clear or set DEFAULT_CAMPAIGN_FORM_VALUES (or a safe empty form) before returning; reference the existing useEffect, mode, campaign, isOpen, formRef.current.reset, and DEFAULT_CAMPAIGN_FORM_VALUES to locate where to add this branch (or alternatively enforce the invariant at the type level so campaign is required when mode === 'edit').src/features/sidebar/components/sidebar-toolbar/close-all-folders.tsx-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorMake the aria-label dynamic to match button behavior.
This is a toggle button, but the
aria-labelis static. WhencloseAllFoldersModeis already active, clicking the button exits the mode rather than closing folders. The label should reflect the actual action.♻️ Proposed fix with dynamic label
<Button variant="ghost" size="icon" onClick={toggleCloseAllFoldersMode} data-state={closeAllFoldersMode ? 'active' : 'inactive'} - aria-label="Close all folders" + aria-label={closeAllFoldersMode ? 'Open all folders' : 'Close all folders'} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sidebar/components/sidebar-toolbar/close-all-folders.tsx` at line 22, The aria-label is static but the button is a toggle; update the button in close-all-folders.tsx to use a dynamic aria-label based on the closeAllFoldersMode state (e.g. aria-label={closeAllFoldersMode ? "Exit close-all-folders mode" : "Enter close-all-folders mode"} or similar), and ensure it uses the same toggle handler you already have (toggleCloseAllFoldersMode / setCloseAllFoldersMode / onClick handler) so the label reflects the action the click will perform.convex/editors/__tests__/editors.test.ts-76-89 (1)
76-89:⚠️ Potential issue | 🟡 MinorAssert the actual defaults here.
creates editor with defaultscurrently passes as long as some editor row exists. A regression in defaultsortOrder,sortDirection, oreditorModewould stay green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/editors/__tests__/editors.test.ts` around lines 76 - 89, Update the "creates editor with defaults" test to assert the actual default fields instead of only existence: after calling dmAuth.query(api.editors.queries.getCurrentEditor, { campaignId: ctx.campaignId }) and storing the result, add explicit expectations that result.sortOrder, result.sortDirection, and result.editorMode equal the intended default values (and any other default fields you care about). Use the same test variables (ctx, dmAuth, id, result) and the query api.editors.queries.getCurrentEditor / mutation api.editors.mutations.setCurrentEditor to locate where to add these assertions.convex/users/__tests__/users.test.ts-105-113 (1)
105-113:⚠️ Potential issue | 🟡 MinorVerify
usernamewas persisted, not just returned.All of the success-path cases here only assert the mutation return value. If
updateUsernamenormalizes and returns the right string but forgets to patchuserProfiles.username, these tests still pass.💡 Example tightening
const result = await authed.mutation(api.users.mutations.updateUsername, { username: 'MyNewName', }) expect(result).toBe('mynewname') + const updated = await authed.query(api.users.queries.getUserProfile, {}) + expect(updated!.username).toBe('mynewname')Also applies to: 134-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/users/__tests__/users.test.ts` around lines 105 - 113, The test currently only asserts the return value of api.users.mutations.updateUsername; update the test to also fetch the persisted user profile and assert that userProfiles.username was updated to the lowercased value. After calling authed.mutation(api.users.mutations.updateUsername, ...), call the existing read/query helper (e.g., authed.query or the getUser/getUserProfile query) to retrieve the user's profile and assert profile.username === 'mynewname' so the mutation both returns and persists the normalized username.convex/sidebarShares/__tests__/sidebarShares.test.ts-79-105 (1)
79-105:⚠️ Potential issue | 🟡 MinorThese assertions don't actually prove soft-delete/undelete behavior.
getSidebarItemSharesonly returns active shares, so both tests still pass ifshareSidebarIteminserts a brand-new row instead of undeleting, or ifunshareSidebarItemhard-deletes the row. Read the underlying share record int.runfor these lifecycle cases.Also applies to: 111-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/sidebarShares/__tests__/sidebarShares.test.ts` around lines 79 - 105, The test currently only calls api.sidebarShares.queries.getSidebarItemShares which returns only active shares, so update the test to read the raw underlying share record (using t.run) to assert lifecycle behavior: after creating a soft-deleted share, call api.sidebarShares.mutations.shareSidebarItem and then use t.run to fetch the share row (by sidebarItemId, sidebarItemType, campaignMemberId) to confirm the existing record's deletionTime was cleared (undeleted) rather than a new row inserted; likewise update the corresponding test for unshare to use t.run to confirm deletionTime is set (soft-delete) and that no hard-delete occurred. Ensure you reference the same identifiers used in the test (shareSidebarItem, unshareSidebarItem, getSidebarItemShares, and t.run) when locating and asserting the record.convex/storage/__tests__/storage.test.ts-63-72 (1)
63-72:⚠️ Potential issue | 🟡 Minor
commitUploadstill has no happy-path coverage.The only authenticated success case is skipped, so regressions in the actual commit logic won't be caught here. Please move this into an integration-style test or seed the missing storage metadata in the harness instead of leaving the core path unverified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/storage/__tests__/storage.test.ts` around lines 63 - 72, Test "commits a tracked upload" is skipped so commitUpload's happy path isn't covered; unskip and either convert to an integration test calling setupUser(), use storage.store(new Blob(['test'], { type: 'text/plain' })) to create a storageId, call authed.mutation(api.storage.mutations.trackUpload, { storageId }) then authed.mutation(api.storage.mutations.commitUpload, { storageId }) and assert the result, or if keeping it as a unit test seed the convex-test storage mock/harness with the expected system metadata (including Blob contentType) for the storageId before calling commitUpload; update references to commitUpload, trackUpload, storage.store, and setupUser accordingly.convex/notes/__tests__/notes.test.ts-299-321 (1)
299-321:⚠️ Potential issue | 🟡 MinorUse
as constfor block type discriminants and strengthen the assertion.The test fixture doesn't narrow the
typestring literals withas const, which violates the coding guideline. Additionally, only checkingcontent.lengthdoesn't verify that the block structure was persisted correctly; a broken serializer could still pass. Other tests in the codebase (e.g.,blockShares/__tests__/blockSharingWorkflows.test.ts) already follow theas constpattern.Example fix
const blocks = [ { id: 'block-1', - type: 'paragraph', - content: [{ type: 'text', text: 'Hello', styles: {} }], + type: 'paragraph' as const, + content: [{ type: 'text' as const, text: 'Hello', styles: {} }], props: { textColor: 'default', textAlignment: 'left', backgroundColor: 'default', }, children: [], }, ] @@ const note = await playerAuth.query(api.notes.queries.getNote, { noteId }) expect(note?.content).toHaveLength(1) + expect(note?.content[0]).toMatchObject({ + id: 'block-1', + type: 'paragraph', + content: [{ type: 'text', text: 'Hello' }], + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes/__tests__/notes.test.ts` around lines 299 - 321, The test should narrow the block discriminants by declaring the fixture with `as const` (the `blocks` constant used when calling playerAuth.mutation(api.notes.mutations.updateNoteContent, { noteId, content: blocks }) ) so the `type` and nested literal fields are treated as exact types; update the fixture to use `as const`. Also strengthen the verification after fetching via playerAuth.query(api.notes.queries.getNote, { noteId }) by asserting the persisted structure exactly matches the original blocks (use a deep equality assertion such as expect(note?.content).toEqual(blocks) rather than only checking length) to ensure the serializer round-trips the full block shape.e2e/settings.spec.ts-17-18 (1)
17-18:⚠️ Potential issue | 🟡 MinorUse word boundary instead of trailing space for theme toggle matcher.
The theme button label is
"Light"or"Dark"with no trailing space (see lines 16 and 22 in preferences-tab.tsx). The current regex/^light /irequires a space after the theme name, which would only match if additional text follows. Using a word boundary with/^light\b/iis more appropriate and robust.Suggested fix
- const targetTheme = initialClass?.includes('dark') ? /^light /i : /^dark /i + const targetTheme = initialClass?.includes('dark') ? /^light\b/i : /^dark\b/i🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/settings.spec.ts` around lines 17 - 18, The regex used to find the theme toggle button (variable targetTheme computed from initialClass) incorrectly requires a trailing space (`/^light /i` / `^dark `); update the matcher to use a word boundary so it matches the exact labels ("Light"/"Dark") by replacing those patterns with `/^light\b/i` and `/^dark\b/i`, then use page.getByRole('button', { name: targetTheme }).click() as before to locate and click the button.e2e/trash.spec.ts-60-61 (1)
60-61:⚠️ Potential issue | 🟡 MinorScope restore button selector to the item row for clarity.
The test currently works because it only creates one trashed item, but scoping the selector makes the intent explicit and prevents fragility if the trash state changes.
Suggested fix
- const restoreBtn = page.getByRole('button', { name: /restore/i }) + const noteRow = page.getByText(noteName, { exact: true }).locator('..') + const restoreBtn = noteRow.getByRole('button', { name: /restore/i }) await restoreBtn.click()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/trash.spec.ts` around lines 60 - 61, Scope the "restore" button lookup to the specific trashed item row instead of selecting the first button globally: find the row or container for the trashed item (e.g., using the item text or test id in the test that creates the trashed item), then call getByRole('button', { name: /restore/i }) on that row/container (rather than page.getByRole) so the restoreBtn is specific to that item; update the selector logic around restoreBtn in the test that creates/checks the trashed item to use the scoped row/container query.e2e/sharing.spec.ts-47-50 (1)
47-50:⚠️ Potential issue | 🟡 MinorTighten share-menu selectors to avoid false positives.
These selectors are overly broad (
getByText(...).first()), so they can match unrelated UI. Scope assertions/interactions to the opened share dialog and target specific roles/labels.Also applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/sharing.spec.ts` around lines 47 - 50, After clicking the share button (page.getByRole('button', { name: /share/i }).click()), scope subsequent assertions to the opened share dialog instead of using page.getByText(...).first(); capture the dialog via a locator such as page.getByRole('dialog', { name: /share/i }) and run assertions against that locator (e.g., dialog.getByText(...), dialog.getByRole(...)) to target specific roles/labels for permissions/access; update the same pattern used around the second occurrence (the code block at lines ~60-64) to avoid false positives.src/features/campaigns/components/__tests__/campaigns-content.test.tsx-17-20 (1)
17-20:⚠️ Potential issue | 🟡 MinorUse JSX instead of
require('react')in the Link mock.Replace the
require('react')call andcreateElementwith direct JSX syntax:return <a href={props.to} {...props}>{children}</a>. This aligns with ESM-first development and avoids dynamic require calls in Vitest's ESM environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/campaigns/components/__tests__/campaigns-content.test.tsx` around lines 17 - 20, The Link mock currently uses a dynamic require and createElement which breaks ESM-first Vitest runs; replace the createElement usage in the Link mock with JSX syntax so the mock returns an anchor via JSX (<a href={props.to} {...props}>{children}</a>) instead of calling require('react'). Update the Link mock function signature (Link: ({ children, ...props }: Record<string, unknown>) => { ... }) to return the JSX element and keep props.to and children usage intact.e2e/helpers/map-helpers.ts-27-29 (1)
27-29:⚠️ Potential issue | 🟡 MinorFile-input selection is ambiguous and may hit the wrong control.
Using
.first()on all file inputs is brittle when additional upload controls are present.Suggested fix
export async function uploadMapImage(page: Page, imagePath: string) { - const fileInput = page.locator('input[type="file"]').first() - await fileInput.setInputFiles(imagePath) + const fileInputs = page.locator('input[type="file"]') + await expect(fileInputs).toHaveCount(1) + await fileInputs.first().setInputFiles(imagePath) await expect(page.locator('[aria-label="Map canvas"]')).toBeVisible({ timeout: 15000, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/map-helpers.ts` around lines 27 - 29, The file-input selection uses page.locator('input[type="file"]').first() which is brittle; change the locator to target the specific upload control used by the map before calling setInputFiles (e.g., narrow the selector by scoping to the map container or a test-id/aria-label such as using page.locator('[aria-label="Map canvas"] input[type="file"]') or a data-testid like page.locator('[data-testid="map-file-input"]')). Update the variable fileInput and the call await fileInput.setInputFiles(imagePath) to use this precise locator so the test always interacts with the intended upload control.convex/bookmarks/__tests__/bookmarks.test.ts-72-84 (1)
72-84:⚠️ Potential issue | 🟡 MinorTest name does not match actor under test.
This case uses
playerAuth, but the title reads like a DM-path test. Rename to reflect player behavior to avoid future misreads.Suggested rename
- it('allows bookmarking DM-owned note without explicit share', async () => { + it('allows player to bookmark DM-owned note without explicit share', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/bookmarks/__tests__/bookmarks.test.ts` around lines 72 - 84, The test title is misleading: the spec uses playerAuth but the name implies a DM path; update the it(...) description to reflect player behavior (e.g., rename the test containing playerAuth, createNote(..., ctx.dm.profile._id) and the toggleBookmark mutation call to something like "allows player to bookmark DM-owned note without explicit share") so the test name matches the actor under test (referencing playerAuth, createNote, and api.bookmarks.mutations.toggleBookmark to locate the test).convex/campaigns/__tests__/campaigns.test.ts-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorRemove the test isolation concern; it's unfounded. Each
describecreates a fresh context.The pattern of
const t = createTestContext()per describe is intentional and sound—eachdescribeblock gets an isolated test context instance, and mutations within a describe are independent (tests use their own setup viasetupUser(),createCampaignWithDm(), etc.). There's no cross-suite pollution or order dependency.However, the coverage gaps are valid:
getUserCampaigns: Test only asserts positive results for DM-owned campaigns. Missing a test for an Accepted player (non-DM) querying the function—would catch regressions to DM-only visibility.
getPlayersByCampaign: Test only asserts positive results when called as a DM. Missing a test for an Accepted player member querying successfully—would catch regressions to DM-only access.
deleteCampaign: Test name claims "all related records" but assertions only verifycampaignMembers,notes,folders, andsessions. Either expand assertions to match the name or narrow the name to describe the actual scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/campaigns/__tests__/campaigns.test.ts` at line 26, Do not change the per-describe test isolation (leave createTestContext() usage as-is); instead add missing assertions/tests: add a test that an Accepted player (use setupUser() to create a non-DM member via createCampaignWithDm() or equivalent helper) calling getUserCampaigns returns the campaign (verify visibility for non-DM Accepted members), add a test that an Accepted player can call getPlayersByCampaign and receives the member list, and for the deleteCampaign test either expand assertions to include every related record claimed in the test name or rename the test to only list the verified related tables (update references to deleteCampaign, getUserCampaigns, getPlayersByCampaign, setupUser(), createCampaignWithDm() accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/sidebarItems/functions/permanentlyDeleteSidebarItem.ts`:
- Around line 41-57: The permission check in permanentlyDeleteSidebarItem uses
ctx.db.query('sidebarItemShares').withIndex('by_campaign_item_member', ...) to
fetch a share but omits filtering out soft-deleted shares; update the query that
finds the share (the withIndex(...) call used to compute share and level) to
chain a .filter((q) => q.eq(q.field('deletionTime'), null)) before .first() so
revoked (soft-deleted) shares are ignored when computing permissionLevel for the
hasAtLeastPermissionLevel check.
In `@e2e/auth.setup.ts`:
- Line 21: Before calling page.context().storageState({ path: authFile }) ensure
the parent directory for authFile exists by awaiting
mkdir(path.dirname(authFile), { recursive: true }); update the setup to import
or use fs/promises.mkdir and path.dirname (or ensure path is imported) and place
await mkdir(...) immediately before the storageState call so that writing
e2e/.auth/user.json cannot fail with ENOENT.
In `@e2e/bookmarks.spec.ts`:
- Around line 44-61: The test "bookmark note and filter by bookmarks" relies on
state set by a prior test; make it independent by seeding the bookmark for
bookmarkedNote before assertions—either add a beforeAll that navigates (use
navigateToCampaign) and bookmarks bookmarkedNote via openContextMenu(page,
bookmarkedNote) then clicking the bookmark menu item, or perform that setup at
the start of this test itself (navigateToCampaign, openContextMenu, click the
/bookmark/i menuitem) so the subsequent click on 'Show bookmarks' and the
expects for sidebar links (bookmarkedNote and regularNote) do not depend on test
order.
In `@e2e/editor.spec.ts`:
- Around line 45-53: The test "create note appears in sidebar" depends on a note
created in another test; update this test to be self-contained by creating its
own unique note (use createNoteHelper with a locally generated noteName, e.g.,
append timestamp or UUID) or seed the note in a beforeAll hook so each test does
not rely on other tests' state; modify the test body that currently uses the
shared campaignName/noteName to instead generate and use a local noteName and
call createNoteHelper(page, localNoteName) before asserting
page.getByRole('link', { name: localNoteName, exact: true }) is visible (also
apply the same change to the other test(s) that reference the shared noteName).
In `@e2e/file-download.spec.ts`:
- Around line 80-87: The test currently only awaits page.waitForEvent('popup')
and downloadItem.click() and silently ignores both results, so add a concrete
download assertion by also awaiting page.waitForEvent('download') (e.g., include
page.waitForEvent('download', { timeout: 5000 }) in the Promise.all alongside
page.waitForEvent('popup') and downloadItem.click()), capture the returned
Download object, assert it is not null/undefined (or assert
download.suggestedFilename or download.path() resolves) to confirm a download
occurred, and still close popup if present; update the code around
page.waitForEvent('popup'), downloadItem.click(), and popup handling
accordingly.
In `@e2e/folder-nesting.spec.ts`:
- Around line 15-27: toggleFolderExpansion currently always clicks the chevron
and thus toggles regardless of current state; change it to be idempotent by
adding a desiredState parameter (e.g., expand: boolean) and first reading the
current state from the chevron or row (use the chevron locator in
toggleFolderExpansion and call chevron.getAttribute('aria-expanded') or
chevron.getAttribute('aria-pressed') / chevron.getAttribute('aria-label') to
infer "expanded" vs "collapsed"), then only call chevron.click() if the current
state differs from the desiredState. Ensure the function signature and callers
are updated to pass the intended state (expand vs collapse).
In `@e2e/game-maps.spec.ts`:
- Around line 176-198: The "toggle pin visibility" test mutates shared state by
leaving a pin hidden when the test finishes; update the test (named "toggle pin
visibility") to restore the pin's original visibility before exit: determine
original state via the existing wasHiding flag (from toggleItem.textContent()),
perform the toggle to test behavior, then if the pin ended up hidden ensure you
toggle it back (or explicitly make it visible via the same menu flow or by
reloading/opening the map with openMap/navigateToCampaign) so other tests like
"remove pin from map" see a visible first pin; use the same pin locator
('[data-pin-id]').first() and toggleItem menu interaction to revert state.
In `@e2e/permissions-granularity.spec.ts`:
- Around line 66-118: Tests are state-coupled (none → view → edit → none)
causing flakiness; make each test self-contained by explicitly setting the
starting permission before assertions (use the same helpers: navigateToCampaign,
openItem, openShareMenu, allPlayersRow and the permSelect locator). For each
test (the ones named 'set all-players permission to View', 'change all-players
permission to Edit', 'change permission back to None') first open the share
menu, then force the permission into the required initial state by clicking
permSelect and selecting the needed option (e.g., select 'none' at start of the
View test, select 'view' at start of the Edit test, select 'edit' at start of
the None test), wait for the selection to settle, and only then perform the
action/assertion; alternatively consolidate the sequence of transitions into a
single scenario test if you prefer one-flow coverage.
In `@e2e/realtime-collaboration.spec.ts`:
- Around line 43-77: The test "two tabs see same note content" currently only
closes page1, page2, context1, context2 on the happy path; wrap the test body in
a try/finally so that page1.close(), page2.close(), context1.close(), and
context2.close() are always called in the finally block (guarding each close
with existence checks), mirroring the same try/finally pattern for the other
tests mentioned (lines 79-124, 126-167) to prevent leaked browser resources.
In `@e2e/settings.spec.ts`:
- Line 5: Replace the brittle use of page.getByRole('button').first().click() in
e2e/settings.spec.ts with a stable locator that targets the user menu button by
its aria-label "User menu" (i.e., use the button role with accessible name "User
menu" or getByLabel for "User menu") for both occurrences (the two places
calling .first()). Locate the two calls to
page.getByRole('button').first().click() and change them to click the explicit
"User menu" button instead.
In `@e2e/sidebar.spec.ts`:
- Around line 77-82: The rename assertion is too broad and can match other page
text; narrow the selectors to the specific sidebar/menu item so you verify the
actual item was renamed: locate the target sidebar entry first (e.g., using
getByRole('menuitem') or a sidebar container locator) then perform the rename
using renameInput and assert the renamed label only within that located
menu-item/sidebar link (use locator.locator or within-style scoping) instead of
page.getByText(renamedName) so the test fails if the sidebar item wasn’t
actually updated.
In `@e2e/view-as-player.spec.ts`:
- Around line 137-140: The test currently uses a nondeterministic selector
page.getByRole('menuitemcheckbox').first() which can pick the wrong player when
multiple checkboxes exist; replace this with a deterministic locator that
targets the player by the E2E_PLAYER_EMAIL text (e.g., use
page.getByRole('menuitemcheckbox', { name: E2E_PLAYER_EMAIL }) or a locator with
hasText/has to match E2E_PLAYER_EMAIL) and then await the locator to be visible
before clicking; ensure E2E_PLAYER_EMAIL is imported/available in the test and
remove the .first() usage in the player selection code.
In `@src/features/auth/components/__tests__/sign-in-form.test.tsx`:
- Around line 13-16: The inline use of require('react') inside the Link mock
violates verbatimModuleSyntax; move the import to top-level ESM and use that
import in the Link function. Add a top-level ESM import (e.g., import React from
'react' or import { createElement } from 'react') and replace the
require('react') call inside the Link definition so Link: ({ children, ...props
}) => React.createElement('a', { href: props.to, ...props }, children) (or use
createElement) – update the Link symbol accordingly and remove the require call.
---
Outside diff comments:
In `@src/features/shadcn/components/collapsible.tsx`:
- Around line 125-131: The spread of caller props comes after the component's
accessibility attributes so consumers can override aria-hidden and inert; in the
Collapsible component move {...props} before the explicit attributes or ensure
aria-hidden and inert are set after props (so they win) — update the JSX in
collapsible.tsx where aria-hidden, inert, style and {...props} are merged (the
element rendering the collapsible content) so the component's
aria-hidden={!isOpen} and inert={!isOpen ? true : undefined} cannot be
overridden by user props.
---
Minor comments:
In `@convex/bookmarks/__tests__/bookmarks.test.ts`:
- Around line 72-84: The test title is misleading: the spec uses playerAuth but
the name implies a DM path; update the it(...) description to reflect player
behavior (e.g., rename the test containing playerAuth, createNote(...,
ctx.dm.profile._id) and the toggleBookmark mutation call to something like
"allows player to bookmark DM-owned note without explicit share") so the test
name matches the actor under test (referencing playerAuth, createNote, and
api.bookmarks.mutations.toggleBookmark to locate the test).
In `@convex/campaigns/__tests__/campaigns.test.ts`:
- Line 26: Do not change the per-describe test isolation (leave
createTestContext() usage as-is); instead add missing assertions/tests: add a
test that an Accepted player (use setupUser() to create a non-DM member via
createCampaignWithDm() or equivalent helper) calling getUserCampaigns returns
the campaign (verify visibility for non-DM Accepted members), add a test that an
Accepted player can call getPlayersByCampaign and receives the member list, and
for the deleteCampaign test either expand assertions to include every related
record claimed in the test name or rename the test to only list the verified
related tables (update references to deleteCampaign, getUserCampaigns,
getPlayersByCampaign, setupUser(), createCampaignWithDm() accordingly).
In `@convex/editors/__tests__/editors.test.ts`:
- Around line 76-89: Update the "creates editor with defaults" test to assert
the actual default fields instead of only existence: after calling
dmAuth.query(api.editors.queries.getCurrentEditor, { campaignId: ctx.campaignId
}) and storing the result, add explicit expectations that result.sortOrder,
result.sortDirection, and result.editorMode equal the intended default values
(and any other default fields you care about). Use the same test variables (ctx,
dmAuth, id, result) and the query api.editors.queries.getCurrentEditor /
mutation api.editors.mutations.setCurrentEditor to locate where to add these
assertions.
In `@convex/notes/__tests__/notes.test.ts`:
- Around line 299-321: The test should narrow the block discriminants by
declaring the fixture with `as const` (the `blocks` constant used when calling
playerAuth.mutation(api.notes.mutations.updateNoteContent, { noteId, content:
blocks }) ) so the `type` and nested literal fields are treated as exact types;
update the fixture to use `as const`. Also strengthen the verification after
fetching via playerAuth.query(api.notes.queries.getNote, { noteId }) by
asserting the persisted structure exactly matches the original blocks (use a
deep equality assertion such as expect(note?.content).toEqual(blocks) rather
than only checking length) to ensure the serializer round-trips the full block
shape.
In `@convex/sidebarShares/__tests__/sidebarShares.test.ts`:
- Around line 79-105: The test currently only calls
api.sidebarShares.queries.getSidebarItemShares which returns only active shares,
so update the test to read the raw underlying share record (using t.run) to
assert lifecycle behavior: after creating a soft-deleted share, call
api.sidebarShares.mutations.shareSidebarItem and then use t.run to fetch the
share row (by sidebarItemId, sidebarItemType, campaignMemberId) to confirm the
existing record's deletionTime was cleared (undeleted) rather than a new row
inserted; likewise update the corresponding test for unshare to use t.run to
confirm deletionTime is set (soft-delete) and that no hard-delete occurred.
Ensure you reference the same identifiers used in the test (shareSidebarItem,
unshareSidebarItem, getSidebarItemShares, and t.run) when locating and asserting
the record.
In `@convex/storage/__tests__/storage.test.ts`:
- Around line 63-72: Test "commits a tracked upload" is skipped so
commitUpload's happy path isn't covered; unskip and either convert to an
integration test calling setupUser(), use storage.store(new Blob(['test'], {
type: 'text/plain' })) to create a storageId, call
authed.mutation(api.storage.mutations.trackUpload, { storageId }) then
authed.mutation(api.storage.mutations.commitUpload, { storageId }) and assert
the result, or if keeping it as a unit test seed the convex-test storage
mock/harness with the expected system metadata (including Blob contentType) for
the storageId before calling commitUpload; update references to commitUpload,
trackUpload, storage.store, and setupUser accordingly.
In `@convex/users/__tests__/users.test.ts`:
- Around line 105-113: The test currently only asserts the return value of
api.users.mutations.updateUsername; update the test to also fetch the persisted
user profile and assert that userProfiles.username was updated to the lowercased
value. After calling authed.mutation(api.users.mutations.updateUsername, ...),
call the existing read/query helper (e.g., authed.query or the
getUser/getUserProfile query) to retrieve the user's profile and assert
profile.username === 'mynewname' so the mutation both returns and persists the
normalized username.
In `@e2e/helpers/map-helpers.ts`:
- Around line 27-29: The file-input selection uses
page.locator('input[type="file"]').first() which is brittle; change the locator
to target the specific upload control used by the map before calling
setInputFiles (e.g., narrow the selector by scoping to the map container or a
test-id/aria-label such as using page.locator('[aria-label="Map canvas"]
input[type="file"]') or a data-testid like
page.locator('[data-testid="map-file-input"]')). Update the variable fileInput
and the call await fileInput.setInputFiles(imagePath) to use this precise
locator so the test always interacts with the intended upload control.
In `@e2e/settings.spec.ts`:
- Around line 17-18: The regex used to find the theme toggle button (variable
targetTheme computed from initialClass) incorrectly requires a trailing space
(`/^light /i` / `^dark `); update the matcher to use a word boundary so it
matches the exact labels ("Light"/"Dark") by replacing those patterns with
`/^light\b/i` and `/^dark\b/i`, then use page.getByRole('button', { name:
targetTheme }).click() as before to locate and click the button.
In `@e2e/sharing.spec.ts`:
- Around line 47-50: After clicking the share button (page.getByRole('button', {
name: /share/i }).click()), scope subsequent assertions to the opened share
dialog instead of using page.getByText(...).first(); capture the dialog via a
locator such as page.getByRole('dialog', { name: /share/i }) and run assertions
against that locator (e.g., dialog.getByText(...), dialog.getByRole(...)) to
target specific roles/labels for permissions/access; update the same pattern
used around the second occurrence (the code block at lines ~60-64) to avoid
false positives.
In `@e2e/trash.spec.ts`:
- Around line 60-61: Scope the "restore" button lookup to the specific trashed
item row instead of selecting the first button globally: find the row or
container for the trashed item (e.g., using the item text or test id in the test
that creates the trashed item), then call getByRole('button', { name: /restore/i
}) on that row/container (rather than page.getByRole) so the restoreBtn is
specific to that item; update the selector logic around restoreBtn in the test
that creates/checks the trashed item to use the scoped row/container query.
In `@src/features/campaigns/components/__tests__/campaigns-content.test.tsx`:
- Around line 17-20: The Link mock currently uses a dynamic require and
createElement which breaks ESM-first Vitest runs; replace the createElement
usage in the Link mock with JSX syntax so the mock returns an anchor via JSX (<a
href={props.to} {...props}>{children}</a>) instead of calling require('react').
Update the Link mock function signature (Link: ({ children, ...props }:
Record<string, unknown>) => { ... }) to return the JSX element and keep props.to
and children usage intact.
In `@src/features/campaigns/components/campaign-dialog.tsx`:
- Around line 87-109: When the dialog opens in useEffect and mode === 'edit' but
campaign is undefined, the form can retain stale values; update the effect to
defensively handle that case by checking if (isOpen && mode === 'edit' &&
!campaign) and then calling formRef.current.reset(...) to clear or set
DEFAULT_CAMPAIGN_FORM_VALUES (or a safe empty form) before returning; reference
the existing useEffect, mode, campaign, isOpen, formRef.current.reset, and
DEFAULT_CAMPAIGN_FORM_VALUES to locate where to add this branch (or
alternatively enforce the invariant at the type level so campaign is required
when mode === 'edit').
In `@src/features/sidebar/components/sidebar-toolbar/close-all-folders.tsx`:
- Line 22: The aria-label is static but the button is a toggle; update the
button in close-all-folders.tsx to use a dynamic aria-label based on the
closeAllFoldersMode state (e.g. aria-label={closeAllFoldersMode ? "Exit
close-all-folders mode" : "Enter close-all-folders mode"} or similar), and
ensure it uses the same toggle handler you already have
(toggleCloseAllFoldersMode / setCloseAllFoldersMode / onClick handler) so the
label reflects the action the click will perform.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 126-150: Remove the unused secret from the e2e-test job by
deleting E2E_CONVEX_URL from the env block, and add the required player
credentials expected by player-related tests by including E2E_PLAYER_EMAIL and
E2E_PLAYER_PASSWORD as environment variables (sourced from
repository/organization secrets) in the same job; reference the e2e-test job,
the env keys (E2E_CONVEX_URL, E2E_PLAYER_EMAIL, E2E_PLAYER_PASSWORD,
TEST_RUN_ID) and the Playwright webServer (pnpm dev:web on localhost:3000) to
locate where to make these changes and ensure player-invite.spec.ts will run
without skipping.
In `@convex/_test/assertions.helper.ts`:
- Around line 6-28: Add an explicit return type to the expectClientError
function signature to improve clarity and typing; change the function
declaration for expectClientError to return Promise<ConvexError | Error> (or a
union that includes the ConvexError type and plain Error) so callers and
TypeScript know the resolved error type, keeping the existing logic in the body
(checks using isClientError and returning the caught error).
In `@convex/_test/factories.helper.ts`:
- Around line 350-376: createSidebarShare duplicates default share setup logic
found in createBlockShare; extract a small helper (e.g., buildShareDefaults or
getShareDefaults) that returns the common defaults object (permissionLevel,
sessionId, commonFields(creatorProfileId), deletionTime/deletedBy if applicable)
and use it in both createSidebarShare and createBlockShare to compose data = {
...defaults, ...overrides } before inserting; update createSidebarShare to call
this helper and remove duplicated default declarations.
- Around line 20-24: The module-level counter used by nextId() causes shared
state across tests; add and export a reset function (e.g., resetCounter or
resetNextId) that sets counter = 0 so tests can restore isolation, update the
module exports to include this function, and ensure tests call it (for example
in a beforeEach) to guarantee reproducible fixtures when using nextId().
In `@convex/_test/identities.helper.ts`:
- Around line 41-46: The players array is currently inferred; make its type
explicit to improve readability and maintainability by declaring a proper type
for the elements (e.g., a PlayerContext or PlayerWithMemberId) and annotating
the players variable as that array type; update the declaration of players (and,
if helpful, create a type alias) so it reflects the shape returned by setupUser
and addPlayerToCampaign while keeping the rest of the loop using setupUser,
addPlayerToCampaign, playerCount, and campaignId unchanged.
In `@convex/auth/__tests__/auth.test.ts`:
- Around line 34-40: Tests currently pass empty strings for email/name which
only exercise falsy-string behavior; update the test invocations of onCreateUser
to omit the email or name properties entirely (remove the email key for the
"missing email" case and remove the name key for the "missing name" case) so the
function is exercised with truly absent fields rather than empty strings; ensure
both places that mirror lines 34-40 and 56-62 use object literals without the
respective property to validate the omitted-field branches in onCreateUser.
In `@convex/blockShares/__tests__/blockShares.test.ts`:
- Around line 211-216: The assertion combines two conditions and should be split
for clearer failures: after calling
dmAuth.query(api.blocks.queries.getBlockWithShares, { noteId, blockId }) assign
to result and then assert explicitly either expect(result).toBeNull() if null is
an acceptable outcome, or expect(result).not.toBeNull() followed by
expect(result!.shares.length).toBe(0) when result must exist but have zero
shares; add a brief comment documenting that null is an acceptable/expected case
if applicable.
In `@convex/blockShares/__tests__/blockSharingWorkflows.test.ts`:
- Around line 310-322: Replace the use of .collect() and in-memory filtering in
the t.run block: instead of materializing allShares, iterate the query result
returned by dbCtx.db.query('blockShares').withIndex('by_campaign_block_member',
...) using a for await (... of query) loop inside the t.run callback, increment
a counter for rows with deletionTime === null, return that count from t.run
(e.g., activeCount) and assert expect(activeCount).toBe(1); update references to
allShares/activeShares accordingly.
In `@convex/campaigns/__tests__/campaignLifecycle.test.ts`:
- Around line 15-149: The full-lifecycle test it('full lifecycle: create, join,
accept, content, remove, delete') is too large—extract the database cleanup
assertions (the t.run blocks that check
remainingNotes/remainingFolders/remainingFiles/remainingGameMaps/remainingMembers
and the final campaign null check, i.e. the code that runs after
api.campaigns.mutations.deleteCampaign) into a separate test or a reusable
helper; implement a new test like it('campaign deletion cleans up resources') or
a helper function assertCampaignDeleted(campaignId, t) that runs the db queries
(using ctx.db.query(...).withIndex(...).collect()) and asserts lengths are zero,
then call that helper from the original lifecycle test (or replace the inline
assertions with a single call) to keep
createFolder/createNote/createFile/createGameMap and the deleteCampaign flow in
the original test focused.
In `@convex/campaigns/__tests__/campaigns.test.ts`:
- Around line 847-894: The test named "hard-deletes campaign and all related
records" only asserts removal of the campaign row and records in
campaignMembers, notes, folders, and sessions (created via
createNote/createFolder/createSession and deleted by the
api.campaigns.mutations.deleteCampaign call), so either rename the test to
reflect the specific tables it verifies (e.g., "hard-deletes campaign and
related members, notes, folders, sessions") or extend it to cover other
campaign-owned entities by creating fixtures for them in setupCampaignContext
and adding corresponding queries/assertions (for each additional table, use
dbCtx.db.query(...).withIndex(...).collect() and expect length 0) to ensure
deleteCampaign cascades to those tables as well.
- Around line 150-162: Add a positive test that verifies an Accepted player
membership sees the campaign: after using setupUser and createCampaignWithDm to
make a campaign (as in the existing test), create a membership for a second user
with role Player and status Accepted (use your existing membership helper used
elsewhere in tests), then call api.campaigns.queries.getUserCampaigns as that
player's authed client and assert the returned list contains the campaignId;
reference the helpers setupUser, createCampaignWithDm and the query
api.campaigns.queries.getUserCampaigns and assert campaigns includes the created
campaignId.
- Around line 311-325: The test currently only calls
api.campaigns.queries.getPlayersByCampaign as the DM (using asDm(ctx)), which
misses the case where a non-DM campaign member should also succeed; update the
test (and the similar block at 347-355) to also authenticate as a regular member
(e.g., create or use a member auth helper like asMember(ctx) or asUser(ctx,
memberId)) and call getPlayersByCampaign with that auth context to assert the
same happy-path expectations (members length and each
member.userProfile.username defined), ensuring the query succeeds for campaign
members who are not DM.
In `@convex/common/__tests__/validation.test.ts`:
- Around line 90-97: Tests validateUsername are duplicated: the boundary checks
for min/max length in the it blocks that call validateUsername('a', 'a', 2, 30)
and validateUsername(over, over, 2, 30) repeat earlier tests; remove or
consolidate these duplicate assertions so each boundary case is tested only once
(keep the original assertions around validateUsername and delete the redundant
it blocks referencing validateUsername with the same min/max scenarios).
In `@convex/sidebarItems/__tests__/purgeExpiredTrash.test.ts`:
- Around line 7-134: Add a new test that checks the exact 30-day boundary for
purgeExpiredTrash: compute THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000 (or
Date.now() - 30*...) then create a note via createNote with location: 'trash'
and deletionTime: Date.now() - THIRTY_DAYS_MS, call the purge via
internal.sidebarItems.internalMutations.purgeExpiredTrash, then fetch the note
(using t.run and dbCtx.db.get(noteId)) and assert it is preserved
(expect(...).not.toBeNull()). This test should live alongside the existing cases
in purgeExpiredTrash.test.ts to ensure the threshold behavior (>30 days vs >=30
days) is explicit.
In `@convex/vitest.config.mts`:
- Line 7: Remove the redundant setupFiles entry from the Vitest config: delete
the setupFiles: ['convex/_test/setup.helper.ts'] line in the configuration
(convex/vitest.config.mts) because convex/_test/setup.helper.ts only exports a
factory (createTestContext) and has no global setup side effects; keep importing
and calling createTestContext() directly from tests instead.
In `@e2e/concurrent-operations.spec.ts`:
- Around line 42-68: The test opens multiple Playwright contexts/pages
(context1, context2, page1, page2) and may leave them open if an assertion
fails; wrap the test body that navigates and asserts (including calls to
navigateToCampaign and createNote) in a try/finally and move the cleanup
(page1.close(), page2.close(), context1.close(), context2.close()) into the
finally block so pages/contexts are always closed even on failure; ensure you
null-check or conditionally close only non-null handles to avoid throwing during
cleanup.
In `@e2e/drag-and-drop.spec.ts`:
- Around line 10-12: noteName and folderName are generated at module load using
Date.now(), causing stale values across test re-runs; move their generation into
a runtime hook or factory so each test run gets fresh names. Replace the
module-scope constants noteName and folderName with values created inside
beforeAll (or a helper function invoked in beforeAll/each) and keep
campaignName/testName usage as needed; update any references to noteName and
folderName in the spec to use the newly created variables from beforeAll.
In `@e2e/error-states.spec.ts`:
- Line 55: The test defines renameInput using getByPlaceholder(/enter a name/i);
replace this with the role-based selector used elsewhere to be consistent and
more accessible—use page.getByRole('textbox', { name: /enter a name/i }) when
assigning renameInput so the test aligns with name-validation.spec.ts and other
E2E tests that prefer role selectors over placeholder matching.
- Line 73: The test uses page.waitForLoadState('networkidle') which is flaky;
replace this global network idle wait with an explicit wait for a stable,
meaningful DOM signal (e.g. a specific element, text, or component that
indicates the page is ready) by updating the test in e2e/error-states.spec.ts to
wait for a concrete selector or locator (via page.waitForSelector or
locator.waitFor) tied to the page state you care about (error panel, submit
button, specific text) instead of using page.waitForLoadState('networkidle').
In `@e2e/helpers/editor-helpers.ts`:
- Around line 32-39: openSlashMenu duplicates the editor lookup logic; update it
to call the existing getEditor() helper instead of repeating
page.locator('[contenteditable="true"]').first(), then use the returned editor
(from getEditor) for click and typing so the same visibility/assertion behavior
is applied consistently across helpers (modify openSlashMenu to call
getEditor(), await its visibility if not already ensured, click it, type '/',
and return the menu as before).
In `@e2e/helpers/permission-helpers.ts`:
- Around line 15-27: The retry loop that clicks userMenuBtn and attempts to
click the "settings" button should fail explicitly if all retries fail; add a
boolean flag (e.g., settingsOpened) initialized false before the loop, set it
true when the page.getByRole('button', { name: /^settings$/i }).click(...)
succeeds inside the try block, and after the for loop throw a clear Error (with
context like "Unable to open settings after 3 attempts") if settingsOpened
remains false so callers see a precise failure instead of a later generic
timeout.
In `@e2e/sidebar-sorting.spec.ts`:
- Around line 29-34: The test is using fixed sleeps (page.waitForTimeout) which
is fragile; remove those waits and ensure deterministic timestamps by creating
notes with explicit creation times or by polling for distinct _creationTime
values: update the test to call createNote with a supplied timestamp (extend
createNote to accept an optional createdAt/creationTime param or use the backend
API helper that sets creationTime) so noteAlpha, noteBeta, noteCharlie get
monotonic timestamps, or alternatively replace waits with a retry/poll loop that
reads the created note's _creationTime until it differs before creating the
next; reference createNote and remove page.waitForTimeout usages in
sidebar-sorting.spec.ts.
In `@package.json`:
- Around line 21-22: Update the E2E npm scripts ("test:e2e" and "test:e2e:ui")
to use the conventional invocation (npx playwright test / npx playwright test
--ui) instead of pointing to ./node_modules/@playwright/test/cli.js, and if Node
compatibility is a concern replace the --env-file usage with a cross-version
solution such as dotenv-cli (e.g., dotenv -e .env.local -- npx playwright test)
so CI and older developer Node versions won’t break.
In `@src/features/campaigns/components/campaign-dialog.tsx`:
- Around line 84-86: The useRef indirection (const formRef = useRef(form);
formRef.current = form) is unnecessary because TanStack's useForm returns a
stable instance; replace usages of formRef.current in the useEffect with the
form variable directly and add form to the effect dependencies (i.e., update the
effect that calls form.reset(...) to depend on [mode, campaign, isOpen, form])
or remove the ref entirely and reference form.reset(...) in the effect; keep
formRef only if you have observed stale-closure problems.
In `@src/features/campaigns/contexts/__tests__/campaign-context.test.tsx`:
- Around line 24-27: The mocked Link component in the test uses require('react')
and createElement; replace it with a JSX functional component to match the
ESM/modern JSX transform: update the Link mock (the Link: ({ children, ...props
}: Record<string, unknown>) => { ... } entry) to return <a href={props.to}
{...props}>{children}</a> using an arrow function component and proper
typing/props spread instead of require/createElement.
In `@src/features/campaigns/hooks/__tests__/use-campaign.test.tsx`:
- Around line 49-85: Combine the two duplicate tests into a single table-driven
test that iterates over role/expected pairs; replace the separate it blocks with
a parameterized test that for each row creates a campaign via createCampaign({
myMembership: { role: CAMPAIGN_MEMBER_ROLE.* } }), builds the
CampaignContextType value (dmUsername, campaignSlug, campaign:
mockAuthQuery(campaign), isDm: expected, isCampaignLoaded: true, campaignId),
wraps with createWrapper(value), calls renderHook(() => useCampaign()), and
asserts result.current.isDm equals expected; keep references to useCampaign,
createCampaign, CAMPAIGN_MEMBER_ROLE, createWrapper, CampaignContextType,
mockAuthQuery and renderHook to locate where to change.
In `@src/features/editor/components/viewer/map/map-viewer.tsx`:
- Around line 823-845: Add keyboard support to the map canvas by wiring an
onKeyDown handler on the same div (role="application") that mirrors the
click/context logic: when Enter or Space is pressed, compute the target position
(use existing getPercentageFromClick or a maintained lastPointerPosition updated
by handleMouseMove) and call handlePlacePin if pendingPinItem or handleMovePin
if pendingPinMove; ensure the handler calls preventDefault/stopPropagation as
needed and falls back to handleMapImageContextMenu for other keys, and keep
existing handlers handleMapClick, handleMouseMove, handlePlacePin,
handleMovePin, handleMapImageContextMenu and getPercentageFromClick as the
referenced symbols to locate where to add this logic.
In `@src/features/editor/utils/__tests__/filter-suggestion-items.test.ts`:
- Around line 4-8: The test's items array lacks an explicit type; update the
declaration of items to include a generic/explicit type matching the input shape
expected by filterSuggestionItems (e.g., the exported item type or an inline
type like { title: string; aliases: string[] }), so the test data is
type-checked; reference the items constant and the filterSuggestionItems input
type (import the exported type from the module if available) and use it in the
items declaration.
In `@src/features/editor/utils/__tests__/heading-utils.test.ts`:
- Around line 21-43: The test fixtures (e.g., the const content array in the
heading-utils.test.ts tests) lack explicit generic types; update each fixture
array declaration to include an explicit element type (for example const
content: Array<Block> = [...] or const content: EditorNode[] = [...]) matching
the shape used in the tests, and apply the same change to the other fixture
arrays in this test file so their types remain stable as the fixtures evolve;
ensure you pick the actual element type name used by the editor model (Block,
EditorNode, HeadingNode, etc.) when adding the generic.
In `@src/features/sidebar/hooks/__tests__/sort-items.test.ts`:
- Line 21: The test fixture array `items` is missing an explicit generic type;
update its declaration to include a typed array such as `const items:
Array<typeof noteA> = [noteB, noteC, noteA]` (or replace `typeof noteA` with the
concrete type used for these fixtures, e.g., `Array<SidebarItem>`), so the
`items` variable and the related fixtures `noteA`, `noteB`, `noteC` are
explicitly typed per repo conventions.
In `@src/test/factories/campaign-factory.ts`:
- Line 19: The campaignId assignment calls testId without a type parameter;
change the call in the campaign factory so testId is invoked with the
appropriate type parameter (e.g., testId<'campaigns'>) to match other tests'
patterns and ensure proper type inference; update the expression that sets
campaignId (the const campaignId = overrides?._id ?? testId(...)) to use
testId<'campaigns'>(...) so the override and generated id types align.
- Line 53: The _id creation uses testId without its generic type parameter;
update the call to testId to include the correct entity type (the type of the
member id used in this factory) so the generic is explicit — e.g., add the
appropriate type argument to testId(...) where _id:
testId(`member_${memberCounter}`) is set in campaign-factory.ts, ensuring the
generic matches the member id type used by this factory.
In `@src/test/factories/sidebar-item-factory.ts`:
- Line 16: The module-level variable itemCounter in sidebar-item-factory.ts
makes generated IDs depend on test execution order; change the factory to accept
an optional id override and avoid relying solely on the shared itemCounter (or
expose a resetCounter function) so tests can provide deterministic IDs or reset
state in beforeEach; specifically update the factory that references itemCounter
to use the provided id when present and fall back to incrementing itemCounter
only when no override is supplied, and add an exported resetItemCounter (or
document resetting behavior) so tests can call it in beforeEach for
deterministic snapshots.
In `@src/test/factories/user-factory.ts`:
- Around line 4-7: The module-level userCounter used by createUser creates
test-order coupling; add and export a reset helper (e.g., resetUserCounter) that
sets userCounter back to 0 and use it in tests when needed. Locate the
userCounter and createUser in this file and implement an exported function
resetUserCounter(): void that assigns userCounter = 0, keeping userCounter
non-default-exported but accessible via the new named export so suites can call
resetUserCounter() to ensure deterministic user IDs.
In `@src/test/helpers/test-id.ts`:
- Around line 1-9: The testId helper is duplicated (function testId<T extends
TableNames>(value: string): Id<T>) — consolidate by creating a single shared
helper module and have the other file re-export it (or remove one implementation
and export the remaining function), update imports to reference the shared
module, and ensure the exported symbol remains testId so existing tests don't
need changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+41
to
+57
| const share = await ctx.db | ||
| .query('sidebarItemShares') | ||
| .withIndex('by_campaign_item_member', (q) => | ||
| q | ||
| .eq('campaignId', rawItem.campaignId) | ||
| .eq('sidebarItemId', itemId) | ||
| .eq('campaignMemberId', membership._id), | ||
| ) | ||
| .first() | ||
|
|
||
| const level = share?.permissionLevel ?? PERMISSION_LEVEL.NONE | ||
| if (!hasAtLeastPermissionLevel(level, PERMISSION_LEVEL.FULL_ACCESS)) { | ||
| throwClientError( | ||
| ERROR_CODE.PERMISSION_DENIED, | ||
| 'You do not have sufficient permission for this item', | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the sidebarItemShares index includes deletionTime or if queries elsewhere filter soft-deleted shares
ast-grep --pattern $'query("sidebarItemShares")
$$$
.withIndex($$$)
$$$'
# Also check the schema for the index definition
rg -n "by_campaign_item_member" --type ts -C 3Repository: ntietje1/wizard-archive
Length of output: 7280
Add .filter() to exclude soft-deleted shares from permission check.
The query retrieves a share to determine permission level but doesn't filter out soft-deleted shares. Similar queries throughout the codebase (e.g., getSidebarItemSharesForItem, getCampaignShares, enhanceSidebarItem) explicitly filter with .filter((q) => q.eq(q.field('deletionTime'), null)). Without this filter, a revoked (soft-deleted) share could still grant permission, creating a security gap.
.withIndex('by_campaign_item_member', (q) =>
q
.eq('campaignId', rawItem.campaignId)
.eq('sidebarItemId', itemId)
.eq('campaignMemberId', membership._id),
)
.filter((q) => q.eq(q.field('deletionTime'), null))
.first()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/sidebarItems/functions/permanentlyDeleteSidebarItem.ts` around lines
41 - 57, The permission check in permanentlyDeleteSidebarItem uses
ctx.db.query('sidebarItemShares').withIndex('by_campaign_item_member', ...) to
fetch a share but omits filtering out soft-deleted shares; update the query that
finds the share (the withIndex(...) call used to compute share and level) to
chain a .filter((q) => q.eq(q.field('deletionTime'), null)) before .first() so
revoked (soft-deleted) shares are ignored when computing permissionLevel for the
hasAtLeastPermissionLevel check.
Comment on lines
+44
to
+61
| test('bookmark note and filter by bookmarks', async ({ page }) => { | ||
| await page.goto('/campaigns') | ||
| await navigateToCampaign(page, campaignName) | ||
|
|
||
| const sidebar = page.getByRole('navigation', { name: 'Sidebar' }) | ||
|
|
||
| await openContextMenu(page, bookmarkedNote) | ||
| await page.getByRole('menuitem', { name: /bookmark/i }).click() | ||
|
|
||
| await page.getByRole('button', { name: 'Show bookmarks' }).click() | ||
|
|
||
| await expect( | ||
| sidebar.getByRole('link', { name: bookmarkedNote, exact: true }), | ||
| ).toBeVisible({ timeout: 10000 }) | ||
| await expect( | ||
| sidebar.getByRole('link', { name: regularNote, exact: true }), | ||
| ).not.toBeVisible() | ||
| }) |
There was a problem hiding this comment.
Make each bookmarks test independent from prior test state.
Line 70 assumes a note is already bookmarked by the earlier test. This creates order-coupled behavior and flaky reruns. Seed the bookmark in beforeAll or in each test that relies on it.
Also applies to: 63-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/bookmarks.spec.ts` around lines 44 - 61, The test "bookmark note and
filter by bookmarks" relies on state set by a prior test; make it independent by
seeding the bookmark for bookmarkedNote before assertions—either add a beforeAll
that navigates (use navigateToCampaign) and bookmarks bookmarkedNote via
openContextMenu(page, bookmarkedNote) then clicking the bookmark menu item, or
perform that setup at the start of this test itself (navigateToCampaign,
openContextMenu, click the /bookmark/i menuitem) so the subsequent click on
'Show bookmarks' and the expects for sidebar links (bookmarkedNote and
regularNote) do not depend on test order.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 21-22: The package.json E2E scripts ("test:e2e" and "test:e2e:ui")
rely on Node's --env-file-if-exists flag (added in Node 22.9.0); update
package.json by either pinning engines.node to ">=22.9.0" (or a suitable semver
like ">=22.9.0") to enforce the minimum Node version, or change the two scripts
to invoke the Playwright CLI via the package bin (e.g., use the installed
Playwright binary like "playwright test" via npx or
./node_modules/.bin/playwright) so the Node flag dependency is avoided—choose
one approach and apply it consistently for "test:e2e" and "test:e2e:ui".
🪄 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: e661786f-6231-40d8-832b-0ce64175fb67
📒 Files selected for processing (3)
package.jsonsrc/features/editor/components/viewer/file/file-viewer.tsxsrc/features/sidebar/components/trash-button.tsx
✅ Files skipped from review due to trivial changes (1)
- src/features/editor/components/viewer/file/file-viewer.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
package.json (1)
21-22:⚠️ Potential issue | 🟠 MajorEnforce Node minimum version for
--env-file-if-existsusage.Line 21 and Line 22 rely on a Node CLI flag, but
package.jsondoes not declare anengines.nodeconstraint. This can still cause local/CI version drift.#!/bin/bash # Verify whether runtime requirements for E2E scripts are enforced. set -euo pipefail echo "== package.json: engines + e2e scripts ==" jq '{engines, scripts: {test_e2e: .scripts["test:e2e"], test_e2e_ui: .scripts["test:e2e:ui"]}}' package.json echo echo "== CI workflow Node version references ==" rg -n --iglob '*.yml' --iglob '*.yaml' 'setup-node|node-version|NODE_VERSION|test:e2e|--env-file-if-exists' .github/workflows🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 21 - 22, Add an explicit engines.node constraint in package.json to pin the minimum Node version that supports the --env-file-if-exists flag used by the scripts "test:e2e" and "test:e2e:ui" (e.g. "engines": {"node": ">=18.12.0"}); also update any CI workflow files that set up Node (references to setup-node, node-version, or NODE_VERSION in .github/workflows) to match this engines requirement so local/CI runtimes are consistent with the CLI flag usage.e2e/view-as-player.spec.ts (1)
137-139:⚠️ Potential issue | 🟠 MajorEscape
E2E_PLAYER_EMAILbefore constructingRegExp.Raw email text in
new RegExp(...)can overmatch (e.g.,./+) and make selection nondeterministic.Suggested fix
- const playerItem = page.getByRole('menuitemcheckbox', { - name: new RegExp(E2E_PLAYER_EMAIL!, 'i'), - }) + const escapedEmail = E2E_PLAYER_EMAIL!.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + const playerItem = page.getByRole('menuitemcheckbox', { + name: new RegExp(escapedEmail, 'i'), + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/view-as-player.spec.ts` around lines 137 - 139, The RegExp is built from raw E2E_PLAYER_EMAIL which can contain regex metacharacters and cause overmatching; escape E2E_PLAYER_EMAIL before constructing the RegExp used in the page.getByRole call (where playerItem is defined) so the email is treated as a literal string. Update the expression that creates new RegExp(E2E_PLAYER_EMAIL!, 'i') to first escape regex-special characters in E2E_PLAYER_EMAIL (e.g., a reusable escape helper or inline replace) and then pass the escaped string into RegExp so playerItem selection is deterministic.e2e/file-download.spec.ts (1)
80-88:⚠️ Potential issue | 🟠 MajorAssert at least one download signal fired.
Both waits can resolve to
null, and the test currently still passes. Add an assertion that eitherdownloadorpopupoccurred after clicking.Suggested patch
- const [popup, download] = await Promise.all([ - page.waitForEvent('popup', { timeout: 5000 }).catch(() => null), - page.waitForEvent('download', { timeout: 5000 }).catch(() => null), - downloadItem.click(), - ]) - if (download) { - expect(download.suggestedFilename()).toBeTruthy() - } - if (popup) await popup.close() + const downloadPromise = page + .waitForEvent('download', { timeout: 5000 }) + .catch(() => null) + const popupPromise = page + .waitForEvent('popup', { timeout: 5000 }) + .catch(() => null) + + await downloadItem.click() + const [download, popup] = await Promise.all([downloadPromise, popupPromise]) + + expect(download || popup).not.toBeNull() + if (download) expect(download.suggestedFilename()).toBeTruthy() + if (popup) await popup.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/file-download.spec.ts` around lines 80 - 88, After awaiting Promise.all for page.waitForEvent('popup', ...) and page.waitForEvent('download', ...) around downloadItem.click(), add an assertion that at least one of the signals fired—i.e., assert that either the local variables download or popup is truthy (for example using expect(download || popup).toBeTruthy())—before proceeding to check download.suggestedFilename() or closing popup; update the block that references download, popup, downloadItem.click() accordingly.
🧹 Nitpick comments (4)
convex/editors/__tests__/editors.test.ts (2)
159-179: Tighten the shape test with semantic ownership checks.Current assertions validate shape, but you can make this more meaningful by asserting the editor belongs to the acting user.
✅ Suggested assertion enhancement
expect(result).toMatchObject({ _id: expect.any(String), _creationTime: expect.any(Number), - userId: expect.any(String), + userId: ctx.dm.profile._id, campaignId: ctx.campaignId, sortOrder: expect.any(String), sortDirection: expect.any(String), editorMode: expect.any(String), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/editors/__tests__/editors.test.ts` around lines 159 - 179, The test currently checks the editor shape but doesn't assert ownership; after retrieving result in the test (the code that calls setupCampaignContext and asDm and queries api.editors.queries.getCurrentEditor), add an assertion that result.userId equals the acting user's id from the test context (e.g., ctx.userId or the DM id provided by setupCampaignContext) to ensure the editor belongs to the caller; keep the existing shape assertions and replace or augment expect.any(String) for userId with a strict equality check to ctx's user id.
94-114: Prefer explicit non-null guards overresult!in assertions.Using non-null assertions here can fail with less-informative runtime errors. Add an explicit guard/assertion before field checks.
♻️ Suggested test hardening
const result = await dmAuth.query(api.editors.queries.getCurrentEditor, { campaignId: ctx.campaignId, }) - expect(result!.sortOrder).toBe(SORT_ORDERS.DateModified) + expect(result).not.toBeNull() + if (!result) throw new Error('Expected editor to exist') + expect(result.sortOrder).toBe(SORT_ORDERS.DateModified)const result = await dmAuth.query(api.editors.queries.getCurrentEditor, { campaignId: ctx.campaignId, }) - expect(result!.sortOrder).toBe(SORT_ORDERS.Alphabetical) - expect(result!.sortDirection).toBe(SORT_DIRECTIONS.Descending) - expect(result!.editorMode).toBe(EDITOR_MODE.EDITOR) + expect(result).not.toBeNull() + if (!result) throw new Error('Expected editor to exist') + expect(result.sortOrder).toBe(SORT_ORDERS.Alphabetical) + expect(result.sortDirection).toBe(SORT_DIRECTIONS.Descending) + expect(result.editorMode).toBe(EDITOR_MODE.EDITOR)Also applies to: 116-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/editors/__tests__/editors.test.ts` around lines 94 - 114, Replace the non-null assertion on the query result with an explicit guard/assertion: after calling dmAuth.query(api.editors.queries.getCurrentEditor, {...}) check that result is not null/undefined (e.g., expect(result).toBeTruthy() or expect(result).not.toBeNull()) before asserting result.sortOrder; apply the same pattern for other tests referenced (lines 116-138) that use result! to ensure clearer failure messages and avoid runtime non-null assertion errors in getCurrentEditor-related assertions.e2e/permissions-granularity.spec.ts (1)
66-148: State-coupling concern addressed; consider extracting setup helper to reduce duplication.The conditional state setup blocks now make each test self-contained by ensuring the expected starting state—this addresses the prior flakiness concern. However, the setup pattern is duplicated across all three tests.
Consider extracting a reusable helper:
♻️ Optional helper to reduce duplication
async function ensurePermissionState(page: Page, state: 'none' | 'view' | 'edit') { const row = allPlayersRow(page) const permSelect = row.locator('[data-slot="select-trigger"]') const regex = new RegExp(state, 'i') if (!(await permSelect.textContent())?.match(regex)) { await permSelect.click() await page .getByRole('option', { name: new RegExp(`^${state}$`, 'i') }) .first() .click() await expect(permSelect).toContainText(regex, { timeout: 5000 }) } return permSelect }Then each test simplifies to:
test('set all-players permission to View', async ({ page }) => { // ... navigation ... const permSelect = await ensurePermissionState(page, 'none') await permSelect.click() await page.getByRole('option', { name: /^view$/i }).first().click() await expect(permSelect).toContainText(/view/i, { timeout: 5000 }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/permissions-granularity.spec.ts` around lines 66 - 148, Extract the duplicated conditional setup into a helper function (e.g., ensurePermissionState) that takes Page and state ('none'|'view'|'edit'), uses allPlayersRow and the '[data-slot="select-trigger"]' locator to check permSelect.textContent(), clicks the select and the matching option if needed, waits for the expected text with expect(...).toContainText, and returns the permSelect; then replace the repeated setup blocks in the three tests with calls to ensurePermissionState(page, 'none'|'view'|'edit') and use the returned permSelect for the subsequent clicks and assertions.e2e/view-as-player.spec.ts (1)
162-168: Stop-view locator is too broad and may hit unrelated text.
getByText(/stop|dm|dungeon master/i)can resolve to unrelated content. Prefer a role-based menu item/button locator scoped to the opened menu.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/view-as-player.spec.ts` around lines 162 - 168, The locator stopOption currently uses page.getByText(/stop|dm|dungeon master/i) which is too broad and may match unrelated text; change it to a role-based locator scoped to the opened menu (e.g., find the opened menu container then use getByRole('menuitem' or 'button', { name: /stop|dm|dungeon master/i }) or page.getByRole on the menu with a name matcher) so the selector targets the menu item only; update the code where stopOption is defined (the getByText call) to use the scoped getByRole/getByLabel/getByRole chain against the menu container instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/file-download.spec.ts`:
- Around line 17-18: Initialize the testFilePath variable to null (instead of
leaving it uninitialized) and update the teardown in test.afterAll to guard the
existence check by only calling fs.existsSync(testFilePath) when testFilePath is
not null; change occurrences around test.beforeAll/test.afterAll and any cleanup
block that uses fs.existsSync (referencing the testFilePath identifier and the
teardown hook) so setup failures don’t get masked by teardown errors.
In `@e2e/view-as-player.spec.ts`:
- Around line 42-43: Guard the URL parsing result before destructuring: in the
beforeAll where you call url.match(/\/campaigns\/([^/]+)\/([^/]+)/) (the match
variable used to extract dmUsername and campaignSlug), check if match is
null/undefined and throw a clear, explicit error (including the actual URL)
before performing const [, dmUsername, campaignSlug] = match; so the test fails
fast with a readable message instead of crashing with an opaque error.
- Around line 72-82: The approval click currently uses
dialog.getByRole(...).first() which can act on the wrong request when multiple
pending players exist; scope the approve locator to the specific player row
instead of using .first(): find the player row locator for the target player
(e.g., the variable representing the row or a locator that matches the player's
name/ID), then call rowLocator.getByRole('button', { name: /approve|accept/i }),
wait for that scoped button to be visible (replace .first().waitFor with the
scoped locator.waitFor) and click it; update any references to approveButton,
dialog, or .first() to use the new scoped locator to ensure the approval targets
the correct player.
---
Duplicate comments:
In `@e2e/file-download.spec.ts`:
- Around line 80-88: After awaiting Promise.all for page.waitForEvent('popup',
...) and page.waitForEvent('download', ...) around downloadItem.click(), add an
assertion that at least one of the signals fired—i.e., assert that either the
local variables download or popup is truthy (for example using expect(download
|| popup).toBeTruthy())—before proceeding to check download.suggestedFilename()
or closing popup; update the block that references download, popup,
downloadItem.click() accordingly.
In `@e2e/view-as-player.spec.ts`:
- Around line 137-139: The RegExp is built from raw E2E_PLAYER_EMAIL which can
contain regex metacharacters and cause overmatching; escape E2E_PLAYER_EMAIL
before constructing the RegExp used in the page.getByRole call (where playerItem
is defined) so the email is treated as a literal string. Update the expression
that creates new RegExp(E2E_PLAYER_EMAIL!, 'i') to first escape regex-special
characters in E2E_PLAYER_EMAIL (e.g., a reusable escape helper or inline
replace) and then pass the escaped string into RegExp so playerItem selection is
deterministic.
In `@package.json`:
- Around line 21-22: Add an explicit engines.node constraint in package.json to
pin the minimum Node version that supports the --env-file-if-exists flag used by
the scripts "test:e2e" and "test:e2e:ui" (e.g. "engines": {"node":
">=18.12.0"}); also update any CI workflow files that set up Node (references to
setup-node, node-version, or NODE_VERSION in .github/workflows) to match this
engines requirement so local/CI runtimes are consistent with the CLI flag usage.
---
Nitpick comments:
In `@convex/editors/__tests__/editors.test.ts`:
- Around line 159-179: The test currently checks the editor shape but doesn't
assert ownership; after retrieving result in the test (the code that calls
setupCampaignContext and asDm and queries api.editors.queries.getCurrentEditor),
add an assertion that result.userId equals the acting user's id from the test
context (e.g., ctx.userId or the DM id provided by setupCampaignContext) to
ensure the editor belongs to the caller; keep the existing shape assertions and
replace or augment expect.any(String) for userId with a strict equality check to
ctx's user id.
- Around line 94-114: Replace the non-null assertion on the query result with an
explicit guard/assertion: after calling
dmAuth.query(api.editors.queries.getCurrentEditor, {...}) check that result is
not null/undefined (e.g., expect(result).toBeTruthy() or
expect(result).not.toBeNull()) before asserting result.sortOrder; apply the same
pattern for other tests referenced (lines 116-138) that use result! to ensure
clearer failure messages and avoid runtime non-null assertion errors in
getCurrentEditor-related assertions.
In `@e2e/permissions-granularity.spec.ts`:
- Around line 66-148: Extract the duplicated conditional setup into a helper
function (e.g., ensurePermissionState) that takes Page and state
('none'|'view'|'edit'), uses allPlayersRow and the
'[data-slot="select-trigger"]' locator to check permSelect.textContent(), clicks
the select and the matching option if needed, waits for the expected text with
expect(...).toContainText, and returns the permSelect; then replace the repeated
setup blocks in the three tests with calls to ensurePermissionState(page,
'none'|'view'|'edit') and use the returned permSelect for the subsequent clicks
and assertions.
In `@e2e/view-as-player.spec.ts`:
- Around line 162-168: The locator stopOption currently uses
page.getByText(/stop|dm|dungeon master/i) which is too broad and may match
unrelated text; change it to a role-based locator scoped to the opened menu
(e.g., find the opened menu container then use getByRole('menuitem' or 'button',
{ name: /stop|dm|dungeon master/i }) or page.getByRole on the menu with a name
matcher) so the selector targets the menu item only; update the code where
stopOption is defined (the getByText call) to use the scoped
getByRole/getByLabel/getByRole chain against the menu container instead.
🪄 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: 9f2413bb-ef6e-42a2-9868-62c9a6b53168
📒 Files selected for processing (28)
.github/workflows/ci.ymlconvex/bookmarks/__tests__/bookmarks.test.tsconvex/editors/__tests__/editors.test.tsconvex/notes/__tests__/notes.test.tsconvex/users/__tests__/users.test.tse2e/auth.setup.tse2e/concurrent-operations.spec.tse2e/editor.spec.tse2e/error-states.spec.tse2e/file-download.spec.tse2e/folder-nesting.spec.tse2e/game-maps.spec.tse2e/helpers/editor-helpers.tse2e/helpers/permission-helpers.tse2e/permissions-granularity.spec.tse2e/realtime-collaboration.spec.tse2e/settings.spec.tse2e/sharing.spec.tse2e/sidebar.spec.tse2e/trash.spec.tse2e/view-as-player.spec.tspackage.jsonsrc/features/auth/components/__tests__/sign-in-form.test.tsxsrc/features/campaigns/components/__tests__/campaigns-content.test.tsxsrc/features/campaigns/components/campaign-dialog.tsxsrc/features/campaigns/contexts/__tests__/campaign-context.test.tsxsrc/features/shadcn/components/collapsible.tsxsrc/features/sidebar/components/sidebar-toolbar/close-all-folders.tsx
✅ Files skipped from review due to trivial changes (9)
- src/features/sidebar/components/sidebar-toolbar/close-all-folders.tsx
- e2e/concurrent-operations.spec.ts
- src/features/campaigns/contexts/tests/campaign-context.test.tsx
- e2e/realtime-collaboration.spec.ts
- convex/bookmarks/tests/bookmarks.test.ts
- e2e/sidebar.spec.ts
- convex/notes/tests/notes.test.ts
- convex/users/tests/users.test.ts
- e2e/error-states.spec.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- e2e/sharing.spec.ts
- src/features/shadcn/components/collapsible.tsx
- e2e/settings.spec.ts
- .github/workflows/ci.yml
- src/features/campaigns/components/campaign-dialog.tsx
- e2e/auth.setup.ts
- src/features/campaigns/components/tests/campaigns-content.test.tsx
- e2e/trash.spec.ts
- e2e/editor.spec.ts
- e2e/helpers/permission-helpers.ts
- e2e/folder-nesting.spec.ts
- e2e/game-maps.spec.ts
- e2e/helpers/editor-helpers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
26-33: Don’t mix a resolved pnpm store path with a hardcoded restore path.
actions/cacheincludes the cached path list in the cache version, andpnpm store pathreturns the active store directory. Here the save step uses the resolved store path, but the restore blocks hardcode~/.local/share/pnpm/store; if the active store differs from that default on these runners, the shared cache won’t hit. Reuse one identical store path everywhere, or letsetup-nodeown the pnpm-store cache instead. (github.com)Also applies to: 45-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 26 - 33, The cache save/restore are using different pnpm store paths: the save step writes the resolved path from the pnpm-store step (step id pnpm-store) into GITHUB_OUTPUT and uses that in actions/cache/save@v4, while the restore steps hardcode ~/.local/share/pnpm/store; make them identical by capturing the store path once (keep the pnpm-store step that runs echo "path=$(pnpm store path)" >> "$GITHUB_OUTPUT") and then reference that output variable (steps.pnpm-store.outputs.path) in both the save and restore cache actions, or alternatively remove manual store caching and delegate pnpm store management to setup-node so the same store path is used consistently across save/restore.
10-12: IncludeNODE_VERSIONin the custom cache key.This cache also stores
node_modules, but the key only varies by OS andpnpm-lock.yaml.actions/cachescopes reuse by the explicit key plus a version derived from compression and cached paths, so a futureNODE_VERSIONbump can still restore a stalenode_modulestree from the old runtime. Add${{ env.NODE_VERSION }}to the key and mirror that change in the repeated restore blocks below. (github.com)🔧 Proposed fix
- key: ${{ env.STORE_PATH_KEY }}${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }} + key: ${{ env.STORE_PATH_KEY }}${{ runner.os }}-node${{ env.NODE_VERSION }}-${{ hashFiles('pnpm-lock.yaml') }}Also applies to: 28-33, 45-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 10 - 12, The cache key currently uses OS and pnpm-lock.yaml but omits NODE_VERSION, which can cause restoring node_modules built under a different Node runtime; update the cache key(s) that reference STORE_PATH_KEY (and any restore blocks that repeat the key) to include ${{ env.NODE_VERSION }} (e.g., append or interpolate the NODE_VERSION variable into the key string) and make the same change in the repeated restore/cache steps mentioned (the blocks that currently use STORE_PATH_KEY and pnpm-lock.yaml) so the cache is scoped by Node version as well as OS and lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 7-9: Update the package.json "engines.node" field to match Vite
7's supported minimum by changing the current value ("node": ">=22.9.0") to at
least ">=22.12.0" (or to Vite's exact supported range such as ">=20.19.0 ||
>=22.12.0"); modify the "engines" -> "node" entry so local installs meet Vite 7
requirements and avoid dev/build failures.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 26-33: The cache save/restore are using different pnpm store
paths: the save step writes the resolved path from the pnpm-store step (step id
pnpm-store) into GITHUB_OUTPUT and uses that in actions/cache/save@v4, while the
restore steps hardcode ~/.local/share/pnpm/store; make them identical by
capturing the store path once (keep the pnpm-store step that runs echo
"path=$(pnpm store path)" >> "$GITHUB_OUTPUT") and then reference that output
variable (steps.pnpm-store.outputs.path) in both the save and restore cache
actions, or alternatively remove manual store caching and delegate pnpm store
management to setup-node so the same store path is used consistently across
save/restore.
- Around line 10-12: The cache key currently uses OS and pnpm-lock.yaml but
omits NODE_VERSION, which can cause restoring node_modules built under a
different Node runtime; update the cache key(s) that reference STORE_PATH_KEY
(and any restore blocks that repeat the key) to include ${{ env.NODE_VERSION }}
(e.g., append or interpolate the NODE_VERSION variable into the key string) and
make the same change in the repeated restore/cache steps mentioned (the blocks
that currently use STORE_PATH_KEY and pnpm-lock.yaml) so the cache is scoped by
Node version as well as OS and lockfile.
🪄 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: 6e32efaf-f7ae-4c42-a8fd-3490aeb55ba5
📒 Files selected for processing (2)
.github/workflows/ci.ymlpackage.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
e2e/folder-nesting.spec.ts (1)
134-134: Make close-all toggle state-aware to avoid mode-dependent flakes.Line 134 always clicks a toggle button. If the page starts in close-all mode, this click exits the mode and can invert the assertion intent. Prefer an idempotent setter (same pattern as
setFolderExpansion).Proposed refactor
+async function setCloseAllFoldersMode(page: Page, enabled: boolean) { + const button = page.getByRole('button', { name: /close-all-folders mode/i }) + const label = await button.getAttribute('aria-label') + const isEnabled = /exit close-all-folders mode/i.test(label ?? '') + if (isEnabled !== enabled) { + await button.click() + } +} + test('close all folders hides child', async ({ page }) => { await page.goto('/campaigns') await navigateToCampaign(page, campaignName) @@ - await page.getByRole('button', { name: /close-all-folders mode/i }).click() + await setCloseAllFoldersMode(page, true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/folder-nesting.spec.ts` at line 134, The test currently always clicks the "close-all-folders mode" button which can flip state unexpectedly; change this to an idempotent setter similar to setFolderExpansion: read the button's current state (e.g., aria-pressed or checked) via the same locator used (page.getByRole('button', { name: /close-all-folders mode/i })) and only click when the current state does not match the desired state, or add a helper like setCloseAll(enabled: boolean) that encapsulates this check and uses the locator to click conditionally.e2e/view-as-player.spec.ts (2)
140-145: Escape regex special characters inplayerUsernameto avoid false matches.Email prefixes can contain regex metacharacters (e.g.,
.,+) that could cause unintended matching. For example,john.doewould matchjohnadoesince.matches any character.🛡️ Suggested defensive fix
const playerUsername = E2E_PLAYER_EMAIL!.split('@')[0] + const escapedUsername = playerUsername.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') const playerItem = page.getByRole('menuitemcheckbox', { - name: new RegExp(playerUsername, 'i'), + name: new RegExp(escapedUsername, 'i'), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/view-as-player.spec.ts` around lines 140 - 145, playerUsername may contain regex metacharacters so building a RegExp from it causes false matches; escape regex special characters in playerUsername before constructing the RegExp used in page.getByRole (or switch to a plain text matcher). Update the code that builds playerItem (the RegExp(...) call) to first sanitize playerUsername by escaping characters like . * + ? ^ $ { } ( ) | [ ] \ so the RegExp only matches the literal username.
166-172: Silent try-catch masks failures and makes test state ambiguous.The empty catch block will swallow any error—not just "element not found"—which could hide real issues (timing failures, UI regressions, etc.). After the catch, the test doesn't know whether it successfully exited view-as mode.
Consider a more explicit state check:
♻️ Suggested refactor
- const stopOption = page.getByText(/stop|dm|dungeon master/i) - try { - await expect(stopOption).toBeVisible({ timeout: 3000 }) - await stopOption.click() - } catch { - /* stop option not present */ - } + const stopOption = page.getByRole('menuitem', { name: /stop viewing|exit|dm view/i }) + const isStopVisible = await stopOption.isVisible().catch(() => false) + if (isStopVisible) { + await stopOption.click() + } + // Verify we're back in DM view by checking for view-as button (not stop) + await expect(page.getByRole('button', { name: /view as/i })).toBeVisible({ timeout: 5000 })This approach:
- Uses
isVisible()for conditional logic instead of swallowing exceptions- Adds explicit verification of final state
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/view-as-player.spec.ts` around lines 166 - 172, Replace the silent try/catch around the stop button lookup/click with an explicit visibility check and clear postcondition: use stopOption.isVisible() (or page.getByText(...).isVisible()) to decide whether to click the stopOption, and after the click (or when it was already not visible) add an explicit expect that the view-as mode has exited (e.g., assert the stopOption is not visible or check the known UI state that indicates view-as mode ended). This removes the empty catch, ensures only expected absent-element behavior is tolerated, and makes the final state of view-as mode asserted.package.json (1)
24-25: Consider usingnpx playwrightinstead of direct path to cli.js.The current approach references
./node_modules/@playwright/test/cli.jsdirectly, which is fragile if the package structure changes. Usingnpx playwright testachieves the same result more idiomatically.♻️ Optional: Simplify E2E script invocation
- "test:e2e": "node --env-file-if-exists=.env.local ./node_modules/@playwright/test/cli.js test", - "test:e2e:ui": "node --env-file-if-exists=.env.local ./node_modules/@playwright/test/cli.js test --ui", + "test:e2e": "node --env-file-if-exists=.env.local ./node_modules/.bin/playwright test", + "test:e2e:ui": "node --env-file-if-exists=.env.local ./node_modules/.bin/playwright test --ui",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 24 - 25, The package.json e2e scripts ("test:e2e" and "test:e2e:ui") reference the Playwright CLI via a fragile path "./node_modules/@playwright/test/cli.js"; update these scripts to invoke Playwright idiomatically using "npx playwright test" (preserving the existing node env-file behavior and the "--ui" flag for "test:e2e:ui") so the commands rely on the installed binary instead of the package file path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/folder-nesting.spec.ts`:
- Around line 45-55: The test allocates Playwright resources (context, page) at
the start but only closes them on the happy path; wrap the setup and teardown
blocks that call browser.newContext(), context.newPage(), and the subsequent
actions (createCampaign, navigateToCampaign, createFolder) inside try/finally so
page.close() and context.close() always run; locate the two sections around the
calls to browser.newContext / context.newPage and ensure each uses try { /*
actions */ } finally { if (page) await page.close(); if (context) await
context.close(); } to prevent leaking browser resources.
---
Nitpick comments:
In `@e2e/folder-nesting.spec.ts`:
- Line 134: The test currently always clicks the "close-all-folders mode" button
which can flip state unexpectedly; change this to an idempotent setter similar
to setFolderExpansion: read the button's current state (e.g., aria-pressed or
checked) via the same locator used (page.getByRole('button', { name:
/close-all-folders mode/i })) and only click when the current state does not
match the desired state, or add a helper like setCloseAll(enabled: boolean) that
encapsulates this check and uses the locator to click conditionally.
In `@e2e/view-as-player.spec.ts`:
- Around line 140-145: playerUsername may contain regex metacharacters so
building a RegExp from it causes false matches; escape regex special characters
in playerUsername before constructing the RegExp used in page.getByRole (or
switch to a plain text matcher). Update the code that builds playerItem (the
RegExp(...) call) to first sanitize playerUsername by escaping characters like .
* + ? ^ $ { } ( ) | [ ] \ so the RegExp only matches the literal username.
- Around line 166-172: Replace the silent try/catch around the stop button
lookup/click with an explicit visibility check and clear postcondition: use
stopOption.isVisible() (or page.getByText(...).isVisible()) to decide whether to
click the stopOption, and after the click (or when it was already not visible)
add an explicit expect that the view-as mode has exited (e.g., assert the
stopOption is not visible or check the known UI state that indicates view-as
mode ended). This removes the empty catch, ensures only expected absent-element
behavior is tolerated, and makes the final state of view-as mode asserted.
In `@package.json`:
- Around line 24-25: The package.json e2e scripts ("test:e2e" and "test:e2e:ui")
reference the Playwright CLI via a fragile path
"./node_modules/@playwright/test/cli.js"; update these scripts to invoke
Playwright idiomatically using "npx playwright test" (preserving the existing
node env-file behavior and the "--ui" flag for "test:e2e:ui") so the commands
rely on the installed binary instead of the package file path.
🪄 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: 4d0176f8-19c9-4a07-9e0e-23783e2f7f9d
📒 Files selected for processing (8)
e2e/file-download.spec.tse2e/folder-nesting.spec.tse2e/helpers/campaign-helpers.tse2e/helpers/editor-helpers.tse2e/player-invite.spec.tse2e/trash.spec.tse2e/view-as-player.spec.tspackage.json
✅ Files skipped from review due to trivial changes (2)
- e2e/helpers/editor-helpers.ts
- e2e/player-invite.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/trash.spec.ts
- e2e/helpers/campaign-helpers.ts
Comment on lines
+45
to
+55
| const context = await browser.newContext({ | ||
| storageState: AUTH_STORAGE_PATH, | ||
| }) | ||
| const page = await context.newPage() | ||
| await page.goto('/campaigns') | ||
| await createCampaign(page, campaignName) | ||
| await navigateToCampaign(page, campaignName) | ||
| await createFolder(page, parentFolder) | ||
| await page.close() | ||
| await context.close() | ||
| }) |
There was a problem hiding this comment.
Guard context/page cleanup with try/finally in setup and teardown.
Line 45 and Line 58 allocate browser resources, but cleanup currently runs only on the happy path. If any awaited step throws before explicit close calls, the context/page can leak and destabilize later tests in the worker.
Proposed fix
test.beforeAll(async ({ browser }) => {
const id = Date.now()
parentFolder = `Parent ${id}`
childNote = `Child ${id}`
const context = await browser.newContext({
storageState: AUTH_STORAGE_PATH,
})
const page = await context.newPage()
- await page.goto('/campaigns')
- await createCampaign(page, campaignName)
- await navigateToCampaign(page, campaignName)
- await createFolder(page, parentFolder)
- await page.close()
- await context.close()
+ try {
+ await page.goto('/campaigns')
+ await createCampaign(page, campaignName)
+ await navigateToCampaign(page, campaignName)
+ await createFolder(page, parentFolder)
+ } finally {
+ await page.close()
+ await context.close()
+ }
})
test.afterAll(async ({ browser }) => {
const context = await browser.newContext({
storageState: AUTH_STORAGE_PATH,
})
const page = await context.newPage()
- await page.goto('/campaigns')
try {
- await deleteCampaign(page, campaignName)
- } catch {
- /* best-effort */
+ await page.goto('/campaigns')
+ try {
+ await deleteCampaign(page, campaignName)
+ } catch (error) {
+ console.warn('Best-effort cleanup failed for campaign:', campaignName, error)
+ }
+ } finally {
+ await page.close()
+ await context.close()
}
- await page.close()
- await context.close()
})Also applies to: 58-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/folder-nesting.spec.ts` around lines 45 - 55, The test allocates
Playwright resources (context, page) at the start but only closes them on the
happy path; wrap the setup and teardown blocks that call browser.newContext(),
context.newPage(), and the subsequent actions (createCampaign,
navigateToCampaign, createFolder) inside try/finally so page.close() and
context.close() always run; locate the two sections around the calls to
browser.newContext / context.newPage and ensure each uses try { /* actions */ }
finally { if (page) await page.close(); if (context) await context.close(); } to
prevent leaking browser resources.
This was referenced Apr 1, 2026
Merged
Closed
Merged
This was referenced Apr 12, 2026
Merged
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
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.
Summary by CodeRabbit
Accessibility
Bug Fixes
Tests
Chores