Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Sriya-Mukkawar, your pull request is larger than the review limit of 150000 diff characters
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
|
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 a full Playwright E2E suite: environment-aware global setup and Playwright config, per-product config modules, an auth-capture script, many new product/shared test specs and helpers, npm scripts, .env/.gitignore/README updates, and multiple client components instrumented with Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Dev (runs save script)
participant SaveScript as SaveAuthScript
participant Browser as Playwright Browser
participant App as App (APP_BASE_URL)
participant Google as Google OAuth
participant FS as FileSystem (.auth/)
Dev->>SaveScript: run save-google-auth-state (PRODUCT/APP_BASE_URL)
SaveScript->>Browser: launch Chromium context
Browser->>App: navigate to APP_BASE_URL/login
App->>Browser: render login with Google button
Browser->>App: click Google login button
App->>Google: redirect to Google OAuth
Browser->>Google: perform auth interactions
Google->>App: redirect back to allowed origin
Browser->>App: land on logged-in page
SaveScript->>Browser: capture storageState()
SaveScript->>FS: write `.auth/user[-<product>].json`
FS-->>SaveScript: confirm saved
SaveScript->>Browser: close context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoComprehensive E2E test suite and component testability improvements (TIDY-430)
WalkthroughsDescription• **Comprehensive E2E test suite expansion**: Added extensive Playwright E2E tests covering authentication, navigation, settings, focus workflows (goals/tasks), memory capture, calendar operations, and overview functionality across all three products (Nucleus, Memotron, Pointron) • **Test infrastructure improvements**: Implemented multi-product Playwright configuration with dynamic base URL resolution, auth state persistence per product, and shared helper utilities for common test operations • **Component testability enhancements**: Added data-testid and testId props throughout the codebase to 20+ components including modals, inputs, toggles, navigation items, and search bars for improved test selectability • **Session settings refactoring**: Converted break reminder and PiP toggle from two-way bindings to event handlers with default fallback values in SessionSettings component • **Accessibility improvements**: Added aria-label attributes to top navigation menu items and modal close buttons for better screen reader support • **Configuration and documentation**: Added product-specific E2E configs for Nucleus, Memotron, and Pointron; comprehensive E2E README; environment configuration example; and auth state persistence script • **Minor fixes**: Fixed indentation in session store type definition, updated deprecated typeface property documentation, added allowed host for Pointron local development Diagramflowchart LR
A["Component Testability<br/>data-testid/testId props"] -->|enables| B["E2E Test Suite<br/>Playwright tests"]
C["Auth State<br/>Persistence Script"] -->|supports| B
D["Product Configs<br/>Nucleus/Memotron/Pointron"] -->|configure| B
E["Helper Utilities<br/>Shared test functions"] -->|support| B
B -->|validates| F["Multi-product<br/>Workflows"]
F -->|covers| G["Navigation<br/>Settings<br/>Focus/Memory<br/>Calendar"]
File Changes1. client/components/settings/interactionMode/InteractionModeSettings.svelte
|
Code Review by Qodo
1.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
21 issues found across 71 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/e2e-playwright/tests/shared/navigation.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/navigation.spec.ts:7">
P1: This shared test bypasses Playwright’s per-project `baseURL` resolution by hardcoding `APP_BASE_URL`, which can send `pointron`/`memotron` runs to the wrong origin.</violation>
</file>
<file name="apps/e2e-playwright/tests/pointron/app-nav.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/pointron/app-nav.spec.ts:96">
P2: This navigation bypasses Playwright’s project `baseURL` resolution and may jump to the wrong origin for the pointron project. Use a relative path with `page.goto` so project-specific/auth-adjusted base URL is always honored.</violation>
</file>
<file name="apps/e2e-playwright/scripts/save-google-auth-state.ts">
<violation number="1" location="apps/e2e-playwright/scripts/save-google-auth-state.ts:78">
P2: Do not swallow Google sign-in click failures; this masks real interaction errors and leads to long, misleading timeout failures.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/overview/overview.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/overview/overview.spec.ts:41">
P2: Avoid fixed `waitForTimeout` sleeps; use condition-based waits for deterministic and less flaky E2E tests.</violation>
<violation number="2" location="apps/e2e-playwright/tests/shared/overview/overview.spec.ts:96">
P1: `Focus` assertions are not scoped to the metric card area, so they can match the tab label and let tests pass without validating the intended widget.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts:38">
P2: Do not suppress Playwright assertion failures with `.catch(() => null)`; let the test fail when the expected UI state is not reached.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/settings/settings.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/settings/settings.spec.ts:37">
P2: The settings-open workflow is duplicated across multiple local helper functions; extract a shared helper to avoid maintenance drift and inconsistent future updates.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/memory/node/creation.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/memory/node/creation.spec.ts:22">
P2: Both node-creation regression tests are hard-skipped, so this new regression spec contributes zero executable coverage.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/calendar/calendar.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/calendar/calendar.spec.ts:18">
P2: Avoid fixed `waitForTimeout` sleeps; wait for concrete UI state instead to reduce E2E flakiness and runtime.</violation>
<violation number="2" location="apps/e2e-playwright/tests/shared/calendar/calendar.spec.ts:59">
P1: Do not swallow failed `expect` assertions; let the test fail when the expected UI state is not reached.</violation>
</file>
<file name="apps/e2e-playwright/tests/nucleus/app-nav.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/nucleus/app-nav.spec.ts:146">
P1: This navigation bypasses the Playwright project `baseURL` resolution and can send the test to the wrong origin. Use a relative `page.goto` so it always honors the configured project base URL/auth-adjusted origin.</violation>
</file>
<file name="apps/e2e-playwright/tests/memotron/settings/settings.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/memotron/settings/settings.spec.ts:29">
P2: Hard-coded `waitForTimeout` calls make this test flaky and slower. Replace them with condition-based waits (locators/assertions) so the test waits only as long as needed.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/collection/collection.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/collection/collection.spec.ts:22">
P2: This regression suite currently has no runnable tests because the only test is unconditionally skipped, so it provides zero coverage and leaves the setup code unreachable.</violation>
</file>
<file name="apps/e2e-playwright/tests/pointron/settings/settings.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/pointron/settings/settings.spec.ts:29">
P2: Avoid fixed `waitForTimeout(500)` delays in the test flow; use state-based waiting/assertions instead to reduce flakiness and unnecessary runtime.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts:22">
P2: The only regression test is unconditionally skipped, so this new spec never executes and provides no coverage.</violation>
</file>
<file name="apps/e2e-playwright/tests/memotron/app-nav.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/memotron/app-nav.spec.ts:96">
P1: Fallback navigation uses a hardcoded env-derived base URL instead of Playwright’s configured project baseURL, which can navigate to the wrong origin for memotron.</violation>
<violation number="2" location="apps/e2e-playwright/tests/memotron/app-nav.spec.ts:137">
P1: The success condition is overly broad (`/Nodes/i`) and can match unrelated visible text, so the test can pass without verifying that the nodes list content is present.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts:67">
P2: Avoid fixed `waitForTimeout` sleeps; wait for specific UI state/assertions to keep tests stable and faster.</violation>
<violation number="2" location="apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts:82">
P3: Extract the repeated timeline verification flow into a shared helper to reduce duplication and maintenance drift.</violation>
</file>
<file name="apps/e2e-playwright/tests/shared/memory/capture.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/memory/capture.spec.ts:33">
P2: The second test duplicates the full capture-editor interaction flow; extract shared steps into a helper to avoid maintenance drift.</violation>
<violation number="2" location="apps/e2e-playwright/tests/shared/memory/capture.spec.ts:63">
P1: Add a post-save assertion; ending with a timeout allows false-positive passes when save does not actually succeed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const iconVisible = await settingsIconBtn.isVisible().catch(() => false); | ||
| if (iconVisible) { | ||
| await settingsIconBtn.click({ timeout: 5_000 }); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
P2: Avoid fixed waitForTimeout(500) delays in the test flow; use state-based waiting/assertions instead to reduce flakiness and unnecessary runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/pointron/settings/settings.spec.ts, line 29:
<comment>Avoid fixed `waitForTimeout(500)` delays in the test flow; use state-based waiting/assertions instead to reduce flakiness and unnecessary runtime.</comment>
<file context>
@@ -0,0 +1,49 @@
+ const iconVisible = await settingsIconBtn.isVisible().catch(() => false);
+ if (iconVisible) {
+ await settingsIconBtn.click({ timeout: 5_000 });
+ await page.waitForTimeout(500);
+ return;
+ }
</file context>
| }); | ||
| }); | ||
|
|
||
| test.skip("bulk edit goals (select multiple, apply action)", async ({ page }) => { |
There was a problem hiding this comment.
P2: The only regression test is unconditionally skipped, so this new spec never executes and provides no coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts, line 22:
<comment>The only regression test is unconditionally skipped, so this new spec never executes and provides no coverage.</comment>
<file context>
@@ -0,0 +1,26 @@
+ });
+ });
+
+ test.skip("bulk edit goals (select multiple, apply action)", async ({ page }) => {
+ await ensureInAppOnHome(page);
+ // TODO: select multiple goals in library, open bulk editor, apply action and assert
</file context>
|
|
||
| await runCommand(page, "Capture"); | ||
|
|
||
| await page.waitForTimeout(1_000); |
There was a problem hiding this comment.
P2: The second test duplicates the full capture-editor interaction flow; extract shared steps into a helper to avoid maintenance drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/shared/memory/capture.spec.ts, line 33:
<comment>The second test duplicates the full capture-editor interaction flow; extract shared steps into a helper to avoid maintenance drift.</comment>
<file context>
@@ -0,0 +1,109 @@
+
+ await runCommand(page, "Capture");
+
+ await page.waitForTimeout(1_000);
+ const editor = page
+ .getByTestId("capture-editor")
</file context>
| await page.waitForTimeout(1_500); | ||
|
|
||
| await page | ||
| .locator("#app-menu") |
There was a problem hiding this comment.
P3: Extract the repeated timeline verification flow into a shared helper to reduce duplication and maintenance drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts, line 82:
<comment>Extract the repeated timeline verification flow into a shared helper to reduce duplication and maintenance drift.</comment>
<file context>
@@ -0,0 +1,262 @@
+ await page.waitForTimeout(1_500);
+
+ await page
+ .locator("#app-menu")
+ .getByRole("button", {
+ name: new RegExp(`^${nucleusProductConfig.timelinePageLabel}$`, "i")
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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)
apps/e2e-playwright/global-setup.ts (1)
28-31:⚠️ Potential issue | 🟠 MajorKill the spawned Vite process on startup timeout.
On timeout, the promise rejects but the dev-server process may keep running. This can leak processes and destabilize subsequent E2E runs.
🛠️ Proposed fix
const timeout = setTimeout(() => { if (!resolved) { + resolved = true; + viteProcess.kill("SIGTERM"); reject(new Error("Timeout waiting for dev server to start")); } }, 60000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/global-setup.ts` around lines 28 - 31, When the startup timeout fires in the timeout handler (see timeout and resolved), ensure the spawned Vite dev-server child process is killed before rejecting: grab the child process reference created when starting the dev server (e.g., viteProcess or devServerProcess), check it's still running, call its kill() (and optionally kill with SIGKILL if necessary) and await/handle its exit, then reject the promise with the timeout error; this prevents leaking the Vite process on startup timeout.
🟡 Minor comments (21)
apps/e2e-playwright/.env.example-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd trailing newline to satisfy dotenv-linter.
Line 1 currently triggers
EndingBlankLine.Proposed fix
APP_BASE_URL=https://local.nucleus.to +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/.env.example` at line 1, The file containing the APP_BASE_URL entry is missing a trailing newline which triggers dotenv-linter's EndingBlankLine rule; open the file that defines APP_BASE_URL (the line with "APP_BASE_URL=https://local.nucleus.to") and add a single newline character at the end of the file so the file ends with a blank line, then save and re-run the linter to confirm the EndingBlankLine warning is resolved.apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts-13-25 (1)
13-25:⚠️ Potential issue | 🟡 MinorPlease keep scaffold text ASCII-only and remove inline TODO comments.
Line 13 contains a non-ASCII dash, and Line 24 uses an inline TODO comment. Please normalize to ASCII and move pending-task tracking out of inline comments.
I can provide a quick cleanup patch for this file if helpful.
As per coding guidelines: "Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts` around lines 13 - 25, Replace the non-ASCII dash in the test.describe title ("goal – bulk editor `@regression`") with a plain ASCII hyphen so the string reads "goal - bulk editor `@regression`", and remove the inline TODO comment inside the test.skip block; instead record the pending task outside the code (e.g., update PR description or issue tracker) and leave the test.skip with a brief ASCII-only scaffold message or an explicit skip reason tied to the test name (refer to the test.describe title and the test.skip block that calls ensureInAppOnHome).apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.ts-13-33 (1)
13-33:⚠️ Potential issue | 🟡 MinorClean up non-ASCII punctuation and inline TODO comments in this scaffold.
The suite title uses non-ASCII punctuation, and Lines 25/32 contain inline TODO comments. Please use ASCII-only text and remove inline TODOs from committed code.
Happy to provide a cleanup diff for this spec.
As per coding guidelines: "Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.ts` around lines 13 - 33, Replace the non-ASCII punctuation in the top-level suite title string passed to test.describe and remove the inline "TODO" comments inside the skipped tests; specifically update the title "task – context menu (all actions) `@regression`" to use an ASCII hyphen (e.g., "task - context menu (all actions) `@regression`") and delete the inline TODO comments within the two skipped tests that call ensureInAppOnHome in the "from library" and "from record page" test.skip blocks so the scaffold contains only ASCII text and no inline TODO annotations.apps/e2e-playwright/tests/shared/memory/node/creation.spec.ts-13-30 (1)
13-30:⚠️ Potential issue | 🟡 MinorUse ASCII test titles and remove inline TODO comments.
Line 13 uses a non-ASCII dash (
–), and Lines 24/29 add inline// TODOcomments. Please switch to ASCII-and track pending work outside inline comments (for example in issue tracking).I can draft a quick patch to normalize the titles and remove the inline TODOs if you want.
As per coding guidelines: "Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/node/creation.spec.ts` around lines 13 - 30, Replace the non-ASCII en dash in the test.describe title and the two test titles used in the test.skip blocks with a plain ASCII hyphen (change the “node – creation flows `@regression`” and any other titles containing “–” to use “-”), and remove the inline "// TODO" comments inside the two test.skip blocks; track the pending work externally instead. Locate the strings in the test.describe(...) and test.skip(...) calls and update the literal titles and delete the inline comments in creation.spec.ts.apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts-13-24 (1)
13-24:⚠️ Potential issue | 🟡 MinorPlease normalize placeholder text to ASCII and remove inline TODO.
Line 13 uses a non-ASCII dash, and Line 24 contains an inline TODO comment. Please keep this file ASCII-only and avoid inline comments in modified code.
I can prepare a small follow-up patch for this cleanup.
As per coding guidelines: "Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts` around lines 13 - 24, The test title contains a non-ASCII en-dash and there's an inline "// TODO" comment to remove: change the string in test.describe from "task – bulk editor `@regression`" to use an ASCII hyphen ("task - bulk editor `@regression`") and remove the inline comment following ensureInAppOnHome(page); (the "// TODO" after the call inside the test.skip block). Ensure no other non-ASCII characters were introduced and avoid adding inline comments in the modified lines.apps/e2e-playwright/tests/shared/focus/task/record-page.spec.ts-13-29 (1)
13-29:⚠️ Potential issue | 🟡 MinorAlign this spec scaffold with ASCII-only and no-inline-comment rules.
Line 13 includes non-ASCII punctuation, and Lines 24/29 add inline TODO comments. Please replace with ASCII text and remove inline TODOs.
If useful, I can generate a quick cleanup diff for this file.
As per coding guidelines: "Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/task/record-page.spec.ts` around lines 13 - 29, Replace the non-ASCII punctuation in the test.describe title (the en dash “–”) with an ASCII hyphen and remove the inline TODO comments inside the two test.skip blocks ("open task record page and assert panels/expected content visible" and "tab switching and visibility check on task record page"); leave the test bodies empty or add proper multi-line TODO placeholders (not inline) if you need a marker, but do not introduce any new inline comments and ensure all edited text is ASCII-only.apps/e2e-playwright/tests/pointron/settings/settings.spec.ts-13-25 (1)
13-25:⚠️ Potential issue | 🟡 MinorNormalize to ASCII and avoid inline comments in this TS spec.
Line 14 uses non-ASCII punctuation (
–), and Line 24 introduces an inline comment. Please keep these edits ASCII-only and comment-free per repo rules.As per coding guidelines: "
Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/pointron/settings/settings.spec.ts` around lines 13 - 25, The test file contains a non-ASCII em dash in the suite title and an inline comment inside the openSettings helper; replace the em dash with a plain ASCII hyphen in the test.describe title and remove the inline comment before the settingsIconBtn declaration in the openSettings function (symbols to edit: test.describe("pointron – settings ..." -> use hyphen, and the openSettings function where settingsIconBtn is declared) so the change is ASCII-only and contains no inline comments.apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts-13-14 (1)
13-14:⚠️ Potential issue | 🟡 MinorUse ASCII punctuation in suite titles/comments.
Line 14 includes a non-ASCII dash (
–). Please replace with ASCII-to match repo edit rules.As per coding guidelines: "
Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts` around lines 13 - 14, The suite title string passed to test.describe contains a non-ASCII en-dash (`–`) — update the string in the test.describe(...) call (the suite title "nucleus – settings (product-specific) `@regression`") to use an ASCII hyphen `-` ("nucleus - settings (product-specific) `@regression`"); also scan the adjacent comment and replace any en-dash occurrences with an ASCII hyphen to keep edits ASCII-only, leaving all other text and formatting unchanged.apps/e2e-playwright/tests/shared/focus/goal/context-menu.spec.ts-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorPlease align with TS file edit rules (ASCII-only + no inline comments)
The suite title uses a non-ASCII dash, and TODO inline comments were introduced in test bodies.
As per coding guidelines,
**/*.{ts,tsx,svelte,js,jsx}: “Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.”Also applies to: 25-25, 32-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/context-menu.spec.ts` at line 13, The test suite title string in test.describe uses a non-ASCII en-dash — replace it with a plain ASCII hyphen so the title reads "goal - context menu (all actions) `@regression`"; also remove any inline TODO comments introduced inside the test bodies (referenced around the existing test cases at the lines near the test.describe and the tests around 25 and 32) so no inline comments remain and only ASCII characters are added; keep the rest of the test code unchanged.apps/e2e-playwright/config/memotron-product.config.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorRemove inline comments in modified TS file
This file introduces inline/doc comments in changed code, which conflicts with repository rules for TS edits.
As per coding guidelines,
**/*.{ts,tsx,svelte,js,jsx}: “never introduce inline comments when modifying code.”Also applies to: 7-7, 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/config/memotron-product.config.ts` around lines 1 - 4, Remove the inline JSDoc/block comments introduced in this TypeScript change (the /** ... */ comment at the top and any inline comments around the modified code at the other noted locations) so the modified TS file contains no new inline/doc comments; if the information is required, move it to the PR description or external documentation rather than leaving /** ... */ or // comments in the source.apps/e2e-playwright/scripts/save-google-auth-state.ts-25-26 (1)
25-26:⚠️ Potential issue | 🟡 MinorRemove inline comments from modified TS code.
These newly introduced inline comments conflict with the repository rule for edited TS/JS files.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.Also applies to: 76-77, 84-86, 92-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/scripts/save-google-auth-state.ts` around lines 25 - 26, The inline comment introduced above the allowedOrigins array (and similar inline comments around lines referenced: 76-77, 84-86, 92-93) violates the repo rule; remove those inline comments so the modified TypeScript file contains no newly added inline comments, leaving only code and/or block comments that preexisted, and keep ASCII-only edits; locate symbols like allowedOrigins to find the exact spots to delete the inline comment text.apps/e2e-playwright/tests/pointron/app-nav.spec.ts-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorUse ASCII-only suite labels.
The describe title includes a non-ASCII dash; replace it with ASCII
-.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts` at line 15, Replace the non-ASCII dash in the test suite label inside the test.describe call (the string "pointron – app layout and menu `@regression`") with a standard ASCII hyphen so it reads "pointron - app layout and menu `@regression`"; update the literal string in the test.describe invocation accordingly without adding comments or other changes.apps/e2e-playwright/tests/shared/overview/overview.spec.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorUse ASCII-only suite names.
The describe title includes a non-ASCII dash character. Replace it with an ASCII
-.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts` at line 18, The test suite title in the test.describe call contains a non-ASCII dash; update the string in the test.describe("overview – Focus widgets and tabs (verify multiple elements) `@regression`", ...) to use a plain ASCII hyphen (-) so it reads "overview - Focus widgets and tabs (verify multiple elements) `@regression`". Ensure you only change the dash character inside the test.describe title and do not add inline comments or other edits.apps/e2e-playwright/config/nucleus-product.config.ts-7-14 (1)
7-14:⚠️ Potential issue | 🟡 MinorRemove inline property comments in this TS config.
These comments were introduced in modified code and conflict with the project rule for TS/JS edits.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/config/nucleus-product.config.ts` around lines 7 - 14, Remove the inline property comments next to appMenuNavLabels, includeHomeInNav, timelinePageLabel, and pathByNavLabel in the TS config and replace them with either a single top-of-file JSDoc/block comment or no comment at all so no inline comments are added to property lines; ensure the properties remain unchanged (types and values intact) and that any necessary documentation is moved to a block comment above the config object rather than trailing each property.apps/e2e-playwright/tests/nucleus/app-nav.spec.ts-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorReplace non-ASCII punctuation in test names with ASCII equivalents.
The modified titles include non-ASCII characters (
–,→). Please use ASCII (-,->) to satisfy repository constraints.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` at line 15, Replace non-ASCII punctuation in the test titles: locate the test.describe call with the title "nucleus – app layout and menu `@regression`" and any individual test title that uses the right-arrow symbol (→) around line 38, and replace the en-dash (–) with a plain hyphen (-) and the right-arrow (→) with ASCII "->" so both test names use only ASCII punctuation; keep the rest of the strings unchanged and do not add inline comments.apps/e2e-playwright/tests/shared/memory/capture.spec.ts-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorUse ASCII-only suite labels.
The suite title includes a non-ASCII dash character. Please switch to plain ASCII (
-) to match repository constraints.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/capture.spec.ts` at line 13, The test suite title passed to test.describe contains a non-ASCII dash; update the string in the test.describe call (the suite label for "memory – capture `@regression`") to use a plain ASCII hyphen ("memory - capture `@regression`") so the suite label is ASCII-only; ensure you only change the dash character inside the test.describe invocation and do not add inline comments or other edits.apps/e2e-playwright/scripts/save-google-auth-state.ts-52-53 (1)
52-53:⚠️ Potential issue | 🟡 MinorKeep user-facing strings ASCII-only in this change.
The log message currently uses a non-ASCII dash character. Please replace it with
-.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/scripts/save-google-auth-state.ts` around lines 52 - 53, Replace the non-ASCII dash character in the two console.log messages with a standard ASCII hyphen (-); update the string in the first console.log ("Opening browser – complete Google sign-in once. We save as soon as you're back on the app.") to use "-" and likewise ensure the second message remains unchanged except for any non-ASCII dash, keeping all other text intact and without adding comments or other changes to the console.log calls.apps/e2e-playwright/tests/shared/navigation.spec.ts-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorUse ASCII-only describe text.
The suite title currently includes a non-ASCII dash character. Please replace with ASCII
-.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/navigation.spec.ts` at line 14, The test suite title in the call to test.describe uses a non-ASCII dash; replace the non-ASCII dash character in the string passed to test.describe (the suite title "shared – auth and nav `@regression`") with an ASCII hyphen "-" so the title becomes "shared - auth and nav `@regression`" and ensure no other characters or inline comments are changed.apps/e2e-playwright/global-setup.ts-77-83 (1)
77-83:⚠️ Potential issue | 🟡 MinorAvoid inline comments in modified TS sections.
This change adds inline comments in edited code paths. Please remove them to align with repo standards.
As per coding guidelines:Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code.Also applies to: 103-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/global-setup.ts` around lines 77 - 83, Remove the inline comment lines introduced in the modified TypeScript sections of global-setup.ts (specifically around the APP_BASE_URL handling where process.env.APP_BASE_URL, baseURL and authDir are used) and also remove the inline comments at the other modified spot (around lines referencing saved auth state / the other noted lines). Keep only code and regular non-inline comments (or move explanatory text to surrounding block comments or commit message) so the edited areas contain no new inline comments per repo standards.apps/e2e-playwright/tests/shared/navigation.spec.ts-2-3 (1)
2-3:⚠️ Potential issue | 🟡 MinorReplace hardcoded nucleus config with product-aware resolver in shared test suite.
This shared test imports
nucleusProductConfigdirectly, creating a structural coupling even thoughhomePathcurrently matches across all three products (nucleus, pointron, memotron). If config values diverge in the future, the test will fail for pointron and memotron. Switch to a dynamic config resolver that detects the active product at runtime instead of importing a product-specific config.Additionally, replace the em-dash (–) on line 14 with an ASCII hyphen (-) to comply with the ASCII-only guideline for edits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/navigation.spec.ts` around lines 2 - 3, Replace the hardcoded import of nucleusProductConfig with a runtime product-aware resolver (e.g. use a function named resolveActiveProductConfig or getActiveProductConfig from a shared config resolver module) and use the returned config.homePath wherever nucleusProductConfig.homePath is referenced in navigation.spec.ts; this removes static coupling to nucleus and ensures the correct homePath for pointron/memotron at runtime. Update the import and any references to use the resolver's result (e.g., const cfg = resolveActiveProductConfig(); use cfg.homePath). Also replace the non-ASCII em-dash character (–) used in the test string with a regular ASCII hyphen (-).apps/e2e-playwright/tests/shared/settings/settings.spec.ts-1464-1464 (1)
1464-1464:⚠️ Potential issue | 🟡 Minor65-second hardcoded wait significantly slows test execution.
await page.waitForTimeout(65_000)waits over a minute. While this may be necessary to test the break reminder after 1 minute, consider:
- Mocking the timer/clock in the app to speed up tests
- Documenting why this long wait is necessary in the test description
- Tagging this test separately so it can be excluded from quick feedback loops
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/settings/settings.spec.ts` at line 1464, The hardcoded 65s wait using page.waitForTimeout(65_000) slows the suite; replace it by mocking the app timer or advancing a fake clock instead of sleeping, or if mocking isn't possible, add a clear comment explaining why 65s is required and mark the test with a slow/integration tag so it can be excluded from fast runs; locate the usage of page.waitForTimeout in this spec (the await page.waitForTimeout(65_000) call) and either (a) integrate the app's clock-mocking helper or Playwright's time manipulation to fast-forward the reminder interval, or (b) reduce the timeout and add a test annotation/comment documenting the reason and tagging the test as slow.
🧹 Nitpick comments (22)
client/components/modal/ModalLayout.svelte (1)
196-202: Consider adding similar attributes to the back button for consistency.The cantilever close button now has
type="button",aria-label, anddata-testid, but the adjacent back button lacks these. For consistency and completeness, consider applying the same pattern here.🔧 Suggested enhancement
{`#if` params.layout?.isShowBackButton} <button + type="button" + aria-label="Go back" + data-testid="modal-back" class="absolute top-16 -right-10 bg-bgs4 w-10 h-12 rounded-r-md flex justify-center items-center hover:brightness-110" on:click={() => appStore.goBack(resource)} use:tooltip={{ text: "Go back", direction: Placement.Left }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/modal/ModalLayout.svelte` around lines 196 - 202, Add the same accessibility and test attributes to the back button as the cantilever close button: update the button that calls appStore.goBack(resource) (the one containing Icon icon="back" and Size.lg) to include type="button", an appropriate aria-label such as "Go back", and a data-testid value (e.g. "modal-back-button") so the element is consistent for accessibility and testing.apps/e2e-playwright/tests/shared/memory/node/context-menu.spec.ts (1)
22-30: Remove inline TODO comments per coding guidelines.The
// TODOinline comments on lines 24 and 29 violate the coding guideline that prohibits introducing inline comments. Consider removing them or tracking outstanding work via a separate issue/ticket instead.As per coding guidelines:
**/*.{ts,tsx,svelte,js,jsx}: "never introduce inline comments when modifying code."♻️ Proposed fix
test.skip("context menu from library", async ({ page }) => { await ensureInAppOnHome(page); - // TODO }); test.skip("context menu from record page", async ({ page }) => { await ensureInAppOnHome(page); - // TODO });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/node/context-menu.spec.ts` around lines 22 - 30, Remove the inline `// TODO` comments in the skipped tests `test.skip("context menu from library", ...)` and `test.skip("context menu from record page", ...)` in context-menu.spec.ts; either delete the comments entirely or replace them with a non-inline alternative such as adding a skip reason string (e.g., test.skip("context menu from library", "work tracked in ISSUE-123", ...)) or create an issue/ticket and reference its ID in the skip message so no inline `// TODO` remains.client/products/pointron/settings/SessionSettings.svelte (1)
18-21: Validate break reminder values before persisting.Current guard only filters null/undefined. Invalid numbers (NaN, 0, negative) can still be persisted.
Proposed hardening
function onBreakReminderChange(event: CustomEvent<{ value: number }>) { const value = event.detail?.value; - if (value != null) pointronPreferences.modify({ breakReminder: value }); + if (typeof value === "number" && Number.isFinite(value) && value > 0) { + pointronPreferences.modify({ breakReminder: value }); + } }Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/products/pointron/settings/SessionSettings.svelte` around lines 18 - 21, The onBreakReminderChange handler currently only checks for nullish values and can persist NaN, 0, or negatives; update onBreakReminderChange to ensure the extracted value is a finite number greater than zero (e.g., use Number.isFinite(value) && value > 0) before calling pointronPreferences.modify({ breakReminder: value }), and apply the same validation to the equivalent reminder change handler elsewhere in the file so invalid values are not persisted.apps/e2e-playwright/tests/shared/focus/task/creation.spec.ts (2)
82-92: Consider adding a testId to the task list toolbar for more robust selection.The current locator chain relying on text content ("By month", "No tasks found") is fragile and may break if UI text changes. A dedicated
testIdon the toolbar container would improve maintainability:// Fragile: const taskListToolbar = page.locator("div").filter({ has: page.getByRole("button", { name: /By month/i }) })... // More robust: const taskListToolbar = page.getByTestId("task-list-toolbar");This would require adding a corresponding
testIdto the client component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/task/creation.spec.ts` around lines 82 - 92, The locator for taskListToolbar is fragile because it chains filters on visible text ("By month", "No tasks found"); add a stable test id on the toolbar component (e.g., data-testid="task-list-toolbar") and update the test to use page.getByTestId("task-list-toolbar") to locate the toolbar, then get the plus button via the existing plusIconInHeader reference (taskListToolbar.getByRole("button").last()) and keep the click call; update the client component to render the corresponding data-testid attribute.
43-43: Consider replacing fixedwaitForTimeoutcalls with explicit wait conditions.Fixed delays (
waitForTimeout(500),waitForTimeout(1_500), etc.) can make tests flaky across different environments and slow down CI. Where possible, prefer explicit waits tied to observable state changes:
waitForSelectorfor element visibilitywaitForResponsefor API completionwaitForLoadStatefor page stabilityIf these delays are specifically for UI animations/transitions, consider adding a brief comment explaining the intent, or wrapping them in a helper like
waitForAnimationSettle()for clarity.Also applies to: 55-55, 80-80, 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/task/creation.spec.ts` at line 43, Replace the fixed sleep calls (e.g., await page.waitForTimeout(500) and the other occurrences at the same file) with explicit waits tied to observable state: use page.waitForSelector(...) when waiting for an element to appear/visible, page.waitForResponse(...) or page.waitForRequest(...) for network activity, or page.waitForLoadState(...) for navigation/idle; if the delays are to wait for UI animations, replace them with a small helper (e.g., waitForAnimationSettle(selector)) or add a comment and keep a minimal timeout — update the calls in tests/shared/focus/task/creation.spec.ts that reference waitForTimeout to use these explicit alternatives.apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts (1)
1-26: LGTM!Placeholder test structure is consistent with the PR's test patterns. The skeleton provides a clear starting point for implementing bulk editor tests.
Do you want me to help implement the bulk edit test logic, or open an issue to track this TODO?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts` around lines 1 - 26, The test file currently contains a skipped placeholder test ("bulk edit nodes") with a TODO; remove the test.skip on the "bulk edit nodes" test, implement the end-to-end steps inside that test to exercise the bulk editor flow (use ensureInAppOnHome(page) to land on the app, keep the existing page.route stub for network filtering, then add actions to open the bulk editor UI, select multiple nodes, apply edits, and assert expected DOM/network changes), and ensure assertions cover both UI state and any relevant API calls; reference the test title "bulk edit nodes", the helper ensureInAppOnHome, and the existing page.route logic when locating where to implement the test.apps/e2e-playwright/tests/shared/memory/node/record-page.spec.ts (1)
1-31: LGTM!Consistent placeholder structure. The test names clearly outline the intended coverage for node record page functionality.
Do you want me to help implement these record page tests, or open an issue to track the TODOs?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/node/record-page.spec.ts` around lines 1 - 31, The two tests in test.describe are placeholders using test.skip and need real implementations or an issue to track them: remove test.skip for the tests titled "open node record and assert panels/content visible" and "tab switching and visibility on node record page" (or create a tracked issue if you prefer), keep the existing test.beforeEach routing setup, call ensureInAppOnHome(page) at the start of each test, then programmatically open a node record (use the app-specific selector for a record row/button), assert visibility of expected panels/content using page.getByTestId / page.locator assertions, and implement tab-switching by clicking the tab controls and asserting corresponding panel visibility; reference ensureInAppOnHome, test.skip, test.describe, and test.beforeEach to locate where to change the code.apps/e2e-playwright/tests/pointron/settings/settings.spec.ts (1)
29-33: Replace fixed sleeps with state-based waits to improve test reliability.The
waitForTimeout(500)calls on lines 29 and 33 make the test timing-sensitive. After clicking the Settings button or running the Settings command, wait for a concrete Settings UI signal (e.g., modal/dialog appearance) instead of an arbitrary delay.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/pointron/settings/settings.spec.ts` around lines 29 - 33, The test uses fixed 500ms sleeps after running the Settings command (the calls to page.waitForTimeout(500) surrounding runCommand(page, "Settings")), which is fragile; replace both sleeps with state-based waits that wait for a concrete Settings UI signal (for example wait for the Settings modal/dialog or a settings panel element to be visible or a specific text/button within it) after calling runCommand(page, "Settings") and after the early-return branch, using Playwright's waitForSelector/waitForRole/waitForResponse as appropriate to detect the settings UI instead of sleeping.apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts (1)
31-31: Replace fixed timeout with deterministic UI wait on the settings modal.The
waitForTimeout(500)on line 31 is brittle in CI. Instead, wait for the Settings modal/dialog element to become visible. Concrete selectors likepage.getByRole("dialog")orpage.getByTestId("modal-close")are available (as evidenced by similar patterns inshared/settings/settings.spec.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts` at line 31, Replace the brittle page.waitForTimeout(500) call with a deterministic wait for the Settings modal to appear: remove page.waitForTimeout and wait for a stable selector (e.g. page.getByRole("dialog") or page.getByTestId("modal-close")) to become visible using the Playwright wait/assert API (e.g. .waitFor or expect(...).toBeVisible()) so the test proceeds only after the settings dialog rendered; update the test in settings.spec.ts where page.waitForTimeout is used and target the modal selectors shown in shared/settings/settings.spec.ts.apps/e2e-playwright/tests/shared/overview/overview.spec.ts (1)
31-41: Deduplicate the repeated “open Overview” setup sequence.The same prelude (
ensureInAppOnHome+runCommand("Overview")+ URL wait + fixed wait) is repeated in nearly every test case. Extracting it into one helper will make this suite easier to update and stabilize.Also applies to: 72-83, 112-123, 152-163, 182-193, 209-220, 251-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts` around lines 31 - 41, The test file repeats the same "open Overview" setup (ensureInAppOnHome + runCommand(page, "Overview") + waiting for URL using nucleusProductConfig.pathByNavLabel.Overview + fixed timeout); extract that sequence into a single helper function (e.g., openOverview(page) or navigateToOverview) that encapsulates ensureInAppOnHome, runCommand(page, "Overview"), the page.waitForURL predicate referencing nucleusProductConfig.pathByNavLabel.Overview, and the page.waitForTimeout(1_500), then replace every repeated block (lines around ensureInAppOnHome/runCommand/waitForURL/waitForTimeout at the start of each test) with a single call to the new helper to deduplicate and centralize the logic.apps/e2e-playwright/tests/shared/memory/capture.spec.ts (1)
33-64: Extract the duplicated capture-edit-save flow into a helper.Both tests duplicate the same editor detection, markdown toggle, typing, and save sequence. Pulling that into one helper will reduce maintenance cost and test drift.
♻️ Proposed refactor
+async function fillCaptureAndSave(page, captureText: string) { + await page.waitForTimeout(1_000); + const editor = page + .getByTestId("capture-editor") + .getByPlaceholder("Start typing to capture...") + .or( + page + .getByTestId("capture-editor") + .getByRole("textbox", { name: /Markdown editor|Start typing/i }) + ) + .first(); + const markdownBtn = page.getByRole("button", { name: /^Markdown$/i }).first(); + const editorVisible = await editor.isVisible().catch(() => false); + if (!editorVisible) { + await markdownBtn.click({ timeout: 5_000 }); + await page.waitForTimeout(800); + } + await editor.waitFor({ state: "visible", timeout: 8_000 }); + await editor.click(); + await page.keyboard.type(captureText, { delay: 50 }); + await page.waitForTimeout(600); + const saveBtn = page.getByRole("button", { name: /Save/i }); + await saveBtn.waitFor({ state: "visible", timeout: 8_000 }); + await saveBtn.click({ timeout: 5_000 }); + await page.waitForTimeout(2_000); +} @@ - await page.waitForTimeout(1_000); - const editor = ... - ... - await page.waitForTimeout(2_000); + await fillCaptureAndSave(page, `E2E capture ${Date.now()}`); @@ - await page.waitForTimeout(1_000); - const editor = ... - ... - await page.waitForTimeout(2_000); + await fillCaptureAndSave(page, `E2E capture UI ${Date.now()}`);Also applies to: 77-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/capture.spec.ts` around lines 33 - 64, Extract the repeated editor detection, markdown toggle, typing and save sequence into a single helper function (e.g., captureAndSave(page, text?)) that encapsulates the logic currently performed with variables editor, markdownBtn, editorVisible, saveBtn and captureText: detect the editor via the same testId/placeholder/or role fallback, click the Markdown toggle if the editor is not visible, wait for the editor to be visible, focus and type the provided text with the same delays, wait, then wait for and click the Save button and final wait; replace the duplicated blocks in both locations (the current block and the one at lines 77-108) with calls to this helper to keep behavior identical.apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts (2)
109-119: Duplicated timeline verification logic—consider extracting a shared helper.Both tests have nearly identical code for locating and asserting timeline entries (lines 109-119 and 251-260). Extracting this into a reusable helper (e.g.,
assertGoalVisibleInTimeline(page, goalName)) would reduce duplication and improve maintainability.Also applies to: 251-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` around lines 109 - 119, The test duplicates timeline-locating/assertion logic (the timelineFocusEntry locator and the page.getByText(goalName) visibility checks) in two places; extract this into a shared helper function (e.g., assertGoalVisibleInTimeline(page, goalName)) that encapsulates creating the locator with the AM/PM regex, awaiting visibility with appropriate timeouts, and asserting the goalName visibility, then replace the duplicated blocks with calls to that helper in the spec where timelineFocusEntry and page.getByText are used.
54-54: Consider replacing fixedwaitForTimeoutcalls with explicit state assertions.The test contains many
waitForTimeoutcalls with arbitrary delays (500ms–4000ms). These introduce flakiness—tests may fail on slower machines or pass only by luck when the app responds faster. Prefer waiting for specific UI states (e.g.,waitFor,expect(...).toBeVisible()) instead of fixed sleeps.For example, Line 54's
await page.waitForTimeout(800)could be replaced with waiting for the search results to appear.Also applies to: 67-67, 79-79, 100-100, 107-107, 140-140, 164-164, 173-173, 182-182, 200-200, 209-209, 221-221, 242-242, 249-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` at line 54, Replace the brittle fixed sleeps (await page.waitForTimeout(...)) with explicit UI-state waits: locate each occurrence of page.waitForTimeout in this goal creation spec and swap it for targeted waits such as await page.waitForSelector('...') or await expect(page.locator('...')).toBeVisible()/toHaveText() that match the actual element you expect (e.g., search results list, suggestion item, created-goal toast/modal, input value update, navigation target). Keep the same logical sequence but assert the specific element or text before proceeding so actions like typing, clicking, and assertions use deterministic waits instead of arbitrary timeouts.apps/e2e-playwright/tests/shared/settings/settings.spec.ts (2)
18-21:process.cwd()may be unreliable for fixture paths.Using
process.cwd()assumes tests always run from the e2e-playwright directory. If tests are run from a different working directory, the path will break. Consider using__dirnameorimport.meta.urlfor more robust path resolution.Proposed fix
+import { fileURLToPath } from "node:url"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + const pdfFixturePath = path.resolve( - process.cwd(), - "tests/fixtures/files/Lorem_ipsum.pdf" + __dirname, + "../../fixtures/files/Lorem_ipsum.pdf" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/settings/settings.spec.ts` around lines 18 - 21, The pdf fixture path uses process.cwd() which breaks if tests run from a different working directory; update the resolver for pdfFixturePath so it derives the base directory from the test file location (e.g., use __dirname or import.meta.url) instead of process.cwd(), and replace the path.resolve(process.cwd(), ...) call used to construct pdfFixturePath with a path resolution that references the test module's directory (look for the pdfFixturePath constant and the path.resolve call in settings.spec.ts).
1343-1360: Multiple inline comments violate coding guidelines.As per coding guidelines: "never introduce inline comments when modifying code." Lines 1343-1344, 1354-1356, 1359, 1389-1406, 1404-1406, and 1414 contain inline comments. Consider removing them or converting to JSDoc/test descriptions.
Also applies to: 1389-1416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/settings/settings.spec.ts` around lines 1343 - 1360, This test contains multiple inline comments within the block handling the PiP checkbox (symbols: pipRow, pipInput, pipSettingExists) which violates the guideline against inline comments; remove those inline comments and instead move their intent into the test description or a short JSDoc/test comment above the test case, and/or rename the locator variables to be self-documenting (e.g., pipRow -> pipToggleRow) so the code is clear without inline comments; ensure behavior and assertions remain unchanged (checking count(), using label:has(input) click, waiting, and expect(pipInput).toBeChecked) while deleting the inline comment lines.apps/e2e-playwright/tests/utils/helpers.ts (3)
56-64: Redundant consecutive loops with identical logic.Lines 56-59 loop up to 4 times, then lines 60-64 loop up to 3 more times with nearly the same logic. If the intent is to retry up to 7 times total, consolidate into a single loop for clarity.
Proposed simplification
- for (let i = 0; i < 4; i += 1) { - const handled = await clickContinueOfflineIfVisible(); - if (!handled) break; - } - for (let i = 0; i < 3; i += 1) { + for (let i = 0; i < 7; i += 1) { const handled = await clickContinueOfflineIfVisible(); if (!handled) break; await page.waitForLoadState("domcontentloaded").catch(() => null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/utils/helpers.ts` around lines 56 - 64, The two consecutive loops calling clickContinueOfflineIfVisible() are redundant; consolidate them into a single loop that retries up to 7 times total, e.g. for (let i = 0; i < 7; i++), call clickContinueOfflineIfVisible() each iteration, break when it returns false, and when it returns true call page.waitForLoadState("domcontentloaded").catch(() => null) as before; update any surrounding comments to reflect the single retry loop and keep the same semantics for handled/break behavior.
7-7: UnusedbaseURLvariable.
baseURLis defined on line 7 but never used in the helper functions. Consider removing it if not needed.Proposed fix
const runtimeEnv = ( globalThis as { process?: { env?: Record<string, string | undefined> } } ).process?.env; -const baseURL = runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/utils/helpers.ts` at line 7, The declared constant baseURL (const baseURL = runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173";) is unused — delete that declaration from apps/e2e-playwright/tests/utils/helpers.ts and remove any now-unused imports or references to runtimeEnv in that file; if the intention was to supply a base URL to helper functions, instead wire runtimeEnv?.APP_BASE_URL into the specific helper(s) that need it (e.g., pass as an argument or use directly inside functions like your request/url helpers) and ensure TypeScript compiles and tests pass after the change.
66-75: Hardcoded product config contradicts product-agnostic intent.The JSDoc states this helper works for memotron/pointron/nucleus by using "the project's baseURL," but the implementation hardcodes
nucleusProductConfigat line 66. While all three products currently have identicalhomePath: "calendar"values, this creates a code smell and brittleness—if configs diverge or the function is adapted for other products, it will fail silently. Accept a product config as a parameter or detect the current product dynamically to align implementation with the declared intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/utils/helpers.ts` around lines 66 - 75, The helper currently hardcodes nucleusProductConfig.homePath when calling page.goto and waiting for the calendar URL; update the helper (the function that calls page.goto and waitForURL) to accept a product config or derive the base path dynamically instead of using nucleusProductConfig.homePath so it becomes product-agnostic—use the provided productConfig.homePath (or resolve from the test project's baseURL) when calling page.goto(...) and in the waitForURL predicate (replace hardcoded "/calendar" checks with the resolved homePath) so the logic works for memotron/pointron/nucleus without silent failures.apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts (2)
45-45: Multiple fixedwaitForTimeoutcalls reduce test reliability.Similar to other test files, this spec uses numerous hardcoded delays. These should be replaced with explicit state assertions where possible to improve test stability.
Also applies to: 72-72, 79-79, 87-87, 116-116, 139-139, 146-146, 149-149, 159-159, 166-166, 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts` at line 45, Replace the hardcoded page.waitForTimeout calls with explicit state-based waits: locate each usage of page.waitForTimeout in this spec (e.g., the calls at 1_500 and the other listed occurrences) and replace them with appropriate Playwright assertions such as await page.waitForSelector(selector, { state: "visible" }), await page.waitForResponse(urlOrPredicate), or await expect(page.locator(selector)).toBeVisible()/toHaveText(...) so the test waits for the actual UI/state change instead of a fixed delay; ensure you pick the selector or response that directly correlates to the action being waited on (e.g., a dialog, button enablement, network response, or updated text) and remove the timeout-only calls.
89-89: Inline comment violates coding guidelines.As per coding guidelines for
**/*.{ts,tsx,svelte,js,jsx}: "never introduce inline comments when modifying code." Consider removing this comment or moving the explanation to a JSDoc or test description.Proposed fix
- // Pinned list item in Quick Focus panel shows goal name and "Not focused today" const pinnedGoal = page🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts` at line 89, Remove the inline comment "// Pinned list item in Quick Focus panel shows goal name and \"Not focused today\"" and either delete it or move its text into a JSDoc block or the surrounding test/it description that asserts the Quick Focus pinned list item behavior (the test in browse.spec.ts that checks the pinned goal display), so no inline comment remains in the test body.apps/e2e-playwright/playwright.config.ts (1)
27-38: Auth file parsing could benefit from stricter validation.The
resolveBaseURLWithAuthfunction parses JSON and casts to a type, but if the file is corrupted or has unexpected structure,state.origins?.mapmay fail silently or produce unexpected results. The current try/catch handles this, but consider adding a simple schema check for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/playwright.config.ts` around lines 27 - 38, The resolveBaseURLWithAuth function currently casts parsed JSON to { origins?: Array<{ origin: string }> } and then accesses state.origins blindly; add a runtime schema check after parsing to ensure state.origins is an array and every item has a string origin (e.g. Array.isArray(state.origins) and state.origins.every(i => typeof i?.origin === "string")), only then map to savedOrigins and use savedOrigins[0] as fallback; if validation fails, treat as no origins (keep existing try/catch behavior) and consider logging the invalid authPath for debugging instead of proceeding with a malformed structure.apps/e2e-playwright/tests/memotron/app-nav.spec.ts (1)
85-108: Complex fallback navigation logic may mask real failures.The test clicks
Nodes, waits, then checks if it's on the Library page, and if not, navigates directly viapage.goto(). This fallback approach can mask genuine navigation bugs—if the first click fails, the test still passes via the fallback. Consider either:
- Asserting the first navigation succeeds without fallback, or
- Logging/reporting when the fallback path is taken so flakiness is visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts` around lines 85 - 108, The fallback navigation using page.goto() masks click failures: after clicking nodesCard/nodesBtn (symbols: nodesCard, nodesBtn, onLibraryPage, memotronProductConfig.pathByNavLabel.Library), replace the silent fallback with a hard assertion that onLibraryPage() is true immediately after the click so the test fails on a real navigation bug; if you must keep the fallback for flakiness, add an explicit log/diagnostic and emit a test warning/failure when the fallback path is taken (i.e., detect !onLibraryPage() and call the test logger or fail the test before performing page.goto()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac248a26-c912-43a3-baa9-eedd3e46592f
⛔ Files ignored due to path filters (2)
apps/e2e-playwright/tests/fixtures/files/Lorem_ipsum.pdfis excluded by!**/*.pdfpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (69)
apps/e2e-playwright/.env.exampleapps/e2e-playwright/.gitignoreapps/e2e-playwright/README.mdapps/e2e-playwright/config/memotron-product.config.tsapps/e2e-playwright/config/nucleus-product.config.tsapps/e2e-playwright/config/pointron-product.config.tsapps/e2e-playwright/global-setup.tsapps/e2e-playwright/package.jsonapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/scripts/save-google-auth-state.tsapps/e2e-playwright/tests/memotron/.gitkeepapps/e2e-playwright/tests/memotron/app-nav.spec.tsapps/e2e-playwright/tests/memotron/settings/settings.spec.tsapps/e2e-playwright/tests/nucleus/app-nav.spec.tsapps/e2e-playwright/tests/nucleus/settings/settings.spec.tsapps/e2e-playwright/tests/pointron/.gitkeepapps/e2e-playwright/tests/pointron/app-nav.spec.tsapps/e2e-playwright/tests/pointron/settings/settings.spec.tsapps/e2e-playwright/tests/regression/navigation.spec.tsapps/e2e-playwright/tests/shared/calendar/calendar.spec.tsapps/e2e-playwright/tests/shared/collection/collection.spec.tsapps/e2e-playwright/tests/shared/focus/goal/browse.spec.tsapps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/goal/context-menu.spec.tsapps/e2e-playwright/tests/shared/focus/goal/creation.spec.tsapps/e2e-playwright/tests/shared/focus/goal/record-page.spec.tsapps/e2e-playwright/tests/shared/focus/task/browse.spec.tsapps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/task/context-menu.spec.tsapps/e2e-playwright/tests/shared/focus/task/creation.spec.tsapps/e2e-playwright/tests/shared/focus/task/record-page.spec.tsapps/e2e-playwright/tests/shared/memory/capture.spec.tsapps/e2e-playwright/tests/shared/memory/node/browse.spec.tsapps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/memory/node/context-menu.spec.tsapps/e2e-playwright/tests/shared/memory/node/creation.spec.tsapps/e2e-playwright/tests/shared/memory/node/record-page.spec.tsapps/e2e-playwright/tests/shared/navigation.spec.tsapps/e2e-playwright/tests/shared/overview/overview.spec.tsapps/e2e-playwright/tests/shared/settings/settings.spec.tsapps/e2e-playwright/tests/smoke/home.spec.tsapps/e2e-playwright/tests/utils/helpers.tsapps/e2e-playwright/tsconfig.jsonapps/pointron/vite.config.tsclient/components/commandBar/CommandBar.svelteclient/components/goals/info/GoalTitleRow.svelteclient/components/library/LibraryRecordsPane.svelteclient/components/library/LibrarySearchBox.svelteclient/components/modal/ModalLayout.svelteclient/components/settings/SettingsAsModal.svelteclient/components/settings/appearance/ColorSchemeSelectorItem.svelteclient/components/settings/interactionMode/InteractionModeSettings.svelteclient/components/settings/taco/TacoSettings.svelteclient/components/shortcuts/settings/ShortcutItem.svelteclient/components/tasks/CreateTask.svelteclient/components/tasks/CreateTaskInlineWizard.svelteclient/components/tasks/Task.svelteclient/components/tasks/TaskLibrary.svelteclient/elements/InlineSearchBar.svelteclient/elements/input/TextInput.svelteclient/layout/leftPanel/LeftNavFixed.svelteclient/layout/leftPanel/LeftNavSettingsPopover.svelteclient/layout/topNav/TopNavLeftLogo.svelteclient/layout/topNav/TopNavLeftMenuItem.svelteclient/products/memotron/capture/Writer.svelteclient/products/pointron/focus/quickstart/QuickStart.svelteclient/products/pointron/focus/session.store.tsclient/products/pointron/settings/SessionSettings.svelteclient/types/appearance.type.ts
💤 Files with no reviewable changes (1)
- apps/e2e-playwright/tests/regression/navigation.spec.ts
| test.skip("collection workflows", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: collection create, browse, edit, etc. | ||
| }); |
There was a problem hiding this comment.
collection workflows test is declared but never executed
test.skip plus TODO-only body means this spec adds no actual workflow validation. Please implement a minimal executable scenario or mark as fixme with tracking so test health is explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/collection/collection.spec.ts` around lines
22 - 25, The "collection workflows" test is currently skipped and contains only
a TODO, so either make it an explicit failing placeholder or implement a minimal
runnable scenario: replace test.skip("collection workflows", ...) with
test.fixme("collection workflows", { reason: "TODO: implement flows" }, ...) to
track it, or implement a small end-to-end flow inside the existing test (use
ensureInAppOnHome(page) then interact with the app UI helpers: trigger the
collection creation UI, fill required fields, submit, and assert the new
collection appears in the list) and keep assertions so the test executes and
validates behavior.
| test.skip("context menu actions from library goal row", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: open Library → Goals, open context menu on a goal, assert and run each action | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("from record page", () => { | ||
| test.skip("context menu actions from goal record page", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: open goal record, open context menu, assert and run each action | ||
| }); |
There was a problem hiding this comment.
@regression spec currently contributes zero coverage
Both scenarios are permanently skipped (test.skip), so this file won’t catch regressions yet. Please either implement a minimal executable assertion path now or convert placeholders to tracked test.fixme with issue linkage so coverage status is explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/context-menu.spec.ts` around
lines 23 - 33, The two tests ("context menu actions from library goal row" and
"context menu actions from goal record page") are permanently skipped via
test.skip so they add zero coverage; either implement a minimal executable
assertion path in each (use ensureInAppOnHome(...) then navigate/open Library →
Goals or open a goal record, open the context menu and assert at least one
expected element/state) or replace test.skip with test.fixme and add the
tracking issue ID in the test title or comment so failures are explicitly
tracked. Locate the tests by the test.skip calls in this file and update the
test instances (and any helpers like ensureInAppOnHome) accordingly.
| test.skip("open goal record page and assert panels/expected content visible", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: open a goal from library or command, assert record page layout and panels | ||
| }); | ||
|
|
||
| test.skip("tab switching and visibility check on goal record page", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: switch tabs on goal record page and assert visibility | ||
| }); |
There was a problem hiding this comment.
Record-page regression scenarios are currently inert
Both tests are skipped and have placeholder steps only. This should ship with at least one executable visibility assertion to avoid a false sense of coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/record-page.spec.ts` around lines
22 - 30, Unskip the first skipped test by changing test.skip(...) to test(...)
in record-page.spec.ts and implement a minimal executable visibility check:
after ensureInAppOnHome(page) use the page to open a goal record (e.g. click a
goal list item like selector '[data-test=goal-item]' or use
page.getByTestId('goal-item')), then assert the record page rendered by checking
a stable selector such as '[data-test=goal-record-panel]' or a heading (use
Playwright's expect(...).toBeVisible()) so the test exercises ensureInAppOnHome,
the click, and a real visibility assertion rather than a TODO.
| test.skip("open Library → Tasks and see task in list", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: create task, open Library → Tasks, assert task visible | ||
| }); | ||
| }); | ||
|
|
||
| test.describe("from pinned resource browser", () => { | ||
| test.skip("pin task and see in pinned list", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO: pin task to quick focus (if supported), assert in pinned list | ||
| }); |
There was a problem hiding this comment.
Task browse spec is all placeholders right now
Both flows are test.skip with TODO-only logic, so this file won’t protect task browsing behavior. Please add at least one runnable assertion path in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/task/browse.spec.ts` around lines 23 -
33, Unskip and implement at least one runnable assertion: for the "open Library
→ Tasks and see task in list" test, remove test.skip and use
ensureInAppOnHome(page) then navigate into the Library → Tasks view (reuse
whatever router/menu interactions used elsewhere in tests), create or ensure a
known task exists (call the same helper used elsewhere to create tasks or seed
one), and assert the task is visible by querying its unique selector/text
(reference the test title "open Library → Tasks and see task in list" and the
helper ensureInAppOnHome). If creating a task is not feasible here,
alternatively assert that the Tasks list renders expected static elements (e.g.,
list container or "No tasks" message) so the test is runnable; keep the "pin
task and see in pinned list" test unchanged or skipped for now.
| test.skip("browse nodes from library", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO | ||
| }); | ||
|
|
||
| test.skip("browse nodes from pinned resource browser", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO | ||
| }); |
There was a problem hiding this comment.
Skipped placeholders leave this regression spec non-functional
Both test cases are skipped and contain only TODOs, so this suite currently cannot detect breakage. Please add at least one runnable happy-path assertion before merging this as a regression spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/memory/node/browse.spec.ts` around lines 22
- 30, Un-skip the two placeholder tests (change test.skip to test) and replace
the TODOs with a minimal happy-path assertion: after calling
ensureInAppOnHome(page) in each test, perform the same basic UI check that a
browse view is present (e.g. open the library or pinned resource browser control
and assert that a node list/card/entry locator is visible). Update the "browse
nodes from library" and "browse nodes from pinned resource browser" tests to use
Playwright's expect on a stable locator (role/test-id/aria label used by the
app) so each test actually runs and fails on regressions.
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/playwright.config.ts`:
- Around line 23-34: The function resolveBaseURLWithAuth currently overrides the
computed baseURL with a saved auth-state origin when they mismatch; change this
so we do NOT silently replace an explicitly configured base URL. Detect whether
baseURL was explicitly set via environment (check process.env.APP_BASE_URL and
process.env[`APP_BASE_URL_${projectName.toUpperCase()}`]) or is
non-empty/default; only replace baseURL with savedOrigins[0] when no explicit
env override exists (i.e., baseURL is empty/undefined or equals the code's
default), otherwise keep the original baseURL in resolveBaseURLWithAuth.
In `@apps/e2e-playwright/tests/shared/navigation.spec.ts`:
- Around line 15-20: The product detection currently in
getActiveProductConfig(pageUrl: string) relies on the page URL host and can
misidentify projects that share a host (e.g., localhost); change the API to
derive product from the Playwright project name instead: update
getActiveProductConfig to accept the Playwright project name (or
TestInfo.project.name) rather than pageUrl, map known project names ("pointron",
"memotron", default "nucleus") to
pointronProductConfig/memotronProductConfig/nucleusProductConfig, and replace
other usages (the similar logic around lines 39-40) to pass the test/project
name or TestInfo so all nav/path assertions use the Playwright project identity
instead of the URL host.
In `@apps/e2e-playwright/tests/shared/settings/settings.spec.ts`:
- Around line 1467-1469: The test currently uses a fixed 65s sleep
(page.waitForTimeout(65_000)) after awaiting focusTimerButton, which slows and
flakes tests; replace that fixed wait with a state-based wait that polls for the
break-reminder condition instead (for example use expect(...).toBeVisible({
timeout: 65_000 }) or page.waitForSelector / page.waitForFunction that watches
the break-reminder element/state) so the test proceeds as soon as the break
reminder appears; update the code that calls page.waitForTimeout to wait on the
break-reminder selector/state (referencing focusTimerButton and the
break-reminder selector or state) and remove the hard-coded sleep.
- Around line 18-21: The pdfFixturePath uses process.cwd() which is brittle in
CI; change it to build the fixture path from the test file directory (use
__dirname) so resolution is relative to
apps/e2e-playwright/tests/shared/settings; update the pdfFixturePath
initialization (the variable name pdfFixturePath and its path.resolve call) to
use path.resolve(__dirname, ...) or path.join(__dirname, ...) with the
appropriate relative segments to reach tests/fixtures/files/Lorem_ipsum.pdf
instead of process.cwd().
In `@apps/e2e-playwright/tests/utils/helpers.ts`:
- Around line 11-16: The current getActiveProductConfig infers product by
checking substrings in the URL host (host.includes("pointron") etc.), which is
fragile; change getActiveProductConfig to use an explicit project identity
instead — add an optional parameter projectId (or read a reliable test-level env
var like PROJECT_ID) and map projectId to pointronProductConfig,
memotronProductConfig, or nucleusProductConfig; if projectId is absent, as a
last resort fall back to the existing host-based logic. Update all callers to
pass the projectId (or ensure the env var is set) so tests running multiple
projects on the same host select the correct product config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f2325a1-80f1-499b-8d59-1af32419e64b
📒 Files selected for processing (5)
apps/e2e-playwright/package.jsonapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/tests/shared/navigation.spec.tsapps/e2e-playwright/tests/shared/settings/settings.spec.tsapps/e2e-playwright/tests/utils/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/e2e-playwright/package.json
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/e2e-playwright/tests/shared/overview/overview.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/shared/overview/overview.spec.ts:95">
P2: The new Focus metric assertion is too weak: counting `"Focus"` matches (`length >= 2`) can pass without the Focus metric card being visible.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
apps/e2e-playwright/tests/shared/overview/overview.spec.ts (1)
34-41: Extract repeated URL pattern matching to a helper.The
waitForURLpattern withnucleusProductConfig.pathByNavLabel.Overviewis duplicated 7 times. Consider extracting this to reduce repetition.Proposed helper
const overviewPath = nucleusProductConfig.pathByNavLabel.Overview; const isOnOverviewPage = (u: string | URL) => new URL(u).pathname === overviewPath || new URL(u).pathname.startsWith(`${overviewPath}/`); // Usage in tests: await page.waitForURL((u) => isOnOverviewPage(u), { timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts` around lines 34 - 41, Extract the repeated URL-check into a small helper to avoid duplication: create a function (e.g., isOnOverviewPage) that reads nucleusProductConfig.pathByNavLabel.Overview and returns true when new URL(u).pathname equals that path or startsWith(`${path}/`), then replace all inline predicates passed to page.waitForURL with a call to this helper (keep the same timeout of 10_000 and the existing waitForTimeout calls unchanged).apps/e2e-playwright/playwright.config.ts (1)
43-44: Consider logging auth file read/parse failures.The empty
catchsilently ignores errors when reading or parsing the auth state file. A brief warning would help diagnose corrupted or invalid auth files without breaking the config.Proposed fix
- } catch { + } catch (err) { + console.warn(`[e2e] ${projectName}: could not read auth state from ${authPath}:`, err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/playwright.config.ts` around lines 43 - 44, The empty catch in playwright.config.ts silently swallows errors when reading/parsing the auth state file; update that catch block (the try that reads/parses the auth file in the Playwright config) to log a warning including the error and the auth file path or identifier (e.g., use console.warn or the project logger) so failures to read/parse the auth file are visible for debugging without throwing.apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts (1)
75-79: SwallowedwaitFortimeout could mask command bar issues.If the command bar fails to close, the test continues with stale UI state. Consider letting this timeout fail the test or adding an explicit assertion.
Proposed fix
- await page - .getByTestId("command-bar-input") - .waitFor({ state: "hidden", timeout: 5_000 }) - .catch(() => null); + await page + .getByTestId("command-bar-input") + .waitFor({ state: "hidden", timeout: 5_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts` around lines 75 - 79, The test currently swallows waitFor errors on the command bar locator (page.getByTestId("command-bar-input").waitFor(...).catch(() => null)), which can mask failures; remove the .catch and replace with an explicit assertion such as using Playwright's expect: await expect(page.getByTestId("command-bar-input")).toBeHidden({ timeout: 5_000 }) (or keep waitFor without .catch and follow it with expect) so the test fails when the command bar does not close and does not continue with stale UI state.apps/e2e-playwright/scripts/save-google-auth-state.ts (1)
23-24: Minor redundancy:appOriginduplicatesbaseOrigin.
baseOriginis already defined at Line 23 with the same value. Consider reusing it at Line 81 instead of creating anotherconst appOrigin.Proposed fix
- const appOrigin = new URL(baseURL).origin; - // Save only when we're back on the app on a post-login path (not /account/login). // Otherwise an HTTP→HTTPS redirect that keeps us on /account/login would be treated as "back" and close before opening Google. const isBackOnApp = (url: URL) => @@ await page.waitForURL((url) => { const u = new URL(url.toString()); - return !(u.origin === appOrigin && u.pathname === "/account/login"); + return !(u.origin === baseOrigin && u.pathname === "/account/login"); }, {Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/scripts/save-google-auth-state.ts` around lines 23 - 24, The code defines baseOrigin = new URL(baseURL).origin and later redundantly declares appOrigin with the same value; remove the extra const appOrigin and replace its usages with baseOrigin (ensure baseOrigin is in scope where appOrigin was used), so all references use the existing baseOrigin variable derived from baseURL and no duplicate URL parsing occurs.apps/e2e-playwright/tests/pointron/app-nav.spec.ts (1)
91-107: Fallback navigation adds complexity.The conditional
if (!onLibraryPage())block withpage.gotosuggests the command bar navigation may not reliably land on the Library page. Consider investigating root cause or documenting why this fallback is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts` around lines 91 - 107, The fallback block using onLibraryPage(), page.goto(libraryPath) and the subsequent waits adds complexity because command-bar navigation sometimes doesn't land on Library; add a concise inline comment above the if (!onLibraryPage()) explaining why this fallback is required (reference libraryPath, onLibraryPage, page.goto, goalsBtn) and add a single-line TODO to investigate/remove the fallback once the root cause of unreliable command-bar navigation is fixed so future maintainers know it's intentional.apps/e2e-playwright/tests/shared/navigation.spec.ts (1)
14-191: Test is comprehensive but quite long.This single test handles multiple concerns: offline recovery, navigation validation, and Overview navigation. Consider splitting into smaller focused tests for better isolation and debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/navigation.spec.ts` around lines 14 - 191, The test "already logged in (Google auth state): handle old page if present, then verify in app `@smoke`" is doing too many things and should be split; extract the offline-recovery flow contained in clickContinueOfflineIfVisible into its own test(s) and keep navigation verification (home/Overview checks using productConfig, navAction, expectedPath) in separate focused tests. Concretely, create one test that exercises clickContinueOfflineIfVisible / the loop that retries offline recovery (reuse the helper clickContinueOfflineIfVisible and the retry loops), asserting it reaches the app/homePath or throws the existing recovery error; create another test that assumes a logged-in app state and only verifies the menu/nav markers and the Overview navigation (use homeNav, navMarkers, todayButton, navAction, expectedPath) with the existing waits and expectations. Ensure both tests reuse getProductConfig(test.info().project.name) and any shared helpers but remove unrelated steps (Google route blocking and excessive retry loops) from each new test to keep them focused.apps/e2e-playwright/tests/utils/helpers.ts (1)
126-136: HardcodedwaitForTimeoutcalls can cause flakiness.Lines 132 and 134 use fixed delays (
500ms,100ms). Consider waiting for a specific UI state (e.g., command list population) instead.Alternative approach
await cmdInput.fill("Quick focus"); - await page.waitForTimeout(500); + // Wait for command list to populate + await page.locator('[data-testid="command-bar-list"] button').first().waitFor({ state: "visible", timeout: 5_000 }); await page.keyboard.press("ArrowDown"); - await page.waitForTimeout(100); await page.keyboard.press("Enter");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/utils/helpers.ts` around lines 126 - 136, The hardcoded waits in runQuickFocusCommand (the 500ms and 100ms waitForTimeout calls) make the test flaky; replace them by waiting for a concrete UI state after filling cmdInput—e.g., waitFor the command list/suggestion item to be visible or enabled (use a locator for the suggestion list or a specific suggestion item) before sending ArrowDown and before pressing Enter, and remove the fixed delays so the function uses await cmdInput.waitFor(...) or await page.getByTestId(.../suggestion-item).waitFor({ state: "visible" }) to synchronize instead of timeouts.apps/e2e-playwright/tests/nucleus/app-nav.spec.ts (2)
204-207: UsenucleusProductConfig.pathByNavLabel.Libraryin both Library UI tests.Hardcoding
/libraryhere duplicates routing knowledge and bypasses the per-product config the rest of this spec already relies on.Also applies to: 263-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` around lines 204 - 207, Replace the hardcoded "/library" URL checks in the Library UI tests with the product-configured path: use nucleusProductConfig.pathByNavLabel.Library when calling page.waitForURL so the test relies on per-product routing; update both occurrences (the waitForURL at the page.waitForURL call near the first Library test and the second instance around the later Library test) to build the regex using new URL(u).pathname and the configured path value instead of the literal "/library".
70-75: Avoid DOM-parent hopping for the panel action.The
locator("..").locator("..").getByRole("button").last()chain is layout-coupled and can start clicking a different control after a harmless markup change. A stable test id or a more specific scoped role/name would make this flow much less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` around lines 70 - 75, The test is brittle because it hops up the DOM with locator("..").locator("..") then selects the last button; update the click to target a stable, scoped selector instead: add or use an existing test id or an accessible name for the panel action (e.g., data-testid="panel-action" or getByRole('button', { name: /.../ })) and call it directly from overviewInPanel or timelineInPanel (replace panelRow.getByRole("button").last().click(...) with overviewInPanel.locator('[data-testid="panel-action"]').click(...) or overviewInPanel.getByRole('button', { name: 'Action name' }).click(...), preserving the hasOverview branching and timeout behavior.apps/e2e-playwright/tests/memotron/app-nav.spec.ts (1)
126-129: UsememotronProductConfig.pathByNavLabel.Libraryhere too.This check hardcodes
/librarywhile the rest of the spec uses the product config. If the Memotron library route changes, this test will drift from the shared config and fail for the wrong reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts` around lines 126 - 129, The test currently hardcodes "/library" in the page.waitForURL check; update it to use the shared memotronProductConfig.pathByNavLabel.Library instead. In the page.waitForURL call (the lambda using new URL(u).pathname) build the same regex/pattern dynamically from memotronProductConfig.pathByNavLabel.Library so the pathname check matches the configured library base and its subpaths (i.e., replace the literal "/library" with the config value when constructing the ^<path>(/.*)?$ match). Ensure you still use page.waitForURL, the URL pathname extraction via new URL(u).pathname, and the same timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts`:
- Line 89: Replace the fixed sleep calls that use page.waitForTimeout(1_500)
with state-based waits: use page.waitForSelector (or locator.waitFor) to wait
for observable UI elements such as the search nodes input ('#search-nodes' or
the test's 'search-nodes' selector) or the nodes list element to be
visible/enabled before proceeding in the navigation tests (these replace the
waits at the three locations using waitForTimeout). Also change the hardcoded
'/library' route to use memotronProductConfig.pathByNavLabel.Library where the
other navigation tests reference the product config so the test uses the
canonical path constant.
In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts`:
- Line 55: Several tests use fixed sleeps via page.waitForTimeout(...) which
causes flakiness; replace each await page.waitForTimeout(...) with an observable
wait that matches the subsequent assertion or action (e.g., use await
page.waitForURL(expectedUrl) when navigating, await
expect(buttonLocator).toBeVisible() or await buttonLocator.waitFor({ state:
'visible' }) before clicks, or await expect(textLocator).toHaveText(...) /
locator.isVisible() when waiting for content). Locate all instances of
page.waitForTimeout and swap them for the specific observable wait that follows
in the test (use page.waitForURL, Locator.waitFor,
expect(...).toBeVisible/toHaveText, or locator.isVisible() as appropriate) to
make the tests deterministic.
---
Nitpick comments:
In `@apps/e2e-playwright/playwright.config.ts`:
- Around line 43-44: The empty catch in playwright.config.ts silently swallows
errors when reading/parsing the auth state file; update that catch block (the
try that reads/parses the auth file in the Playwright config) to log a warning
including the error and the auth file path or identifier (e.g., use console.warn
or the project logger) so failures to read/parse the auth file are visible for
debugging without throwing.
In `@apps/e2e-playwright/scripts/save-google-auth-state.ts`:
- Around line 23-24: The code defines baseOrigin = new URL(baseURL).origin and
later redundantly declares appOrigin with the same value; remove the extra const
appOrigin and replace its usages with baseOrigin (ensure baseOrigin is in scope
where appOrigin was used), so all references use the existing baseOrigin
variable derived from baseURL and no duplicate URL parsing occurs.
In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts`:
- Around line 126-129: The test currently hardcodes "/library" in the
page.waitForURL check; update it to use the shared
memotronProductConfig.pathByNavLabel.Library instead. In the page.waitForURL
call (the lambda using new URL(u).pathname) build the same regex/pattern
dynamically from memotronProductConfig.pathByNavLabel.Library so the pathname
check matches the configured library base and its subpaths (i.e., replace the
literal "/library" with the config value when constructing the ^<path>(/.*)?$
match). Ensure you still use page.waitForURL, the URL pathname extraction via
new URL(u).pathname, and the same timeout.
In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts`:
- Around line 204-207: Replace the hardcoded "/library" URL checks in the
Library UI tests with the product-configured path: use
nucleusProductConfig.pathByNavLabel.Library when calling page.waitForURL so the
test relies on per-product routing; update both occurrences (the waitForURL at
the page.waitForURL call near the first Library test and the second instance
around the later Library test) to build the regex using new URL(u).pathname and
the configured path value instead of the literal "/library".
- Around line 70-75: The test is brittle because it hops up the DOM with
locator("..").locator("..") then selects the last button; update the click to
target a stable, scoped selector instead: add or use an existing test id or an
accessible name for the panel action (e.g., data-testid="panel-action" or
getByRole('button', { name: /.../ })) and call it directly from overviewInPanel
or timelineInPanel (replace panelRow.getByRole("button").last().click(...) with
overviewInPanel.locator('[data-testid="panel-action"]').click(...) or
overviewInPanel.getByRole('button', { name: 'Action name' }).click(...),
preserving the hasOverview branching and timeout behavior.
In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts`:
- Around line 91-107: The fallback block using onLibraryPage(),
page.goto(libraryPath) and the subsequent waits adds complexity because
command-bar navigation sometimes doesn't land on Library; add a concise inline
comment above the if (!onLibraryPage()) explaining why this fallback is required
(reference libraryPath, onLibraryPage, page.goto, goalsBtn) and add a
single-line TODO to investigate/remove the fallback once the root cause of
unreliable command-bar navigation is fixed so future maintainers know it's
intentional.
In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts`:
- Around line 75-79: The test currently swallows waitFor errors on the command
bar locator (page.getByTestId("command-bar-input").waitFor(...).catch(() =>
null)), which can mask failures; remove the .catch and replace with an explicit
assertion such as using Playwright's expect: await
expect(page.getByTestId("command-bar-input")).toBeHidden({ timeout: 5_000 }) (or
keep waitFor without .catch and follow it with expect) so the test fails when
the command bar does not close and does not continue with stale UI state.
In `@apps/e2e-playwright/tests/shared/navigation.spec.ts`:
- Around line 14-191: The test "already logged in (Google auth state): handle
old page if present, then verify in app `@smoke`" is doing too many things and
should be split; extract the offline-recovery flow contained in
clickContinueOfflineIfVisible into its own test(s) and keep navigation
verification (home/Overview checks using productConfig, navAction, expectedPath)
in separate focused tests. Concretely, create one test that exercises
clickContinueOfflineIfVisible / the loop that retries offline recovery (reuse
the helper clickContinueOfflineIfVisible and the retry loops), asserting it
reaches the app/homePath or throws the existing recovery error; create another
test that assumes a logged-in app state and only verifies the menu/nav markers
and the Overview navigation (use homeNav, navMarkers, todayButton, navAction,
expectedPath) with the existing waits and expectations. Ensure both tests reuse
getProductConfig(test.info().project.name) and any shared helpers but remove
unrelated steps (Google route blocking and excessive retry loops) from each new
test to keep them focused.
In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts`:
- Around line 34-41: Extract the repeated URL-check into a small helper to avoid
duplication: create a function (e.g., isOnOverviewPage) that reads
nucleusProductConfig.pathByNavLabel.Overview and returns true when new
URL(u).pathname equals that path or startsWith(`${path}/`), then replace all
inline predicates passed to page.waitForURL with a call to this helper (keep the
same timeout of 10_000 and the existing waitForTimeout calls unchanged).
In `@apps/e2e-playwright/tests/utils/helpers.ts`:
- Around line 126-136: The hardcoded waits in runQuickFocusCommand (the 500ms
and 100ms waitForTimeout calls) make the test flaky; replace them by waiting for
a concrete UI state after filling cmdInput—e.g., waitFor the command
list/suggestion item to be visible or enabled (use a locator for the suggestion
list or a specific suggestion item) before sending ArrowDown and before pressing
Enter, and remove the fixed delays so the function uses await
cmdInput.waitFor(...) or await page.getByTestId(.../suggestion-item).waitFor({
state: "visible" }) to synchronize instead of timeouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50e7189b-4080-48f9-830d-33282b924d44
📒 Files selected for processing (13)
apps/e2e-playwright/global-setup.tsapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/scripts/save-google-auth-state.tsapps/e2e-playwright/tests/memotron/app-nav.spec.tsapps/e2e-playwright/tests/memotron/settings/settings.spec.tsapps/e2e-playwright/tests/nucleus/app-nav.spec.tsapps/e2e-playwright/tests/pointron/app-nav.spec.tsapps/e2e-playwright/tests/shared/calendar/calendar.spec.tsapps/e2e-playwright/tests/shared/focus/goal/browse.spec.tsapps/e2e-playwright/tests/shared/memory/capture.spec.tsapps/e2e-playwright/tests/shared/navigation.spec.tsapps/e2e-playwright/tests/shared/overview/overview.spec.tsapps/e2e-playwright/tests/utils/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/e2e-playwright/global-setup.ts
- apps/e2e-playwright/tests/shared/memory/capture.spec.ts
- apps/e2e-playwright/tests/memotron/settings/settings.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/e2e-playwright/tests/memotron/app-nav.spec.ts (1)
89-89:⚠️ Potential issue | 🟠 MajorReplace the fixed sleeps and stop hardcoding the Library route.
page.waitForTimeout(...)keeps these navigation checks flaky, and Line 127 bypassesmemotronProductConfig.pathByNavLabel.Libraryeven though this file already uses the config elsewhere. Wait on the Nodes view state instead, and reuse the config constant for the URL assertion.Also applies to: 106-106, 127-127, 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts` at line 89, Replace the fixed sleep calls that use page.waitForTimeout(...) with an explicit wait for the Nodes view to be ready (e.g., waitForSelector or waitForFunction that checks the Nodes list/container or a specific Nodes-view text/state) instead of timing; also change the hardcoded Library route assertion to use the existing memotronProductConfig.pathByNavLabel.Library constant so the URL check is consistent. Update every occurrence mentioned (the wait at page.waitForTimeout and the URL assertion usage referenced as memotronProductConfig.pathByNavLabel.Library) so tests wait on the Nodes view state (selector/text) and reuse the config constant for URL assertions.apps/e2e-playwright/tests/nucleus/app-nav.spec.ts (1)
55-55:⚠️ Potential issue | 🟠 MajorReplace the fixed sleeps with observable waits.
These
page.waitForTimeout(...)calls keep the spec timing-sensitive in CI. Wait on the post-action state instead: URL change, modal dismissal, Logs empty state, or thesearch-goals/search-taskslocators that the tests already assert later.Also applies to: 61-61, 75-75, 81-81, 139-139, 156-156, 173-173, 185-185, 211-211, 237-237, 241-241, 247-247, 270-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` at line 55, Replace all page.waitForTimeout(...) calls with observable waits: after actions use await page.waitForURL(...) when a navigation should occur, await locator('selector').waitFor({ state: 'hidden' }) for modal dismissal, await locator('[data-testid="search-goals"]').waitFor(...) or locator('[data-testid="search-tasks"]').waitFor(...) for the search result elements, or wait for the Logs empty state locator to appear/disappear; update each occurrence of page.waitForTimeout in the tests (the calls labeled page.waitForTimeout(1_000) and the other instances flagged) to the appropriate conditional wait so the test waits for the post-action state instead of a fixed sleep.
🧹 Nitpick comments (10)
apps/e2e-playwright/tests/shared/memory/node/context-menu.spec.ts (1)
14-20: Consider extracting the Google-accounts route guard to a shared utility.This
beforeEachroute-interception pattern appears across multiple test files in the suite. Extracting it to a reusable helper (e.g.,blockGoogleAccountsRequests(page)inutils/helpers.ts) would reduce duplication and centralize the blocking logic.♻️ Suggested helper extraction
In
utils/helpers.ts:export async function blockGoogleAccountsRequests(page: Page): Promise<void> { await page.route("**/*", (route) => { const reqUrl = route.request().url(); if (/accounts\.google\.com/i.test(reqUrl)) route.abort(); else route.continue(); }); }Then in this file:
+import { ensureInAppOnHome, blockGoogleAccountsRequests } from "../../../utils/helpers"; -import { ensureInAppOnHome } from "../../../utils/helpers"; test.describe("node - context menu (from library, from record page) `@regression`", () => { test.beforeEach(async ({ page }) => { - await page.route("**/*", (route) => { - const reqUrl = route.request().url(); - if (/accounts\.google\.com/i.test(reqUrl)) route.abort(); - else route.continue(); - }); + await blockGoogleAccountsRequests(page); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/memory/node/context-menu.spec.ts` around lines 14 - 20, Extract the repeated Playwright route-interception into a shared helper: create and export an async function named blockGoogleAccountsRequests(page: Page) that encapsulates the current page.route("**/*", ...) logic (checking route.request().url() and aborting on /accounts\.google\.com/i, otherwise continuing). Replace the inline beforeEach block in context-menu.spec.ts with a call to await blockGoogleAccountsRequests(page). Ensure the helper is exported from utils/helpers.ts and imported where used so all tests can reuse the same function.apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts (3)
137-139: Hardcoded "Goals" label not in product config.Other navigation labels (Library, Calendar) come from
nucleusProductConfig, but "Goals" is hardcoded. If this submenu label changes, only this test will break rather than updating the config.Consider adding a
goalsSubMenuLabelor similar to the product config for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` around lines 137 - 139, The test hardcodes the "Goals" submenu label in the selector (the page.getByRole(... name: /^Goals(\s+\d+)?$/i) call) instead of using the shared nucleusProductConfig; add a new key (e.g., goalsSubMenuLabel) to nucleusProductConfig and update this spec to read that label and build the selector from it (preserve the optional count regex or generate it dynamically) so the test uses the centralized product label rather than a hardcoded string.
67-67: Consider documenting purpose of 4-second wait.The
waitForTimeout(4_000)appears to let the focus timer accumulate before finishing the session. A brief inline note explaining this would aid future maintainers—though per coding guidelines, avoid inline comments in production code; for tests, a named constant or wrapper could clarify intent:Optional clarity improvement
+ // Let focus timer accumulate before finishing await page.waitForTimeout(4_000);Or extract to a constant:
const FOCUS_SESSION_MIN_DURATION_MS = 4_000; // ... await page.waitForTimeout(FOCUS_SESSION_MIN_DURATION_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` at line 67, Replace the magic numeric sleep in the test by introducing a clearly named constant and using that constant in the call to page.waitForTimeout to document intent; e.g., declare FOCUS_SESSION_MIN_DURATION_MS near the top of the test file (or in the test helper module) and change the await page.waitForTimeout(4_000) invocation to await page.waitForTimeout(FOCUS_SESSION_MIN_DURATION_MS) so the purpose of the wait is explicit and maintainable.
185-193: Fragile locator for Quick Focus panel.Using
.locator("div").filter({ has: quickFocusSearch })will match any ancestordivcontaining the search input, which could be the entire page container. This may causegoalThumbnailto match unintended buttons outside the Quick Focus panel.Consider using a more specific selector such as a
data-testidon the Quick Focus panel, or narrowing the locator chain:Proposed approach
- const quickFocusPanel = page.locator("div").filter({ - has: quickFocusSearch - }); - const goalThumbnail = quickFocusPanel - .locator("button") - .filter({ hasText: goalName }) - .first(); + // If a data-testid is added to the Quick Focus panel: + const quickFocusPanel = page.getByTestId("quick-focus-panel"); + const goalThumbnail = quickFocusPanel + .locator("button") + .filter({ hasText: goalName }) + .first();Alternatively, scope relative to the search input's parent with a more specific structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` around lines 185 - 193, The current quickFocusPanel locator is too broad and may match any ancestor div containing quickFocusSearch; update the locator to scope to the actual Quick Focus panel before finding goalThumbnail. Replace the div.filter(...) chain that creates quickFocusPanel with a more specific selector (e.g., a data-testid like "quick-focus-panel", a unique class/role, or the nearest parent of quickFocusSearch) and then locate the button with .locator("button").filter({ hasText: goalName }).first(); ensure you update references to quickFocusPanel, quickFocusSearch, and goalThumbnail accordingly so the search is scoped to the intended panel.apps/e2e-playwright/playwright.config.ts (1)
17-20: Auth path naming logic is duplicated across files.The
getAuthPathhelper here uses dynamic per-project naming (user.jsonfor nucleus,user-{product}.jsonfor others), butglobal-setup.ts(lines 84-85) hardcodes the same list:["user.json", "user-memotron.json", "user-pointron.json"]. If a new product is added,global-setup.tswould need manual updates while this file handles it automatically.Consider extracting a shared constant or helper for auth file names to ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/playwright.config.ts` around lines 17 - 20, Extract the auth filename logic into a shared helper or constant (e.g., a function getAuthFilename(productName) or an exported AUTH_FILE_NAMES generator) and replace the duplicated logic in getAuthPath and the hardcoded list in global-setup.ts to call that shared helper; ensure the helper returns "user.json" for "nucleus" and `user-{product}.json` for others, export it from a common module and import it where getAuthPath and the global setup enumerate auth files so adding a new product updates both places automatically.apps/e2e-playwright/tests/shared/overview/overview.spec.ts (1)
34-40: Consider extracting the repeated URL wait pattern.The same
page.waitForURLpattern with the Overview path regex is repeated 7 times across tests. Consider extracting this into a helper function.Proposed approach
async function waitForOverviewPage(page: import("@playwright/test").Page) { await page.waitForURL( (u) => new RegExp(`^${nucleusProductConfig.pathByNavLabel.Overview}(\\/.*)?$`).test( new URL(u).pathname ), { timeout: 10_000 } ); await page.waitForTimeout(1_500); }Then use
await waitForOverviewPage(page);in each test.Also applies to: 75-81, 112-118, 151-157, 181-187, 208-214, 250-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts` around lines 34 - 40, Extract the repeated page.waitForURL pattern into a helper function (e.g. async function waitForOverviewPage(page: Page)) that encapsulates the RegExp using nucleusProductConfig.pathByNavLabel.Overview, calls page.waitForURL with the same timeout (10_000) and the optional page.waitForTimeout(1_500) delay, then replace each inline occurrence of page.waitForURL(...) in the tests with await waitForOverviewPage(page);; ensure the helper is exported/available to the spec and import the Page type from "@playwright/test" if needed.apps/e2e-playwright/tests/memotron/settings/settings.spec.ts (1)
23-34: Consider reusing the sharedopenSettingshelper.This local
openSettingshelper is similar to the one inshared/settings/settings.spec.ts. While the difference is minor (this one doesn't assert Settings text visibility after clicking), using the shared helper would reduce duplication and ensure consistent behavior across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/memotron/settings/settings.spec.ts` around lines 23 - 34, Replace the locally defined openSettings function with the shared helper: remove the local async function openSettings and import the shared openSettings helper, then call that helper wherever the local one was used; if the shared helper asserts Settings visibility after click (different behavior), accept that behavior or add a small option/adapter in the shared helper to preserve the original 500ms wait if necessary, but avoid duplicating the function in this file and reference the shared openSettings symbol in calls.apps/e2e-playwright/global-setup.ts (1)
102-103: Empty catch block silently ignores auth state parsing errors.If the auth state JSON is malformed or the file read fails, this empty catch block will silently continue without any indication of the problem. Consider logging a warning to help diagnose issues.
Proposed fix
- } catch { - } + } catch (err) { + console.warn("[e2e] Failed to check auth state origins:", err); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/global-setup.ts` around lines 102 - 103, The empty catch in global-setup.ts that swallows errors when parsing the auth state should instead log a warning with the error details and the auth file path (or content) to aid diagnosis; update the catch block that surrounds the auth state JSON parsing in the global setup function to call console.warn or the project's logger (including the caught Error object and the auth file variable name) so malformed JSON or read failures are visible while preserving existing flow.apps/e2e-playwright/tests/pointron/app-nav.spec.ts (1)
126-127: UsepointronProductConfig.pathByNavLabel.Libraryinstead of hardcoding/library.The rest of this spec already routes through the product config, so this assertion should too. That keeps the test aligned with the per-product navigation config introduced in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts` around lines 126 - 127, Replace the hardcoded string "/library" in the page.waitForURL assertion with the product-configured path constant pointronProductConfig.pathByNavLabel.Library so the test follows per-product navigation; update the predicate passed to page.waitForURL (the callback using new URL(u).pathname) to construct the regex using pointronProductConfig.pathByNavLabel.Library (or escape/anchor it appropriately) instead of the literal "/library", and keep the rest of the waitForURL call and test flow unchanged.apps/e2e-playwright/tests/nucleus/app-nav.spec.ts (1)
204-205: ReusenucleusProductConfig.pathByNavLabel.Libraryfor the Library URL assertions.These two checks hardcode
/libraryeven though the file already uses the product config for the same route elsewhere. Keeping the assertion on the config avoids drift if the Nucleus Library path changes.Also applies to: 263-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` around lines 204 - 205, The tests hardcode "/library" in the page.waitForURL assertions; update them to use nucleusProductConfig.pathByNavLabel.Library instead so the assertions follow the product config. Replace the regex/string that currently contains "/library" in the page.waitForURL calls with a dynamic pattern built from nucleusProductConfig.pathByNavLabel.Library (and do the same for the other occurrence later in the file), ensuring you still match optional trailing segments as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/scripts/save-google-auth-state.ts`:
- Around line 75-76: The click on the Google button uses "void
googleButton.click(...)" so click errors are swallowed; replace that with
awaiting the click (await googleButton.click({ noWaitAfter: true, timeout:
10_000 })) and optionally wrap the await in a try/catch that logs and rethrows
so any click failure (stale element, timeout) is propagated instead of letting
the script hang after googleButton.waitFor; locate the googleButton.waitFor and
googleButton.click calls to apply this change.
In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts`:
- Line 89: Replace the fixed sleep calls (uses of page.waitForTimeout) with
deterministic waits that assert or wait for visible locators for the Goals view
(use the existing goalsListVisible, the `search-goals` button locator, the
"Create new goal" button or the "Looks like you don't have any goals" text) via
expect(...).toBeVisible() or locator.waitFor(); do this for the other similar
sleeps referenced in the review. Also replace the hardcoded "/library" path with
the configured constant pointronProductConfig.pathByNavLabel.Library to navigate
to the Library consistently.
In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts`:
- Around line 45-48: The current code swallows a failing assertion by calling
.catch(() => null) on await expect(goalNameInput).toBeHidden(...); remove the
.catch and either await the assertion directly (await
expect(goalNameInput).toBeHidden({ timeout: 10_000 })) or replace it with a
non-assertion wait such as await page.waitForSelector(the same locator used for
goalNameInput, { state: "hidden", timeout: 10_000 }); ensure this check
completes before calling page.keyboard.press("Escape") so the test fails visibly
if the dialog did not close.
In `@apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.ts`:
- Around line 23-26: Replace the two test stubs that use test.skip with inline
TODO comments by marking them as deferred using Playwright's test.fixme API:
find the "test.skip(\"context menu actions from library task row\", async ({
page }) => { ... });" and the other skipped test at lines shown and change each
invocation to test.fixme("context menu actions from library task row") (and the
other test's title) so the tests are explicitly flagged as FIXME/deferred and
remove the inline "// TODO" comments; ensure you keep the original test titles
and signatures where applicable when converting to fixme calls.
- Around line 15-19: The page.route handler is currently matching every request
via the glob "**/*" which is unnecessary; change the route pattern to target the
Google Accounts domain only (e.g. "https://accounts.google.com/**" or a
host-only glob that includes accounts.google.com) and simplify the handler to
directly abort matching requests instead of checking reqUrl with a regex; you
can also remove any unnecessary async/await around
route.abort()/route.continue() since those are synchronous methods.
In `@apps/e2e-playwright/tests/shared/memory/node/record-page.spec.ts`:
- Around line 22-30: Remove the inline `// TODO` comments from the skipped tests
"open node record and assert panels/content visible" and "tab switching and
visibility on node record page"; leave the tests as skipped with their calls to
ensureInAppOnHome(page) (or remove the empty bodies entirely) and track any work
in an issue tracker instead of inline comments—update the test functions (the
test.skip blocks) by deleting the `// TODO` lines so no inline comments remain.
---
Duplicate comments:
In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts`:
- Line 89: Replace the fixed sleep calls that use page.waitForTimeout(...) with
an explicit wait for the Nodes view to be ready (e.g., waitForSelector or
waitForFunction that checks the Nodes list/container or a specific Nodes-view
text/state) instead of timing; also change the hardcoded Library route assertion
to use the existing memotronProductConfig.pathByNavLabel.Library constant so the
URL check is consistent. Update every occurrence mentioned (the wait at
page.waitForTimeout and the URL assertion usage referenced as
memotronProductConfig.pathByNavLabel.Library) so tests wait on the Nodes view
state (selector/text) and reuse the config constant for URL assertions.
In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts`:
- Line 55: Replace all page.waitForTimeout(...) calls with observable waits:
after actions use await page.waitForURL(...) when a navigation should occur,
await locator('selector').waitFor({ state: 'hidden' }) for modal dismissal,
await locator('[data-testid="search-goals"]').waitFor(...) or
locator('[data-testid="search-tasks"]').waitFor(...) for the search result
elements, or wait for the Logs empty state locator to appear/disappear; update
each occurrence of page.waitForTimeout in the tests (the calls labeled
page.waitForTimeout(1_000) and the other instances flagged) to the appropriate
conditional wait so the test waits for the post-action state instead of a fixed
sleep.
---
Nitpick comments:
In `@apps/e2e-playwright/global-setup.ts`:
- Around line 102-103: The empty catch in global-setup.ts that swallows errors
when parsing the auth state should instead log a warning with the error details
and the auth file path (or content) to aid diagnosis; update the catch block
that surrounds the auth state JSON parsing in the global setup function to call
console.warn or the project's logger (including the caught Error object and the
auth file variable name) so malformed JSON or read failures are visible while
preserving existing flow.
In `@apps/e2e-playwright/playwright.config.ts`:
- Around line 17-20: Extract the auth filename logic into a shared helper or
constant (e.g., a function getAuthFilename(productName) or an exported
AUTH_FILE_NAMES generator) and replace the duplicated logic in getAuthPath and
the hardcoded list in global-setup.ts to call that shared helper; ensure the
helper returns "user.json" for "nucleus" and `user-{product}.json` for others,
export it from a common module and import it where getAuthPath and the global
setup enumerate auth files so adding a new product updates both places
automatically.
In `@apps/e2e-playwright/tests/memotron/settings/settings.spec.ts`:
- Around line 23-34: Replace the locally defined openSettings function with the
shared helper: remove the local async function openSettings and import the
shared openSettings helper, then call that helper wherever the local one was
used; if the shared helper asserts Settings visibility after click (different
behavior), accept that behavior or add a small option/adapter in the shared
helper to preserve the original 500ms wait if necessary, but avoid duplicating
the function in this file and reference the shared openSettings symbol in calls.
In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts`:
- Around line 204-205: The tests hardcode "/library" in the page.waitForURL
assertions; update them to use nucleusProductConfig.pathByNavLabel.Library
instead so the assertions follow the product config. Replace the regex/string
that currently contains "/library" in the page.waitForURL calls with a dynamic
pattern built from nucleusProductConfig.pathByNavLabel.Library (and do the same
for the other occurrence later in the file), ensuring you still match optional
trailing segments as before.
In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts`:
- Around line 126-127: Replace the hardcoded string "/library" in the
page.waitForURL assertion with the product-configured path constant
pointronProductConfig.pathByNavLabel.Library so the test follows per-product
navigation; update the predicate passed to page.waitForURL (the callback using
new URL(u).pathname) to construct the regex using
pointronProductConfig.pathByNavLabel.Library (or escape/anchor it appropriately)
instead of the literal "/library", and keep the rest of the waitForURL call and
test flow unchanged.
In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts`:
- Around line 137-139: The test hardcodes the "Goals" submenu label in the
selector (the page.getByRole(... name: /^Goals(\s+\d+)?$/i) call) instead of
using the shared nucleusProductConfig; add a new key (e.g., goalsSubMenuLabel)
to nucleusProductConfig and update this spec to read that label and build the
selector from it (preserve the optional count regex or generate it dynamically)
so the test uses the centralized product label rather than a hardcoded string.
- Line 67: Replace the magic numeric sleep in the test by introducing a clearly
named constant and using that constant in the call to page.waitForTimeout to
document intent; e.g., declare FOCUS_SESSION_MIN_DURATION_MS near the top of the
test file (or in the test helper module) and change the await
page.waitForTimeout(4_000) invocation to await
page.waitForTimeout(FOCUS_SESSION_MIN_DURATION_MS) so the purpose of the wait is
explicit and maintainable.
- Around line 185-193: The current quickFocusPanel locator is too broad and may
match any ancestor div containing quickFocusSearch; update the locator to scope
to the actual Quick Focus panel before finding goalThumbnail. Replace the
div.filter(...) chain that creates quickFocusPanel with a more specific selector
(e.g., a data-testid like "quick-focus-panel", a unique class/role, or the
nearest parent of quickFocusSearch) and then locate the button with
.locator("button").filter({ hasText: goalName }).first(); ensure you update
references to quickFocusPanel, quickFocusSearch, and goalThumbnail accordingly
so the search is scoped to the intended panel.
In `@apps/e2e-playwright/tests/shared/memory/node/context-menu.spec.ts`:
- Around line 14-20: Extract the repeated Playwright route-interception into a
shared helper: create and export an async function named
blockGoogleAccountsRequests(page: Page) that encapsulates the current
page.route("**/*", ...) logic (checking route.request().url() and aborting on
/accounts\.google\.com/i, otherwise continuing). Replace the inline beforeEach
block in context-menu.spec.ts with a call to await
blockGoogleAccountsRequests(page). Ensure the helper is exported from
utils/helpers.ts and imported where used so all tests can reuse the same
function.
In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts`:
- Around line 34-40: Extract the repeated page.waitForURL pattern into a helper
function (e.g. async function waitForOverviewPage(page: Page)) that encapsulates
the RegExp using nucleusProductConfig.pathByNavLabel.Overview, calls
page.waitForURL with the same timeout (10_000) and the optional
page.waitForTimeout(1_500) delay, then replace each inline occurrence of
page.waitForURL(...) in the tests with await waitForOverviewPage(page);; ensure
the helper is exported/available to the spec and import the Page type from
"@playwright/test" if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b6ee58c-6df6-45f5-88b6-b9960f3433e0
📒 Files selected for processing (30)
apps/e2e-playwright/global-setup.tsapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/scripts/save-google-auth-state.tsapps/e2e-playwright/tests/memotron/app-nav.spec.tsapps/e2e-playwright/tests/memotron/settings/settings.spec.tsapps/e2e-playwright/tests/nucleus/app-nav.spec.tsapps/e2e-playwright/tests/nucleus/settings/settings.spec.tsapps/e2e-playwright/tests/pointron/app-nav.spec.tsapps/e2e-playwright/tests/pointron/settings/settings.spec.tsapps/e2e-playwright/tests/shared/calendar/calendar.spec.tsapps/e2e-playwright/tests/shared/collection/collection.spec.tsapps/e2e-playwright/tests/shared/focus/goal/browse.spec.tsapps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/goal/context-menu.spec.tsapps/e2e-playwright/tests/shared/focus/goal/creation.spec.tsapps/e2e-playwright/tests/shared/focus/goal/record-page.spec.tsapps/e2e-playwright/tests/shared/focus/task/browse.spec.tsapps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/focus/task/context-menu.spec.tsapps/e2e-playwright/tests/shared/focus/task/creation.spec.tsapps/e2e-playwright/tests/shared/focus/task/record-page.spec.tsapps/e2e-playwright/tests/shared/memory/capture.spec.tsapps/e2e-playwright/tests/shared/memory/node/browse.spec.tsapps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.tsapps/e2e-playwright/tests/shared/memory/node/context-menu.spec.tsapps/e2e-playwright/tests/shared/memory/node/creation.spec.tsapps/e2e-playwright/tests/shared/memory/node/record-page.spec.tsapps/e2e-playwright/tests/shared/navigation.spec.tsapps/e2e-playwright/tests/shared/overview/overview.spec.tsapps/e2e-playwright/tests/shared/settings/settings.spec.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/e2e-playwright/tests/shared/focus/goal/bulk-editor.spec.ts
- apps/e2e-playwright/tests/shared/collection/collection.spec.ts
- apps/e2e-playwright/tests/shared/memory/capture.spec.ts
- apps/e2e-playwright/tests/shared/memory/node/browse.spec.ts
- apps/e2e-playwright/tests/shared/focus/task/creation.spec.ts
- apps/e2e-playwright/tests/shared/memory/node/creation.spec.ts
- apps/e2e-playwright/tests/shared/focus/goal/record-page.spec.ts
- apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts
- apps/e2e-playwright/tests/pointron/settings/settings.spec.ts
- apps/e2e-playwright/tests/shared/focus/goal/context-menu.spec.ts
- apps/e2e-playwright/tests/shared/focus/task/bulk-editor.spec.ts
- apps/e2e-playwright/tests/shared/memory/node/bulk-editor.spec.ts
- apps/e2e-playwright/tests/shared/focus/task/record-page.spec.ts
- apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts
| await page.route("**/*", (route) => { | ||
| const reqUrl = route.request().url(); | ||
| if (/accounts\.google\.com/i.test(reqUrl)) route.abort(); | ||
| else route.continue(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.ts | head -35Repository: 21nCo/nucleus
Length of output: 1289
🌐 Web query:
Playwright page.route handler best practices pattern matching URL regex
💡 Result:
URL matching for page.route() / context.route()
Prefer glob patterns for most cases
Playwright’s “URL string” patterns are globs (not regex) and are matched against the entire URL. Use them when you can—they’re readable and usually sufficient. (playwright.dev)
Common, reliable patterns:
- Match an endpoint anywhere:
**/api/login - Match a file type:
**/*.js - Match multiple extensions:
**/*.{png,jpg,jpeg} - Match across path segments: use
**(single*won’t cross/) (playwright.dev)
Use RegExp when matching rules are truly complex
When you need “real” regex logic (query-string rules, optional segments, etc.), pass a RegExp instead of a string glob. Playwright explicitly recommends regex for complex matching. (playwright.dev)
Regex best practices:
- Anchor intentionally: use
^...$if you mean “whole URL”, or a targeted suffix like/\/api\/v\d+\/users(\?.*)?$/. - Escape dots (
\.) and other metacharacters. - Avoid overly broad patterns like
/.*/(easy to accidentally intercept everything and slow tests).
Example:
await page.route(/^https:\/\/api\.example\.com\/v\d+\/users(\?.*)?$/, route => {
// ...
});Handler best practices (ordering, chaining, scope, cleanup)
Register routes before navigation (and before the request can fire)
If you set up a route after page.goto() (or after a user action that triggers requests), early requests may already be in-flight and won’t be intercepted (a common source of “why didn’t my route run?”). (browserstack.com)
Use browserContext.route() for “global” interception (popups/new pages)
Context-level routes apply to all pages in the context, including popups. Also, Playwright notes that for popups the earliest moment a Page exists is after the initial navigation has started—so if you need to intercept that very first request, you should use browserContext.route() (not page.route() on the popup). (playwright.dev)
Understand route priority + use route.fallback() to compose handlers
- If multiple routes match, the most recently registered handler runs first. (playwright.dev)
route.continue()sends the request immediately and stops any further matching handlers.route.fallback()continues the chain so earlier handlers can still run. (playwright.dev)
Pattern for layered routing:
// broad “default” last (lower priority)
await page.route('**/*', route => route.abort()); // example
// more specific first (higher priority)
await page.route('**/api/**', async route => {
// handle API, else:
await route.fallback();
});Matching uses the original request URL (even if you rewrite it)
If you call route.fallback({ url: ... }) (or otherwise override the URL), Playwright still matches routes using the original request URL—so don’t expect rewriting to change which handlers apply. (playwright.dev)
Clean up routes to prevent cross-test pollution
Remove routes you no longer need:
browserContext.unroute(url[, handler])(playwright.dev)browserContext.unrouteAll()(including HAR routes) (playwright.dev)
This is especially important in shared fixtures / long-lived contexts.
If you share a couple example URLs you’re trying to match (including query strings), I can propose the safest glob vs regex patterns and show how to structure multiple handlers without conflicts.
Citations:
- 1: https://playwright.dev/docs/network
- 2: https://playwright.dev/docs/network
- 3: https://playwright.dev/docs/network
- 4: https://www.browserstack.com/guide/playwright-intercept-request?utm_source=openai
- 5: https://playwright.dev/docs/network
- 6: https://playwright.dev/docs/api/class-route
- 7: https://playwright.dev/docs/api/class-route
- 8: https://playwright.dev/docs/api/class-route
- 9: https://playwright.dev/docs/api/class-browsercontext
- 10: https://playwright.dev/docs/api/class-browsercontext
Narrow the route pattern to avoid intercepting all requests.
The current pattern "**/*" intercepts every network request. Since the intent is only to block Google Accounts, use a more specific pattern to reduce routing overhead:
- await page.route("**/*", (route) => {
+ await page.route(/accounts\.google\.com/i, (route) => {
const reqUrl = route.request().url();
- if (/accounts\.google\.com/i.test(reqUrl)) route.abort();
- else route.continue();
+ route.abort();
});This narrows interception to the target domain only. Note: route.abort() and route.continue() are synchronous methods—no need for async/await.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.route("**/*", (route) => { | |
| const reqUrl = route.request().url(); | |
| if (/accounts\.google\.com/i.test(reqUrl)) route.abort(); | |
| else route.continue(); | |
| }); | |
| await page.route(/accounts\.google\.com/i, (route) => { | |
| route.abort(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.ts` around
lines 15 - 19, The page.route handler is currently matching every request via
the glob "**/*" which is unnecessary; change the route pattern to target the
Google Accounts domain only (e.g. "https://accounts.google.com/**" or a
host-only glob that includes accounts.google.com) and simplify the handler to
directly abort matching requests instead of checking reqUrl with a regex; you
can also remove any unnecessary async/await around
route.abort()/route.continue() since those are synchronous methods.
| test.skip("context menu actions from library task row", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "context-menu.spec.ts" 2>/dev/null | head -5Repository: 21nCo/nucleus
Length of output: 39
🏁 Script executed:
find . -type f -name "context-menu.spec.ts" 2>/dev/nullRepository: 21nCo/nucleus
Length of output: 258
🏁 Script executed:
cat -n apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.tsRepository: 21nCo/nucleus
Length of output: 1289
🏁 Script executed:
rg "test\.fixme" --type ts -A 2 | head -30Repository: 21nCo/nucleus
Length of output: 39
🏁 Script executed:
rg "test\.skip|test\.fixme" apps/e2e-playwright --type ts | head -20Repository: 21nCo/nucleus
Length of output: 2443
🏁 Script executed:
rg "@playwright/test" apps/e2e-playwright --type ts -A 5 | head -40Repository: 21nCo/nucleus
Length of output: 3085
🌐 Web query:
Playwright test.fixme API documentation
💡 Result:
Playwright’s test.fixme is documented here (official docs):
test.fixmeAPI (classTest) — signatures, behavior, and examples (declaring a fixme test; runtimetest.fixme(condition, description)/test.fixme(callback, description)/test.fixme()). [1]- Link:
https://playwright.dev/docs/next/api/class-test#test-fixme
- Link:
testInfo.fixmeAPI (classTestInfo) — runtime method that marks the currently running test as fixme and aborts it immediately (testInfo.fixme()/testInfo.fixme(condition, description)). [2]- Link:
https://playwright.dev/docs/next/api/class-testinfo#test-info-fixme
- Link:
- Conceptual overview: Annotations (explains that
test.fixme()marks as fixme and the test is not run; includes examples like using it inbeforeEach). [3]- Link:
https://playwright.dev/docs/next/test-annotations
- Link:
Key behavior: test.fixme() marks a test as “fixme” (intended to be fixed) and Playwright will not run the test past the test.fixme() call. [1]
Common patterns (from the docs):
- Declare a fixme test:
test.fixme(title, body)[1] - Conditionally fixme at runtime inside a test:
test.fixme(condition, description)[1] - Abort the currently running test via
testInfo:testInfo.fixme(condition, description)[2]
Sources: [1] [2] [3]
Replace skipped TODO stubs with test.fixme to remove inline comments and properly mark deferred tests.
Lines 23–26 and 30–33 contain test.skip with inline // TODO comments, which violates the coding guideline "never introduce inline comments when modifying code." Use test.fixme(title) instead—Playwright's standard API for marking tests as intentionally deferred. This removes both the inline comment violation and the skip pattern:
test.describe("from library", () => {
- test.skip("context menu actions from library task row", async ({ page }) => {
- await ensureInAppOnHome(page);
- // TODO
- });
+ test.fixme("context menu actions from library task row");
});
test.describe("from record page", () => {
- test.skip("context menu actions from task record page", async ({ page }) => {
- await ensureInAppOnHome(page);
- // TODO
- });
+ test.fixme("context menu actions from task record page");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.skip("context menu actions from library task row", async ({ page }) => { | |
| await ensureInAppOnHome(page); | |
| // TODO | |
| }); | |
| test.fixme("context menu actions from library task row"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/task/context-menu.spec.ts` around
lines 23 - 26, Replace the two test stubs that use test.skip with inline TODO
comments by marking them as deferred using Playwright's test.fixme API: find the
"test.skip(\"context menu actions from library task row\", async ({ page }) => {
... });" and the other skipped test at lines shown and change each invocation to
test.fixme("context menu actions from library task row") (and the other test's
title) so the tests are explicitly flagged as FIXME/deferred and remove the
inline "// TODO" comments; ensure you keep the original test titles and
signatures where applicable when converting to fixme calls.
| test.skip("open node record and assert panels/content visible", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO | ||
| }); | ||
|
|
||
| test.skip("tab switching and visibility on node record page", async ({ page }) => { | ||
| await ensureInAppOnHome(page); | ||
| // TODO | ||
| }); |
There was a problem hiding this comment.
Remove inline // TODO comments.
The coding guidelines specify not to introduce inline comments. Since these tests are already marked with test.skip, the pending status is clear. Consider tracking implementation work in an issue tracker or removing the comment bodies entirely.
Proposed fix
test.skip("open node record and assert panels/content visible", async ({ page }) => {
await ensureInAppOnHome(page);
- // TODO
});
test.skip("tab switching and visibility on node record page", async ({ page }) => {
await ensureInAppOnHome(page);
- // TODO
});As per coding guidelines: **/*.{ts,tsx,svelte,js,jsx}: "never introduce inline comments when modifying code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.skip("open node record and assert panels/content visible", async ({ page }) => { | |
| await ensureInAppOnHome(page); | |
| // TODO | |
| }); | |
| test.skip("tab switching and visibility on node record page", async ({ page }) => { | |
| await ensureInAppOnHome(page); | |
| // TODO | |
| }); | |
| test.skip("open node record and assert panels/content visible", async ({ page }) => { | |
| await ensureInAppOnHome(page); | |
| }); | |
| test.skip("tab switching and visibility on node record page", async ({ page }) => { | |
| await ensureInAppOnHome(page); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/memory/node/record-page.spec.ts` around
lines 22 - 30, Remove the inline `// TODO` comments from the skipped tests "open
node record and assert panels/content visible" and "tab switching and visibility
on node record page"; leave the tests as skipped with their calls to
ensureInAppOnHome(page) (or remove the empty bodies entirely) and track any work
in an issue tracker instead of inline comments—update the test functions (the
test.skip blocks) by deleting the `// TODO` lines so no inline comments remain.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
apps/e2e-playwright/global-setup.ts (1)
85-86: Consider using a static import forfs.The
fsmodule is dynamically imported inside the async function, but it's a built-in module that could be imported statically at the top of the file likepathandchild_processfor consistency.Proposed fix
import path from "node:path"; import { spawn, type ChildProcess } from "node:child_process"; +import fs from "node:fs"; import type { FullConfig } from "@playwright/test";Then remove the dynamic import:
- const fs = await import("node:fs"); const currentOrigin = new URL(baseURL).origin;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/global-setup.ts` around lines 85 - 86, Replace the dynamic import of the built-in fs module inside the async block by adding a static import at the top of the file (like the existing imports for path and child_process) and remove the line using "const fs = await import('node:fs')"; update any references that used that local fs variable to use the statically imported fs instead (look for the dynamic import in global-setup.ts and usages thereafter).apps/e2e-playwright/tests/pointron/app-nav.spec.ts (1)
103-107: Replace the remaining fixed sleep with a state-based wait.Line 106 uses
waitForTimeout(600)after clicking the Goals button. Replace with a state-based wait using the existinggoalsListVisiblepattern.Proposed fix
await goalsBtn.click({ timeout: 5_000 }); - await page.waitForTimeout(600); + await page + .getByTestId("search-goals") + .or(page.getByRole("button", { name: /Create new goal/i })) + .or(page.getByText(/Looks like you don't have any goals/i)) + .first() + .waitFor({ state: "visible", timeout: 10_000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts` around lines 103 - 107, Replace the fixed sleep after clicking the Goals button by waiting on the goals list visibility: after await goalsBtn.click(...) remove await page.waitForTimeout(600) and instead await goalsListVisible.waitFor({ state: "visible", timeout: 15_000 }) (or the same timeout used elsewhere), using the existing goalsListVisible locator to drive the state-based wait.apps/e2e-playwright/tests/shared/settings/settings.spec.ts (1)
670-673: Avoid swallowing assertion errors with.catch(() => null).Lines 672 and 724 use
.catch(() => null)onexpect()assertions. This pattern silently ignores assertion failures, which could mask real test issues. If these assertions are optional, consider using conditional checks instead.- await expect(page.getByText("Settings", { exact: true }).first()).toBeHidden({ - timeout: 5_000 - }).catch(() => null); + const settingsHidden = await page.getByText("Settings", { exact: true }).first().isHidden().catch(() => true); + // Proceed regardless - settings may already be closed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/settings/settings.spec.ts` around lines 670 - 673, The test is swallowing assertion failures by appending .catch(() => null) to expect(page.getByText("Settings", { exact: true }).first()).toBeHidden(...); remove the .catch and either make the assertion explicit or replace it with a conditional check: locate the element via page.getByText(...).first() (or a locator variable), call an existence/visibility check (e.g., isVisible() or count()) and branch accordingly for optional behavior, or wrap the expect in a try/catch that only suppresses known non-critical errors while rethrowing unexpected assertion failures so true test failures are not masked.apps/e2e-playwright/tests/shared/overview/overview.spec.ts (1)
34-42: Consider replacing fixed waits with state-based waits ornetworkidle.Lines 41, 82, 119, 158, 188, 215, and 257 all use
waitForTimeout(1_500)afterwaitForURL. Consider usingwaitUntil: 'networkidle'in the navigation or waiting for specific content visibility instead.Alternative approach
await page.waitForURL( (u) => new RegExp(`^${nucleusProductConfig.pathByNavLabel.Overview}(\\/.*)?$`).test( new URL(u).pathname ), - { timeout: 10_000 } + { timeout: 10_000, waitUntil: 'networkidle' } ); - await page.waitForTimeout(1_500);Or wait for a specific element that indicates the page is ready:
await page.getByRole("button", { name: /^All$/i }).first().waitFor({ state: "visible", timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts` around lines 34 - 42, The fixed 1_500ms sleep after page.waitForURL (calls to page.waitForTimeout(1_500)) is brittle; replace each waitForTimeout with a state-based wait — either pass waitUntil: 'networkidle' / use page.waitForLoadState('networkidle') after navigation (where waitForURL is used) or wait for a specific readiness element to appear (e.g., replace the timeout with await page.getByRole("button", { name: /^All$/i }).first().waitFor({ state: "visible", timeout: 10_000 });). Update every occurrence of waitForTimeout in overview.spec.ts to use one of these approaches so tests wait for real page readiness instead of a fixed delay.apps/e2e-playwright/tests/nucleus/app-nav.spec.ts (3)
206-214: Replace fixed sleep with state-based wait.Line 209 uses
waitForTimeout(600)after clicking Goals. The subsequent assertion already provides the wait condition.Proposed fix
await page.getByRole("button", { name: /^Goals(\s+\d+)?$/i }).first().click({ timeout: 5_000 }); - await page.waitForTimeout(600); await expect(page.getByTestId("search-goals")).toBeVisible({ timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` around lines 206 - 214, After clicking the Goals button (page.getByRole("button", { name: /^Goals(\s+\d+)?$/i }).first().click(...)), remove the fixed sleep await page.waitForTimeout(600) and instead rely on the state-based wait already used for the assertion: await expect(page.getByTestId("search-goals")).toBeVisible({ timeout: 10_000 }) (or optionally replace with page.getByTestId("search-goals").waitFor({ state: "visible", timeout: 10_000 })). This eliminates the flaky fixed delay and uses element visibility as the synchronization point.
74-85: Replace fixed sleeps with state-based waits.Lines 74 and 80 use
waitForTimeout(500)before clicking UI elements. Since the subsequent assertion on line 82-84 waits for "No sessions found", the fixed sleeps can be replaced with direct assertions.Proposed fix
await panelRow.getByRole("button").last().click({ timeout: 8_000 }); - await page.waitForTimeout(500); await page .locator("[id^='mdcontainer-']") .getByRole("button", { name: /^Focus$/i }) .click({ timeout: 5_000 }); - await page.waitForTimeout(500); await expect( page.getByText("No sessions found").first() ).toBeVisible({ timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` around lines 74 - 85, Replace the fixed 500ms sleeps with state-based waits: wait for the target button locator ("locator(\"[id^='mdcontainer-']\").getByRole(\"button\", { name: /^Focus$/i })") to be visible/enabled (e.g., use locator.waitFor or expect(locator).toBeVisible/ToBeEnabled) before calling click, and remove the pre-assertion wait before checking page.getByText("No sessions found").first().toBeVisible({ timeout: 10000 }) since that assertion already waits for the expected state.
233-250: Replace fixed sleeps with state-based waits.Lines 235, 239, and 245 use short fixed sleeps that could be removed since the subsequent assertions provide appropriate wait conditions.
Proposed fix
if (!checked) { await goalsRow.locator("label").click({ timeout: 2_000 }); - await page.waitForTimeout(300); } await page.keyboard.press("Escape"); - await page.waitForTimeout(300); await page .getByRole("button", { name: /^Goals(\s+\d+)?$/i }) .first() .click({ timeout: 5_000 }); - await page.waitForTimeout(600); await expect(page.getByTestId("search-goals")).toBeVisible({ timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts` around lines 233 - 250, Replace the fixed waitForTimeout sleeps with state-based waits: after clicking goalsRow.locator("label"), wait for the checkbox state (e.g., await expect(goalsRow.locator('input[type="checkbox"]')).toBeChecked() or toBeUnchecked() as appropriate) instead of waitForTimeout(300); after sending page.keyboard.press("Escape"), wait for the relevant UI to close (e.g., await expect(page.getByTestId("search-goals")).not.toBeVisible() or await expect(someMenuLocator).toBeHidden()) instead of waitForTimeout(300); keep the final expect(page.getByTestId("search-goals")).toBeVisible() after clicking the Goals button and remove the preceding waitForTimeout(600). Use the existing locators (goalsRow.locator("label"), page.keyboard.press, page.getByRole(...).first().click, page.getByTestId("search-goals")) to implement these assertions.apps/e2e-playwright/tests/memotron/app-nav.spec.ts (1)
108-112: Replace the remaining fixed sleep with a state-based wait.Line 111 still uses
waitForTimeout(600)after clicking the Nodes button. Since lines 114-119 already define a state-based locator for the nodes list, use it here instead.Proposed fix
await nodesBtn.click({ timeout: 5_000 }); - await page.waitForTimeout(600); + await page + .getByTestId("search-nodes") + .or(page.getByRole("button", { name: /Create new node|New node/i })) + .or(page.getByText(/No nodes|don't have any nodes/i)) + .first() + .waitFor({ state: "visible", timeout: 10_000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts` around lines 108 - 112, The hard-coded sleep call page.waitForTimeout(600) after clicking nodesBtn should be replaced with a state-based wait using the existing nodes-list locator defined later; remove page.waitForTimeout(600) and instead await the nodes list locator's waitFor with an appropriate state and timeout (for example await nodesList.waitFor({ state: "visible", timeout: 15_000 })), keeping the click on nodesBtn and using the existing nodes-list locator name used elsewhere in the test.apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts (2)
19-27: Scope the route interception to Google auth URLs only.Routing
**/*means every request in every test goes through this handler even though only one host is blocked. A targeted matcher is cheaper and less invasive.♻️ Narrow the route to the host you actually want to block
test.beforeEach(async ({ page }) => { - await page.route("**/*", (route) => { - const reqUrl = route.request().url(); - if (/accounts\.google\.com/i.test(reqUrl)) { - route.abort(); - return; - } - route.continue(); - }); + await page.route(/https:\/\/accounts\.google\.com\//i, (route) => { + route.abort(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` around lines 19 - 27, The route interception in test.beforeEach is too broad (page.route("**/*", ...)); narrow the matcher to only Google auth hosts by replacing the global pattern with a targeted one (e.g., a host-specific URL pattern matching accounts.google.com or *.google.com auth paths) so only requests to accounts.google.com are handled by the handler that calls route.abort() and all other requests use the default flow; update the call site in test.beforeEach where page.route is invoked to use the scoped matcher while keeping the existing request URL check and abort/continue logic.
54-55: Replace the heuristic sleeps with state-based waits where you can.These
waitForTimeout(...)calls are the main flake vector in this spec. Prefer waiting for the specific result of the preceding action—search result visible, dialog gone, focus controls updated, timeline entry rendered—instead of betting on fixed delays.Also applies to: 66-79, 99-106, 139-140, 158-163, 172-181, 199-220, 236-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` around lines 54 - 55, The spec uses heuristic waits (page.waitForTimeout) causing flakes; replace each sleep (e.g., the call before page.keyboard.press("Enter") and other occurrences) with state-based waits that assert the expected UI change: use page.waitForSelector / locator.waitFor / page.waitForFunction or Playwright expect matchers to wait for search results to appear, dialogs to disappear, focus controls to update, or timeline entries to render, and where appropriate wait for network activity with page.waitForResponse or route interception; update the sequences around keyboard.press("Enter"), dialog interactions, and timeline assertions (the blocks flagged at lines ~66-79, 99-106, 139-140, 158-163, 172-181, 199-220, 236-248) to wait for the specific selector/state that indicates the prior action completed instead of fixed timeouts.apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts (1)
46-46: Avoid regex-interpolatinglibraryPathinwaitForURL.These predicates assume the path string is regex-safe. If a future product path contains
.,+,(, etc., the match becomes broader or narrower than intended. Compareu.pathnamedirectly instead.♻️ Suggested change for both `waitForURL` calls
- await page.waitForURL((u) => new RegExp(`^${libraryPath}(\\/.*)?$`).test(new URL(u).pathname), { timeout: 10_000 }); + await page.waitForURL( + (u) => u.pathname === libraryPath || u.pathname.startsWith(`${libraryPath}/`), + { timeout: 10_000 } + );Also applies to: 113-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts` at line 46, The current page.waitForURL predicate builds a RegExp with libraryPath and tests new URL(u).pathname, which is unsafe if libraryPath contains regex metacharacters; change the predicate in the page.waitForURL calls (the ones using new RegExp(`^${libraryPath}(\\/.*)?$`) and testing new URL(u).pathname) to compare pathnames directly (e.g., check exact equality or startsWith on new URL(u).pathname against libraryPath and the expected trailing slash/segments) instead of interpolating libraryPath into a regular expression; update both occurrences that call page.waitForURL so they use direct pathname comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts`:
- Line 30: The test titles and an inline comment introduced non-ASCII and
inline-comment edits; locate the test declarations like test("open Library →
Goals and see goal in list", ...) and any other tests in the same file that use
the Unicode arrow (e.g., titles matching "Library →" or similar) and replace the
Unicode arrow with an ASCII equivalent such as "->" or the word "to" (e.g.,
"open Library -> Goals..." or "open Library to Goals..."); also remove any
inline '//' comments introduced inside executable code (ensure no inline
comments remain in the modified test blocks) and apply the same fixes to the
other affected test titles noted in the review.
- Around line 126-142: The current pattern swallows click failures by using
.then/.catch on newGoalBtn.click and the Close button click; instead ensure the
elements are visible/enabled before clicking: explicitly wait for newGoalBtn (or
fallback createNewGoalBtn) to be visible/enabled (use newGoalBtn.waitFor or
assert with expect(newGoalBtn).toBeVisible()/toBeEnabled()) and only then call
click, and do the same for the Close button found via page.getByRole; update the
flow around goalNameInput.fill and page.keyboard.press so clicks are not wrapped
in .catch but guarded by explicit visibility/enabled checks to surface real
failures.
In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts`:
- Line 2: Replace the hard-coded Nucleus import and literals by resolving the
active product config the same way ensureInAppOnHome does: remove the
nucleusProductConfig import and instead obtain the current product config (e.g.,
assign to a local productConfig variable via the shared resolver used by
ensureInAppOnHome), then drive all navigations/assertions (the nav label
"Library" and path "/library" and the other occurrences at lines ~80-93,
129-135, 165-235) from productConfig properties (use the product's nav label and
path keys used elsewhere in the test suite) so the shared tests work for any
product.
---
Nitpick comments:
In `@apps/e2e-playwright/global-setup.ts`:
- Around line 85-86: Replace the dynamic import of the built-in fs module inside
the async block by adding a static import at the top of the file (like the
existing imports for path and child_process) and remove the line using "const fs
= await import('node:fs')"; update any references that used that local fs
variable to use the statically imported fs instead (look for the dynamic import
in global-setup.ts and usages thereafter).
In `@apps/e2e-playwright/tests/memotron/app-nav.spec.ts`:
- Around line 108-112: The hard-coded sleep call page.waitForTimeout(600) after
clicking nodesBtn should be replaced with a state-based wait using the existing
nodes-list locator defined later; remove page.waitForTimeout(600) and instead
await the nodes list locator's waitFor with an appropriate state and timeout
(for example await nodesList.waitFor({ state: "visible", timeout: 15_000 })),
keeping the click on nodesBtn and using the existing nodes-list locator name
used elsewhere in the test.
In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts`:
- Around line 206-214: After clicking the Goals button (page.getByRole("button",
{ name: /^Goals(\s+\d+)?$/i }).first().click(...)), remove the fixed sleep await
page.waitForTimeout(600) and instead rely on the state-based wait already used
for the assertion: await expect(page.getByTestId("search-goals")).toBeVisible({
timeout: 10_000 }) (or optionally replace with
page.getByTestId("search-goals").waitFor({ state: "visible", timeout: 10_000
})). This eliminates the flaky fixed delay and uses element visibility as the
synchronization point.
- Around line 74-85: Replace the fixed 500ms sleeps with state-based waits: wait
for the target button locator
("locator(\"[id^='mdcontainer-']\").getByRole(\"button\", { name: /^Focus$/i
})") to be visible/enabled (e.g., use locator.waitFor or
expect(locator).toBeVisible/ToBeEnabled) before calling click, and remove the
pre-assertion wait before checking page.getByText("No sessions
found").first().toBeVisible({ timeout: 10000 }) since that assertion already
waits for the expected state.
- Around line 233-250: Replace the fixed waitForTimeout sleeps with state-based
waits: after clicking goalsRow.locator("label"), wait for the checkbox state
(e.g., await expect(goalsRow.locator('input[type="checkbox"]')).toBeChecked() or
toBeUnchecked() as appropriate) instead of waitForTimeout(300); after sending
page.keyboard.press("Escape"), wait for the relevant UI to close (e.g., await
expect(page.getByTestId("search-goals")).not.toBeVisible() or await
expect(someMenuLocator).toBeHidden()) instead of waitForTimeout(300); keep the
final expect(page.getByTestId("search-goals")).toBeVisible() after clicking the
Goals button and remove the preceding waitForTimeout(600). Use the existing
locators (goalsRow.locator("label"), page.keyboard.press,
page.getByRole(...).first().click, page.getByTestId("search-goals")) to
implement these assertions.
In `@apps/e2e-playwright/tests/pointron/app-nav.spec.ts`:
- Around line 103-107: Replace the fixed sleep after clicking the Goals button
by waiting on the goals list visibility: after await goalsBtn.click(...) remove
await page.waitForTimeout(600) and instead await goalsListVisible.waitFor({
state: "visible", timeout: 15_000 }) (or the same timeout used elsewhere), using
the existing goalsListVisible locator to drive the state-based wait.
In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts`:
- Line 46: The current page.waitForURL predicate builds a RegExp with
libraryPath and tests new URL(u).pathname, which is unsafe if libraryPath
contains regex metacharacters; change the predicate in the page.waitForURL calls
(the ones using new RegExp(`^${libraryPath}(\\/.*)?$`) and testing new
URL(u).pathname) to compare pathnames directly (e.g., check exact equality or
startsWith on new URL(u).pathname against libraryPath and the expected trailing
slash/segments) instead of interpolating libraryPath into a regular expression;
update both occurrences that call page.waitForURL so they use direct pathname
comparison.
In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts`:
- Around line 19-27: The route interception in test.beforeEach is too broad
(page.route("**/*", ...)); narrow the matcher to only Google auth hosts by
replacing the global pattern with a targeted one (e.g., a host-specific URL
pattern matching accounts.google.com or *.google.com auth paths) so only
requests to accounts.google.com are handled by the handler that calls
route.abort() and all other requests use the default flow; update the call site
in test.beforeEach where page.route is invoked to use the scoped matcher while
keeping the existing request URL check and abort/continue logic.
- Around line 54-55: The spec uses heuristic waits (page.waitForTimeout) causing
flakes; replace each sleep (e.g., the call before page.keyboard.press("Enter")
and other occurrences) with state-based waits that assert the expected UI
change: use page.waitForSelector / locator.waitFor / page.waitForFunction or
Playwright expect matchers to wait for search results to appear, dialogs to
disappear, focus controls to update, or timeline entries to render, and where
appropriate wait for network activity with page.waitForResponse or route
interception; update the sequences around keyboard.press("Enter"), dialog
interactions, and timeline assertions (the blocks flagged at lines ~66-79,
99-106, 139-140, 158-163, 172-181, 199-220, 236-248) to wait for the specific
selector/state that indicates the prior action completed instead of fixed
timeouts.
In `@apps/e2e-playwright/tests/shared/overview/overview.spec.ts`:
- Around line 34-42: The fixed 1_500ms sleep after page.waitForURL (calls to
page.waitForTimeout(1_500)) is brittle; replace each waitForTimeout with a
state-based wait — either pass waitUntil: 'networkidle' / use
page.waitForLoadState('networkidle') after navigation (where waitForURL is used)
or wait for a specific readiness element to appear (e.g., replace the timeout
with await page.getByRole("button", { name: /^All$/i }).first().waitFor({ state:
"visible", timeout: 10_000 });). Update every occurrence of waitForTimeout in
overview.spec.ts to use one of these approaches so tests wait for real page
readiness instead of a fixed delay.
In `@apps/e2e-playwright/tests/shared/settings/settings.spec.ts`:
- Around line 670-673: The test is swallowing assertion failures by appending
.catch(() => null) to expect(page.getByText("Settings", { exact: true
}).first()).toBeHidden(...); remove the .catch and either make the assertion
explicit or replace it with a conditional check: locate the element via
page.getByText(...).first() (or a locator variable), call an
existence/visibility check (e.g., isVisible() or count()) and branch accordingly
for optional behavior, or wrap the expect in a try/catch that only suppresses
known non-critical errors while rethrowing unexpected assertion failures so true
test failures are not masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f846bdf-fff7-4e79-a219-10b6284c4b9e
📒 Files selected for processing (10)
apps/e2e-playwright/global-setup.tsapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/scripts/save-google-auth-state.tsapps/e2e-playwright/tests/memotron/app-nav.spec.tsapps/e2e-playwright/tests/nucleus/app-nav.spec.tsapps/e2e-playwright/tests/pointron/app-nav.spec.tsapps/e2e-playwright/tests/shared/focus/goal/browse.spec.tsapps/e2e-playwright/tests/shared/focus/goal/creation.spec.tsapps/e2e-playwright/tests/shared/overview/overview.spec.tsapps/e2e-playwright/tests/shared/settings/settings.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/e2e-playwright/scripts/save-google-auth-state.ts
| }); | ||
|
|
||
| test.describe("from library", () => { | ||
| test("open Library → Goals and see goal in list", async ({ page }, testInfo) => { |
There was a problem hiding this comment.
Remove the new non-ASCII titles and code comment.
This file introduces Unicode → characters in test names and a // comment in executable code. Both violate the repo's edit rules for *.ts/*.js files.
As per coding guidelines, "Maintain ASCII-only edits unless files already contain extended characters, and never introduce inline comments when modifying code."
Also applies to: 54-54, 91-91, 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts` at line 30, The
test titles and an inline comment introduced non-ASCII and inline-comment edits;
locate the test declarations like test("open Library → Goals and see goal in
list", ...) and any other tests in the same file that use the Unicode arrow
(e.g., titles matching "Library →" or similar) and replace the Unicode arrow
with an ASCII equivalent such as "->" or the word "to" (e.g., "open Library ->
Goals..." or "open Library to Goals..."); also remove any inline '//' comments
introduced inside executable code (ensure no inline comments remain in the
modified test blocks) and apply the same fixes to the other affected test titles
noted in the review.
| const clicked = await newGoalBtn | ||
| .click({ timeout: 3_000 }) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
| if (!clicked) { | ||
| await createNewGoalBtn.click({ timeout: 5_000 }); | ||
| } | ||
|
|
||
| const goalNameInput = page.getByTestId("goal-name-input"); | ||
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await page | ||
| .getByRole("button", { name: /^Close$/i }) | ||
| .first() | ||
| .click({ timeout: 5_000 }) | ||
| .catch(() => null); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the relevant file
find . -name "browse.spec.ts" -path "*/e2e-playwright/tests/shared/focus/goal/*" | head -20Repository: 21nCo/nucleus
Length of output: 1828
🏁 Script executed:
# Get the file size and read the specific lines
wc -l apps/e2e-playwright/tests/shared/focus/goal/browse.spec.tsRepository: 21nCo/nucleus
Length of output: 1828
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '120,145p' apps/e2e-playwright/tests/shared/focus/goal/browse.spec.tsRepository: 21nCo/nucleus
Length of output: 1828
🌐 Web query:
Playwright isVisible method E2E testing documentation
💡 Result:
Playwright isVisible() / isVisible docs (E2E)
Recommended for E2E assertions (auto-waits / retries):
- Use web-first assertions like
await expect(locator).toBeVisible()—these wait until the condition is met (or time out). (playwright.dev)
import { test, expect } from '@playwright/test';
test('banner is shown', async ({ page }) => {
const banner = page.getByTestId('banner');
await expect(banner).toBeVisible(); // auto-waits
});For conditional logic (no waiting):
await locator.isVisible()returns a boolean immediately and does not wait for the element to become visible (thetimeoutoption is ignored). Preferexpect(...).toBeVisible()when you’re asserting visibility to avoid flakiness. (playwright.dev)
const dialog = page.getByRole('dialog', { name: 'Cookie consent' });
if (await dialog.isVisible()) {
await page.getByRole('button', { name: 'Accept' }).click();
}Avoid in new tests: page.isVisible(selector)
page.isVisible()is discouraged in favor of locator-based APIs, does not wait, and a selector that matches nothing is considered not visible. (playwright.dev)
Related core docs to read:
- Locators overview (why locators are central to auto-waiting). (playwright.dev)
- Locator API (
locator.isVisible()+ guidance to preferexpect(locator).toBeVisible()). (playwright.dev) - Test assertions (auto-retrying async matchers like
toBeVisible). (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/test-assertions
- 2: https://playwright.dev/docs/next/api/class-locator
- 3: https://playwright.dev/docs/next/api/class-page
- 4: https://playwright.dev/docs/locators
- 5: https://playwright.dev/docs/next/api/class-locator
- 6: https://playwright.dev/docs/test-assertions
🌐 Web query:
Playwright click error handling best practices avoid catch blocks
💡 Result:
Playwright click error-handling (without sprinkling try/catch everywhere)
1) Let Playwright “do the waiting” (and fail loudly when it can’t)
locator.click() already auto-waits by running actionability checks (visible, enabled, stable, etc.). Most click “errors” are really “not actionable within timeout” problems—so the fix is usually better waiting/locators, not catching exceptions. [1]
2) Assert the preconditions you actually need, then click
Instead of catching a click failure, make the required UI state explicit:
const button = page.getByRole('button', { name: 'Submit' });
await expect(button).toBeVisible();
await expect(button).toBeEnabled();
await button.click();Playwright’s web-first expect(...) assertions auto-retry until the condition is met (or timeouts), reducing flakiness without manual loops/catches. [8]
3) Use resilient locators (avoid “sometimes it clicks the wrong thing”)
Prefer role/label/test-id style locators; avoid ambiguous selectors that can match multiple elements (strictness is a feature, not an annoyance). [4]
4) For “eventually” behaviors, use expect.toPass() / expect.poll() (not try/catch retries)
When the condition is more complex than a single built-in web assertion (or you need to retry a small sequence), wrap it:
await expect(async () => {
await page.getByRole('button', { name: 'Refresh' }).click();
await expect(page.getByText('Updated')).toBeVisible();
}).toPass();Playwright explicitly recommends expect.toPass / expect.poll for retriable assertions/flows. [8]
5) Avoid force: true except as a last-resort diagnostic
force bypasses important actionability checks, which often hides real UI bugs and can make tests less meaningful. Use it only to confirm whether the issue is actionability-related. [1]
6) Prefer runner-level retries + traces over per-click catching
If you’re dealing with intermittent infra/CI hiccups, configure test retries (not ad-hoc catch around actions). [5]
Also enable tracing on retry so you can debug the real cause instead of swallowing it:
// playwright.config.ts
export default defineConfig({
retries: 2,
use: { trace: 'on-first-retry' },
});Playwright supports retries and trace modes like on-first-retry. [6]
Quick “replace try/catch” checklist
- Click fails? Add/adjust precondition assertions (
toBeVisible,toBeEnabled,toHaveText, etc.). [8] - Flaky timing? Use
expect.toPass()/expect.poll()around the smallest retryable block. [8] - Ambiguous element? Fix the locator (role/label/test-id), don’t catch the strict-mode error. [4]
- CI-only flakes? Use retries + trace on retry, then fix root cause. [5][6]
Sources: Playwright actionability/auto-waiting [1], best practices [2], locators guidance [4], assertions (toPass, poll) [8], retries/config/trace [5][6].
Replace click-failure catching with explicit visibility checks.
Using .catch(() => false/null) to handle click failures masks real regressions. If the New goal or Close button is rendered but becomes unclickable, this test silently takes the alternate path instead of failing. Playwright best practice is to assert the element state explicitly before clicking.
♻️ Recommended approach using Playwright's web-first assertions
- const clicked = await newGoalBtn
- .click({ timeout: 3_000 })
- .then(() => true)
- .catch(() => false);
- if (!clicked) {
- await createNewGoalBtn.click({ timeout: 5_000 });
- }
+ if (await newGoalBtn.isVisible()) {
+ await newGoalBtn.click({ timeout: 5_000 });
+ } else {
+ await createNewGoalBtn.waitFor({ state: "visible", timeout: 5_000 });
+ await createNewGoalBtn.click({ timeout: 5_000 });
+ }
...
- await page
- .getByRole("button", { name: /^Close$/i })
- .first()
- .click({ timeout: 5_000 })
- .catch(() => null);
+ const closeButton = page.getByRole("button", { name: /^Close$/i }).first();
+ if (await closeButton.isVisible()) {
+ await closeButton.click({ timeout: 5_000 });
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/browse.spec.ts` around lines 126
- 142, The current pattern swallows click failures by using .then/.catch on
newGoalBtn.click and the Close button click; instead ensure the elements are
visible/enabled before clicking: explicitly wait for newGoalBtn (or fallback
createNewGoalBtn) to be visible/enabled (use newGoalBtn.waitFor or assert with
expect(newGoalBtn).toBeVisible()/toBeEnabled()) and only then call click, and do
the same for the Close button found via page.getByRole; update the flow around
goalNameInput.fill and page.keyboard.press so clicks are not wrapped in .catch
but guarded by explicit visibility/enabled checks to surface real failures.
| @@ -0,0 +1,261 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
| import { nucleusProductConfig } from "../../../../config/nucleus-product.config"; | |||
There was a problem hiding this comment.
Resolve the active product config instead of hard-coding Nucleus here.
This file sits under tests/shared, but it imports nucleusProductConfig and also hard-codes Library / /library. That makes the flow Nucleus-only, so the shared suite will break as soon as another product uses different nav labels or paths. Please resolve the current product config once and drive these navigations/assertions from it, the same way ensureInAppOnHome already does.
Also applies to: 80-93, 129-135, 165-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/shared/focus/goal/creation.spec.ts` at line 2,
Replace the hard-coded Nucleus import and literals by resolving the active
product config the same way ensureInAppOnHome does: remove the
nucleusProductConfig import and instead obtain the current product config (e.g.,
assign to a local productConfig variable via the shared resolver used by
ensureInAppOnHome), then drive all navigations/assertions (the nav label
"Library" and path "/library" and the other occurrences at lines ~80-93,
129-135, 165-235) from productConfig properties (use the product's nav label and
path keys used elsewhere in the test suite) so the shared tests work for any
product.
| * Memotron product config for E2E. Mirrors client/products/product.config.ts for Memotron | ||
| * so tests use home path and app menu labels instead of hardcoding. | ||
| */ | ||
| export const memotronProductConfig = { |
There was a problem hiding this comment.
@Sriya-Mukkawar, Is there anyway we can use the central existing product config file instead of duplicating here?
Like for example, if I want to change the app menu for Nucleus - now I have to change it in both product config file and here in test config file.
There was a problem hiding this comment.
I have added a single shared config file (client/products/product-nav.config.ts) that holds the app menu (IDs, labels, paths) for all products. The app’s product.config.ts and the E2E test configs both use this file (E2E just re-exports it), so changing something is now a single edit in that one place instead of updating both the product config and the test config.
There was a problem hiding this comment.
Thanks Sriya, looks good now.
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
2 participants
Summary by cubic
Unifies E2E testing across Nucleus, Pointron, and Memotron with a shared, single-source product nav config used by both the app and tests. Adds saved Google auth, per‑project base URLs, and stronger selectors to expand coverage across auth, nav, settings, focus, memory, and overview for TIDY-430.
New Features
.envloading; save‑auth scripts per product (tsx) with auto‑load from.auth/user*.json; supports APP_BASE_URL and APP_BASE_URL_; warns on origin mismatch; global setup skips dev server when a base URL is set and kills on startup timeout.Migration
.authis gitignored.Written for commit 7d9a70d. Summary will update on new commits.
Summary by CodeRabbit
Tests
Documentation
Chores