Open
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
Contributor
Reviewer's GuideImplements an end-to-end Playwright regression test for the login/signup flow redirecting to the calendar, adds environment-controlled bypass behaviour for login redirect failures, makes appearance typeface access null-safe via optional chaining, and updates the E2E project dependencies and appearance type documentation accordingly. Sequence diagram for Playwright login/signup regression testsequenceDiagram
actor Developer
participant Env as Env_vars
participant Runner as Playwright_runner
participant Browser
participant WebApp
participant AuthService
participant Router
Developer->>Runner: npm test:regression
Runner->>Env: Read SKIP_E2E
alt SKIP_E2E_enabled
Runner-->>Developer: Skip_E2E_suite
else SKIP_E2E_disabled
Runner->>Browser: Launch_browser
Browser->>WebApp: GET local.nucleus.to
WebApp-->>Browser: Landing_page
Browser->>WebApp: Click_Login_or_Signup
WebApp->>AuthService: Authenticate_user
AuthService-->>WebApp: Auth_success_with_session
WebApp->>Router: Request_post_login_route
Router-->>Browser: Redirect_to_calendar
Runner->>Env: Read E2E_BYPASS_LOGIN_FAILURE
alt Redirect_successful
Runner-->>Developer: Assert_path_is_calendar_pass
else Redirect_failed
alt E2E_BYPASS_LOGIN_FAILURE_disabled
Runner-->>Developer: Test_failure_redirect_not_calendar
else E2E_BYPASS_LOGIN_FAILURE_enabled
Runner-->>Developer: Annotate_and_exit_early
end
end
end
Class diagram for updated appearance and typeface handlingclassDiagram
class AppearancePreferences {
string? typeface
string theme
string colorScheme
}
class AppearanceStore {
boolean isSyncWithSystem
Theme systemTheme
string? typeface
}
class UserPreferences {
AppearancePreferences appearance
number accessibilitySizingFactor
}
class ThemeLayer {
+string typeface
+string defaultTypeface
+refreshSizing()
+refreshTailwind()
+onUserPreferencesChange(preferences)
}
class AppearanceSettings {
+renderTypefaceSelector(userPreferences)
+onTypefaceChange(newTypeface)
}
class FocusPlayer {
+string fallBackTypefaceString
+boolean isPipShown
+AppearancePreferences appearance
+renderPlayer()
}
UserPreferences "1" --> "1" AppearancePreferences : has
AppearanceStore "1" --> "1" AppearancePreferences : syncs_to
ThemeLayer --> UserPreferences : subscribes_to
ThemeLayer --> AppearancePreferences : reads_optional_typeface
AppearanceSettings --> UserPreferences : reads_optional_typeface
AppearanceSettings --> AppearancePreferences : updates_typeface
FocusPlayer --> AppearancePreferences : reads_optional_typeface_for_font
Flow diagram for E2E login test environment flag behaviourflowchart TD
A[Start Playwright regression suite] --> B{SKIP_E2E == 1?}
B -- Yes --> C[Skip all E2E tests]
C --> Z[End]
B -- No --> D[Run login_signup_redirect_test]
D --> E[Execute login or signup flow]
E --> F{Redirects to /calendar?}
F -- Yes --> G[Assert URL is /calendar]
G --> H[Mark test as passed]
H --> Z
F -- No --> I{E2E_BYPASS_LOGIN_FAILURE == 0?}
I -- Yes --> J[Fail test due to redirect regression]
J --> Z
I -- No --> K[Annotate test and exit early]
K --> Z
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoAdd login flow E2E test and fix typeface optional chaining
WalkthroughsDescription• Added Playwright login flow test with email/password authentication • Fixed optional chaining for appearance typeface property access • Added environment variable handling for test bypass logic • Reformatted package.json to single-line format Diagramflowchart LR
A["E2E Test Suite"] -->|"Add login/signup flow"| B["Navigation Test"]
C["Appearance Components"] -->|"Fix optional chaining"| D["Typeface Property Access"]
E["Package Configuration"] -->|"Reformat"| F["package.json"]
B -->|"Environment bypass"| G["Test Resilience"]
File Changes1. apps/e2e-playwright/tests/regression/navigation.spec.ts
|
Contributor
📝 WalkthroughWalkthroughThis PR establishes comprehensive Playwright-based end-to-end testing infrastructure for the Nucleus application, adding authentication state handling, global setup configuration, extensive test suites for core features (focus goals/tasks, memory nodes, calendar, navigation), testing utilities, and test identifiers across multiple UI components to enable reliable test selectors. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant GlobalSetup as Global Setup
participant Browser as Browser
participant App as Nucleus App
participant AuthState as Auth State File
Note over GlobalSetup: Initialization Phase
Test->>GlobalSetup: Start Playwright tests
GlobalSetup->>AuthState: Check if .auth/user.json exists
alt Auth State Exists
AuthState-->>GlobalSetup: Return saved auth state
GlobalSetup->>Browser: Launch Chrome (headless)
Browser->>App: Navigate to APP_BASE_URL
App-->>Browser: Load authenticated session
else Auth State Missing
GlobalSetup->>Browser: Launch Vite dev server (apps/nucleus)
Browser->>App: Start dev app
App-->>Browser: Dev server ready
GlobalSetup->>Browser: Assign baseURL
end
GlobalSetup-->>Test: Setup complete
Note over Test: Test Execution Phase
Test->>Browser: Execute test steps
Browser->>App: Navigate, interact
App-->>Browser: Render UI
Browser->>Test: Capture results
Test->>Test: Assert outcomes
sequenceDiagram
participant Test as E2E Test
participant Auth as Auth Script
participant Browser as Playwright Browser
participant Google as Google OAuth
participant App as Nucleus App
participant Disk as Disk (.auth/user.json)
Note over Auth: One-time Auth Setup
Auth->>Browser: Launch Chrome (non-headless)
Browser->>App: Navigate to /login
App-->>Browser: Show "Continue with Google"
Browser->>Google: Click Google sign-in button
Google->>Google: OAuth flow
Google-->>Browser: Redirect with auth code
Browser->>App: Complete OAuth callback
App-->>Browser: Redirect to home/calendar
Browser->>Disk: Save storage state (user.json)
Note over Test: Subsequent Tests
Test->>Disk: Load .auth/user.json
Disk-->>Test: Return saved auth state
Test->>Browser: Configure with storageState
Browser->>App: Inject auth into session
App-->>Browser: App renders authenticated
Test->>Test: Execute test scenario
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/e2e-playwright/tests/regression/navigation.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/regression/navigation.spec.ts:26">
P1: Avoid hardcoding login credentials in the test; pull the username from environment/secret configuration to prevent credential leakage.</violation>
<violation number="2" location="apps/e2e-playwright/tests/regression/navigation.spec.ts:28">
P0: Do not commit plaintext passwords. Load the password from a secure environment variable or test secret store.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts`:
- Around line 26-29: The test currently uses hardcoded credentials in the
Playwright steps (the calls to page.getByPlaceholder("username@email.com"),
page.getByPlaceholder("********"), and the subsequent getByRole("button", ...)
login flow); replace those literal email/password strings with environment
variables (e.g. process.env.E2E_USERNAME and process.env.E2E_PASSWORD), load
them via your preferred secrets mechanism (dotenv or CI secrets), and add a
guard that fails the test with a clear error if the env vars are missing so
tests don’t accidentally run with empty credentials.
- Around line 3-6: Currently shouldBypassLoginFailure defaults to true via the
globalThis indirection, which makes the login assertion a no-op; change logic to
default to false so bypass only occurs when E2E_BYPASS_LOGIN_FAILURE is
explicitly set (e.g., check process.env.E2E_BYPASS_LOGIN_FAILURE === "1" or
similar) and replace the globalThis runtimeEnv reference with direct process.env
access; update any uses of shouldBypassLoginFailure in navigation.spec.ts to
reflect the inverted meaning.
In `@client/products/pointron/focus/player/FocusPlayer.svelte`:
- Around line 204-206: The style string uses Svelte template expression syntax
inside a plain JS string so the font-family resolves to the literal text; change
the ternary branch that builds the style to use a JavaScript template literal
that interpolates $appearance?.typeface ?? fallBackTypefaceString when
isPipShown is true (update the expression where style is set in
FocusPlayer.svelte, referencing isPipShown, $appearance and
fallBackTypefaceString) so the actual font-family value is injected instead of
the literal braces.
🧹 Nitpick comments (2)
client/layout/layers/themeLayer/ThemeLayer.svelte (1)
41-42: Optional chaining for typeface looks good, but note inconsistency with otherappearanceaccesses below.Lines 50–53 still access
x.appearance.lightColorSchemeId,x.appearance.darkColorSchemeId, etc. without optional chaining. Ifappearancecan beundefined, those paths will throw at runtime. Consider applying the same guard consistently.apps/e2e-playwright/tests/regression/navigation.spec.ts (1)
8-11: Same simplification applies to the skip flag.-test.skip( - runtimeEnv?.SKIP_E2E === "1", +test.skip( + process.env.SKIP_E2E === "1", "E2E suite disabled by environment" );
Comment on lines
+3
to
+6
| const runtimeEnv = ( | ||
| globalThis as { process?: { env?: Record<string, string | undefined> } } | ||
| ).process?.env; | ||
| const shouldBypassLoginFailure = runtimeEnv?.E2E_BYPASS_LOGIN_FAILURE !== "0"; |
Contributor
There was a problem hiding this comment.
Bypass flag defaults to true, making the login assertion effectively a no-op.
shouldBypassLoginFailure is true unless E2E_BYPASS_LOGIN_FAILURE is explicitly "0". This means the test silently passes even when login fails, defeating its purpose. Consider inverting the default so the test fails unless bypass is explicitly opted into.
Also, since Playwright tests run in Node, you can use process.env directly instead of the globalThis indirection.
Proposed fix — invert default and simplify env access
-const runtimeEnv = (
- globalThis as { process?: { env?: Record<string, string | undefined> } }
-).process?.env;
-const shouldBypassLoginFailure = runtimeEnv?.E2E_BYPASS_LOGIN_FAILURE !== "0";
+const shouldBypassLoginFailure = process.env.E2E_BYPASS_LOGIN_FAILURE === "1";🤖 Prompt for AI Agents
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 3 - 6,
Currently shouldBypassLoginFailure defaults to true via the globalThis
indirection, which makes the login assertion a no-op; change logic to default to
false so bypass only occurs when E2E_BYPASS_LOGIN_FAILURE is explicitly set
(e.g., check process.env.E2E_BYPASS_LOGIN_FAILURE === "1" or similar) and
replace the globalThis runtimeEnv reference with direct process.env access;
update any uses of shouldBypassLoginFailure in navigation.spec.ts to reflect the
inverted meaning.
Comment on lines
+26
to
+29
| await page.getByPlaceholder("username@email.com").fill("sriyamukkawar@gmail.com"); | ||
| await page.getByRole("button", { name: "Enter password", exact: true }).click(); | ||
| await page.getByPlaceholder("********").fill("Sriya@1234"); | ||
| await page.getByRole("button", { name: "Log in", exact: true }).last().click(); |
Contributor
There was a problem hiding this comment.
Hardcoded credentials must not be committed to source control.
This leaks a real email address and password into the repository history. Move these to environment variables (or a Playwright .env / secrets store) and reference them via process.env.
🔒 Proposed fix — read credentials from environment
- await page.getByPlaceholder("username@email.com").fill("sriyamukkawar@gmail.com");
- await page.getByRole("button", { name: "Enter password", exact: true }).click();
- await page.getByPlaceholder("********").fill("Sriya@1234");
+ const email = process.env.E2E_LOGIN_EMAIL;
+ const password = process.env.E2E_LOGIN_PASSWORD;
+ if (!email || !password) {
+ test.skip(true, "E2E_LOGIN_EMAIL / E2E_LOGIN_PASSWORD not set");
+ return;
+ }
+ await page.getByPlaceholder("username@email.com").fill(email);
+ await page.getByRole("button", { name: "Enter password", exact: true }).click();
+ await page.getByPlaceholder("********").fill(password);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await page.getByPlaceholder("username@email.com").fill("sriyamukkawar@gmail.com"); | |
| await page.getByRole("button", { name: "Enter password", exact: true }).click(); | |
| await page.getByPlaceholder("********").fill("Sriya@1234"); | |
| await page.getByRole("button", { name: "Log in", exact: true }).last().click(); | |
| const email = process.env.E2E_LOGIN_EMAIL; | |
| const password = process.env.E2E_LOGIN_PASSWORD; | |
| if (!email || !password) { | |
| test.skip(true, "E2E_LOGIN_EMAIL / E2E_LOGIN_PASSWORD not set"); | |
| return; | |
| } | |
| await page.getByPlaceholder("username@email.com").fill(email); | |
| await page.getByRole("button", { name: "Enter password", exact: true }).click(); | |
| await page.getByPlaceholder("********").fill(password); | |
| await page.getByRole("button", { name: "Log in", exact: true }).last().click(); |
🤖 Prompt for AI Agents
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 26 -
29, The test currently uses hardcoded credentials in the Playwright steps (the
calls to page.getByPlaceholder("username@email.com"),
page.getByPlaceholder("********"), and the subsequent getByRole("button", ...)
login flow); replace those literal email/password strings with environment
variables (e.g. process.env.E2E_USERNAME and process.env.E2E_PASSWORD), load
them via your preferred secrets mechanism (dotenv or CI secrets), and add a
guard that fails the test with a clear error if the env vars are missing so
tests don’t accidentally run with empty credentials.
Comment on lines
204
to
206
| style={isPipShown | ||
| ? "font-family: {$appearance.typeface ?? fallBackTypefaceString};" | ||
| ? "font-family: {$appearance?.typeface ?? fallBackTypefaceString};" | ||
| : ""} |
Contributor
There was a problem hiding this comment.
Bug: Svelte expression syntax {...} inside a JS string literal is not interpolated.
The curly braces are treated as literal text here because this is a JavaScript ternary, not Svelte template markup. The rendered font-family will be the literal string {$appearance?.typeface ?? fallBackTypefaceString} instead of the resolved value.
🐛 Proposed fix — use a JS template literal
style={isPipShown
- ? "font-family: {$appearance?.typeface ?? fallBackTypefaceString};"
+ ? `font-family: ${$appearance?.typeface ?? fallBackTypefaceString};`
: ""}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| style={isPipShown | |
| ? "font-family: {$appearance.typeface ?? fallBackTypefaceString};" | |
| ? "font-family: {$appearance?.typeface ?? fallBackTypefaceString};" | |
| : ""} | |
| style={isPipShown | |
| ? `font-family: ${$appearance?.typeface ?? fallBackTypefaceString};` | |
| : ""} |
🤖 Prompt for AI Agents
In `@client/products/pointron/focus/player/FocusPlayer.svelte` around lines 204 -
206, The style string uses Svelte template expression syntax inside a plain JS
string so the font-family resolves to the literal text; change the ternary
branch that builds the style to use a JavaScript template literal that
interpolates $appearance?.typeface ?? fallBackTypefaceString when isPipShown is
true (update the expression where style is set in FocusPlayer.svelte,
referencing isPipShown, $appearance and fallBackTypefaceString) so the actual
font-family value is injected instead of the literal braces.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+26
to
+29
| await page.getByPlaceholder("username@email.com").fill("sriyamukkawar@gmail.com"); | ||
| await page.getByRole("button", { name: "Enter password", exact: true }).click(); | ||
| await page.getByPlaceholder("********").fill("Sriya@1234"); | ||
| await page.getByRole("button", { name: "Log in", exact: true }).last().click(); |
There was a problem hiding this comment.
1. Hardcoded login credentials 📘 Rule violation ⛨ Security
The new Playwright test hardcodes an email and password directly in source control, which constitutes committed secrets/credentials. This can lead to credential leakage and unauthorized access if reused elsewhere.
Agent Prompt
## Issue description
The Playwright test commits an email/password directly in the repository, which violates the no-secrets policy.
## Issue Context
Credentials should be injected at runtime via environment variables / CI secrets (and ideally use a dedicated test account), not stored in code.
## Fix Focus Areas
- apps/e2e-playwright/tests/regression/navigation.spec.ts[26-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+15
to
+33
| await page.goto("http://local.nucleus.to/"); | ||
| await page.waitForLoadState("domcontentloaded"); | ||
| const loginSignupButton = page.getByRole("button", { name: "Login/Signup" }); | ||
| await expect(loginSignupButton).toBeVisible({ | ||
| timeout: 30_000 | ||
| }); | ||
| await loginSignupButton.click(); | ||
|
|
||
| const navVisible = await app.isNavigationVisible(); | ||
| expect(navVisible).toBe(true); | ||
| await expect(page).toHaveURL(/\/account\/login/); | ||
| await page.getByRole("button", { name: "Log in", exact: true }).first().click(); | ||
|
|
||
| await page.getByPlaceholder("username@email.com").fill("sriyamukkawar@gmail.com"); | ||
| await page.getByRole("button", { name: "Enter password", exact: true }).click(); | ||
| await page.getByPlaceholder("********").fill("Sriya@1234"); | ||
| await page.getByRole("button", { name: "Log in", exact: true }).last().click(); | ||
|
|
||
| const redirectedToCalendar = await page | ||
| .waitForURL(/local\.nucleus\.to\/calendar\/?$/, { | ||
| timeout: 20_000 |
There was a problem hiding this comment.
2. Hardcoded url ignores baseurl 🐞 Bug ✓ Correctness
The regression test hardcodes http://local.nucleus.to/ and asserts a local.nucleus.to/calendar redirect, but Playwright is configured to run against APP_BASE_URL set by global-setup to a local Vite server. This will fail in CI/local runs or hit the wrong environment.
Agent Prompt
### Issue description
Test hardcodes an external domain and URL assertions, conflicting with Playwright `baseURL`.
### Issue Context
`global-setup.ts` sets `APP_BASE_URL` to the started Vite server; `playwright.config.ts` uses it as `baseURL`. Tests should use relative navigation (`page.goto('/')`) and relative URL expectations.
### Fix Focus Areas
- apps/e2e-playwright/tests/regression/navigation.spec.ts[15-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+42
| if (x.appearance?.typeface !== typeface) { | ||
| typeface = x.appearance?.typeface ?? defaultTypeface; |
There was a problem hiding this comment.
3. Preferences loader drops appearance 🐞 Bug ⛯ Reliability
Client code is being updated to treat userPreferences.appearance as optional in a few places, but the underlying store loader can still overwrite seeded preferences with server data that omits appearance, causing runtime crashes in other call sites that still dereference appearance without guards.
Agent Prompt
### Issue description
`userPreferences` can lose the `appearance` object if the server payload omits it, because the KV loader replaces the whole store value.
### Issue Context
`seedUserPreferences` defines `appearance`, but `UserPreferencesStore.loader` does not enforce it. Downstream code still dereferences `x.appearance.*`.
### Fix Focus Areas
- client/components/settings/userPreferences.store.ts[68-86]
- client/components/flux/resourceStores/kv.store.ts[74-79]
- client/layout/layers/themeLayer/ThemeLayer.svelte[49-56]
- client/components/settings/appearance/AppearanceSettings.svelte[20-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
apps/e2e-playwright/package.json
Outdated
| "@playwright/test": "^1.49.0" | ||
| } | ||
| } | ||
| {"name":"e2e-playwright","private":true,"scripts":{"test":"playwright test","test:smoke":"playwright test --project=smoke","test:regression":"playwright test --project=regression","test:ci":"playwright test --reporter=line"},"devDependencies":{"@playwright/test":"^1.49.0","@types/node":"^22.0.0"}} |
There was a problem hiding this comment.
4. Lockfile not updated 🐞 Bug ⛯ Reliability
apps/e2e-playwright/package.json adds @types/node, but the root package-lock.json workspace entry for apps/e2e-playwright does not include it. With npm workspaces, this commonly breaks `npm ci` due to lockfile/package.json mismatch.
Agent Prompt
### Issue description
Workspace package.json and root package-lock.json are out of sync after adding `@types/node`.
### Issue Context
npm workspaces typically require lockfile updates for workspace dependency changes; CI often uses `npm ci`.
### Fix Focus Areas
- apps/e2e-playwright/package.json[1-1]
- package-lock.json[76-80]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The login flow test hardcodes a real email, password, and base URL; consider reading these from environment variables or test fixtures instead to avoid leaking credentials and to make the test portable across environments.
- The redirectedToCalendar boolean built via waitForURL().then(...).catch(...) could be simplified and made more idiomatic by using Playwright's built-in expect(page).toHaveURL(/calendar/, { timeout: ... }) and handling the bypass condition around that expectation.
- The package.json for e2e-playwright was reformatted into a single line, which makes future diffs harder to read; consider preserving the original multiline JSON formatting when adding @types/node.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The login flow test hardcodes a real email, password, and base URL; consider reading these from environment variables or test fixtures instead to avoid leaking credentials and to make the test portable across environments.
- The redirectedToCalendar boolean built via waitForURL().then(...).catch(...) could be simplified and made more idiomatic by using Playwright's built-in expect(page).toHaveURL(/calendar/, { timeout: ... }) and handling the bypass condition around that expectation.
- The package.json for e2e-playwright was reformatted into a single line, which makes future diffs harder to read; consider preserving the original multiline JSON formatting when adding @types/node.
## Individual Comments
### Comment 1
<location> `apps/e2e-playwright/tests/regression/navigation.spec.ts:6` </location>
<code_context>
+const runtimeEnv = (
+ globalThis as { process?: { env?: Record<string, string | undefined> } }
+).process?.env;
+const shouldBypassLoginFailure = runtimeEnv?.E2E_BYPASS_LOGIN_FAILURE !== "0";
test.skip(
</code_context>
<issue_to_address>
**issue (testing):** Bypass flag makes the login test potentially misleading by defaulting to bypassing failures
Because `E2E_BYPASS_LOGIN_FAILURE !==
</issue_to_address>
### Comment 2
<location> `apps/e2e-playwright/tests/regression/navigation.spec.ts:26-28` </location>
<code_context>
+ await expect(page).toHaveURL(/\/account\/login/);
+ await page.getByRole("button", { name: "Log in", exact: true }).first().click();
+
+ await page.getByPlaceholder("username@email.com").fill("sriyamukkawar@gmail.com");
+ await page.getByRole("button", { name: "Enter password", exact: true }).click();
+ await page.getByPlaceholder("********").fill("Sriya@1234");
+ await page.getByRole("button", { name: "Log in", exact: true }).last().click();
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Hard-coded credentials in the test are brittle and risky
Using a real email and password here tightly couples the test to that account and password policy, and risks these creds leaking or being reused outside test environments. Please load credentials from environment variables or a test config/fixture for a dedicated test user so they can be rotated without code changes.
Suggested implementation:
```typescript
await expect(page).toHaveURL(/\/account\/login/);
await page.getByRole("button", { name: "Log in", exact: true }).first().click();
const testUserEmail = runtimeEnv?.E2E_TEST_USER_EMAIL;
const testUserPassword = runtimeEnv?.E2E_TEST_USER_PASSWORD;
if (!testUserEmail || !testUserPassword) {
throw new Error(
"Missing E2E test user credentials. Please set E2E_TEST_USER_EMAIL and E2E_TEST_USER_PASSWORD."
);
}
await page.getByPlaceholder("username@email.com").fill(testUserEmail);
await page.getByRole("button", { name: "Enter password", exact: true }).click();
await page.getByPlaceholder("********").fill(testUserPassword);
await page.getByRole("button", { name: "Log in", exact: true }).last().click();
```
1. Ensure your Playwright/CI environment sets `E2E_TEST_USER_EMAIL` and `E2E_TEST_USER_PASSWORD` (e.g. via `.env` or CI secrets).
2. Optionally, centralize these env lookups in a shared test config/util if multiple specs need the same credentials.
</issue_to_address>
### Comment 3
<location> `apps/e2e-playwright/tests/regression/navigation.spec.ts:31-44` </location>
<code_context>
+ await page.getByPlaceholder("********").fill("Sriya@1234");
+ await page.getByRole("button", { name: "Log in", exact: true }).last().click();
+
+ const redirectedToCalendar = await page
+ .waitForURL(/local\.nucleus\.to\/calendar\/?$/, {
+ timeout: 20_000
+ })
+ .then(() => true)
+ .catch(() => false);
+
+ if (!redirectedToCalendar && shouldBypassLoginFailure) {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting explicitly on the failure path when redirect does not occur
Since a failed redirect is just mapped to `false`, we lose visibility into what actually happened. It would be more robust to assert a concrete failure condition (e.g. still on `/account/login` or an error toast is shown) before bypassing, so test failures and frequent bypasses are easier to diagnose.
```suggestion
const redirectedToCalendar = await page
.waitForURL(/local\.nucleus\.to\/calendar\/?$/, {
timeout: 20_000
})
.then(() => true)
.catch(() => false);
// Explicitly assert the failure path when redirect does not occur so that
// bypassed logins remain diagnosable.
if (!redirectedToCalendar) {
await expect(page).toHaveURL(/\/account\/login/);
}
if (!redirectedToCalendar && shouldBypassLoginFailure) {
test.info().annotations.push({
type: "bypass",
description: "Login redirect bypassed: stayed on /account/login"
});
return;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The Playwright test hardcodes a real-looking email, password, and base URL; consider moving these into environment variables or Playwright config (e.g., via fixtures or storageState) to avoid leaking credentials and to make the test portable across environments.
- The login redirect bypass controlled by E2E_BYPASS_LOGIN_FAILURE can silently mask regressions; it may be clearer to either skip the test explicitly under that condition or split strict vs. relaxed behaviour into separate tests/projects so CI signals remain unambiguous.
- The e2e-playwright/package.json file is now minified onto a single line, which makes diffs and manual edits harder; consider keeping the standard multi-line JSON formatting for better readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Playwright test hardcodes a real-looking email, password, and base URL; consider moving these into environment variables or Playwright config (e.g., via fixtures or storageState) to avoid leaking credentials and to make the test portable across environments.
- The login redirect bypass controlled by E2E_BYPASS_LOGIN_FAILURE can silently mask regressions; it may be clearer to either skip the test explicitly under that condition or split strict vs. relaxed behaviour into separate tests/projects so CI signals remain unambiguous.
- The e2e-playwright/package.json file is now minified onto a single line, which makes diffs and manual edits harder; consider keeping the standard multi-line JSON formatting for better readability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
There was a problem hiding this comment.
3 issues found across 11 files (changes from recent commits).
Prompt for AI agents (all 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/scripts/save-google-auth-state.ts">
<violation number="1" location="apps/e2e-playwright/scripts/save-google-auth-state.ts:26">
P1: Rule violated: **Flag Security Vulnerabilities**
Remove the plain HTTP dev origin. The security rule requires HTTPS/TLS for network requests; keeping http://dev.nucleus.to allows authentication over an insecure channel and risks credential/session exposure.</violation>
</file>
<file name="client/components/settings/appearance/AppearanceSettings.svelte">
<violation number="1">
P2: Potential runtime error: appearance is accessed without optional chaining. If $userPreferences.appearance is undefined (as suggested by other optional checks), this line will throw. Use optional chaining or a default appearance object.</violation>
</file>
<file name="client/layout/layers/themeLayer/ThemeLayer.svelte">
<violation number="1">
P2: Guard against undefined `appearance` before accessing `typeface` to avoid runtime errors when preferences are not initialized.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| baseOrigin, | ||
| baseOrigin.startsWith("http:") ? `https://${baseHost}` : `http://${baseHost}`, | ||
| "https://dev.nucleus.to", | ||
| "http://dev.nucleus.to" |
Contributor
There was a problem hiding this comment.
P1: Rule violated: Flag Security Vulnerabilities
Remove the plain HTTP dev origin. The security rule requires HTTPS/TLS for network requests; keeping http://dev.nucleus.to allows authentication over an insecure channel and risks credential/session exposure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/scripts/save-google-auth-state.ts, line 26:
<comment>Remove the plain HTTP dev origin. The security rule requires HTTPS/TLS for network requests; keeping http://dev.nucleus.to allows authentication over an insecure channel and risks credential/session exposure.</comment>
<file context>
@@ -0,0 +1,104 @@
+ baseOrigin,
+ baseOrigin.startsWith("http:") ? `https://${baseHost}` : `http://${baseHost}`,
+ "https://dev.nucleus.to",
+ "http://dev.nucleus.to"
+];
+const allowedOriginsSet = new Set(allowedOrigins);
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts`:
- Around line 97-98: The current assertion uses the local variable pathname and
accepts /signup and /account/login, which wrongly treats unauthenticated states
as success; remove this overly permissive expect (the line using pathname and
the boolean OR check) or tighten it to only allow the intended successful paths
(e.g., "/" or "/calendar" and "/calendar/"), and rely on the subsequent explicit
calendar check to fail the test if authentication blocks progress (the later
/calendar assertion already enforces the real success condition).
🧹 Nitpick comments (7)
apps/e2e-playwright/.env.example (1)
1-1: Add a trailing newline.The file is missing a trailing newline, which is flagged by dotenv-linter and is a POSIX convention that avoids issues with some tools.
apps/e2e-playwright/playwright.config.ts (1)
46-46: Remove the inline comment.The coding guidelines for
*.tsfiles require: "never introduce inline comments when modifying code." Move the explanation to a block comment above or remove it entirely.Proposed fix
- video: "on", // keep video for every run (pass or fail) + video: "on",As per coding guidelines:
**/*.{ts,tsx,svelte,js,jsx}: "never introduce inline comments when modifying code."apps/e2e-playwright/scripts/save-google-auth-state.ts (2)
22-27: Hardcodeddev.nucleus.toorigins leak into all environments.The
allowedOriginslist unconditionally includesdev.nucleus.toregardless ofAPP_BASE_URL. If the user runs this script against a staging or production URL, the redirect detector will still acceptdev.nucleus.toas a valid "back on app" origin, which is misleading and could save auth state prematurely on a wrong host.Consider building
allowedOriginssolely from the configuredbaseURL:Proposed fix
const allowedOrigins = [ baseOrigin, baseOrigin.startsWith("http:") ? `https://${baseHost}` : `http://${baseHost}`, - "https://dev.nucleus.to", - "http://dev.nucleus.to" ];
66-66: Minor:appOriginduplicatesbaseOriginfrom line 19.
appOriginon line 66 computes the same value asbaseOrigindeclared on line 19. ReusebaseOriginto reduce redundancy.Proposed fix
- const appOrigin = new URL(baseURL).origin; + const appOrigin = baseOrigin;apps/e2e-playwright/tests/regression/navigation.spec.ts (3)
89-95: Three near-identical offline-dismiss loops — extract a helper.Lines 89–95, 102–106, and 134–139 all repeat the same pattern: loop N times calling
clickContinueOfflineIfVisible(), break if not handled, optionally wait. This is a DRY violation that makes an already long test harder to maintain.Proposed helper
async function dismissOfflineOverlay( page: Page, fn: () => Promise<boolean>, maxAttempts: number, waitForAppUrl?: { predicate: (url: URL) => boolean; timeout: number } ) { for (let i = 0; i < maxAttempts; i++) { const handled = await fn(); if (!handled) break; if (waitForAppUrl) { await page.waitForURL( (u) => waitForAppUrl.predicate(new URL(u.toString())), { timeout: waitForAppUrl.timeout } ).catch(() => null); } else { await page.waitForLoadState("domcontentloaded").catch(() => null); } } }Also applies to: 102-106, 134-139
14-16: Test name describes implementation, not behavior.
"already logged in (Google auth state): handle old page if present, then verify in app"is an implementation detail. Consider a behavior-focused name like"redirects to /calendar and renders app navigation after login".
18-25: Blocking all Google account requests may mask real auth failures.
route.abort()on anyaccounts.google.comrequest silently swallows OAuth redirects. If the stored auth state expires or is invalid, the test won't see a clear auth error — it'll just see unexpected UI states. Consider logging aborted requests at minimum for debuggability.
Comment on lines
+97
to
+98
| const pathname = new URL(page.url()).pathname; | ||
| expect(pathname === "/" || pathname === "/calendar" || pathname === "/calendar/" || pathname === "/signup" || pathname === "/account/login").toBe(true); |
Contributor
There was a problem hiding this comment.
Assertion is too permissive — it accepts login/signup as success.
This expect passes if the URL is /signup or /account/login, meaning the test "succeeds" even when the app never progressed past auth. If the intent is to verify a logged-in state, this assertion should not include those paths. If reaching login/signup is an acceptable intermediate state, remove this assertion and rely on the later /calendar check (line 128) which correctly throws on failure.
Proposed fix — remove the overly permissive assertion
- const pathname = new URL(page.url()).pathname;
- expect(pathname === "/" || pathname === "/calendar" || pathname === "/calendar/" || pathname === "/signup" || pathname === "/account/login").toBe(true);The hard failure on line 128–132 already covers the case where /calendar is unreachable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const pathname = new URL(page.url()).pathname; | |
| expect(pathname === "/" || pathname === "/calendar" || pathname === "/calendar/" || pathname === "/signup" || pathname === "/account/login").toBe(true); |
🤖 Prompt for AI Agents
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 97 -
98, The current assertion uses the local variable pathname and accepts /signup
and /account/login, which wrongly treats unauthenticated states as success;
remove this overly permissive expect (the line using pathname and the boolean OR
check) or tighten it to only allow the intended successful paths (e.g., "/" or
"/calendar" and "/calendar/"), and rely on the subsequent explicit calendar
check to fail the test if authentication blocks progress (the later /calendar
assertion already enforces the real success condition).
Member
|
@Sriya-Mukkawar, this looks like a good start. Please proceed to writing comprehensive tests. One quick thing to note is that don't hardcode "Calendar" as the default home page. The home page, app menu items, top menu items are all configured per product. i.e. the app menu, home page will all change based on if its memotron or pointron or nucleus. So, Please use the product config in tests as well to determine what menu items and home page should be present based on which product we are testing. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts (1)
4-7: SameglobalThisindirection issue as innavigation.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around lines 4 - 7, The code introduces an unnecessary and fragile typed indirection to access env vars; update the runtimeEnv/baseURL logic to use the standard Node global (e.g. process.env) or a simple any-cast on globalThis. Replace the current runtimeEnv declaration with something like const runtimeEnv = process?.env ?? (globalThis as any).process?.env, and keep baseURL = runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173" so runtimeEnv and baseURL no longer rely on the awkward typed globalThis cast.apps/e2e-playwright/tests/regression/navigation.spec.ts (2)
4-7:globalThisindirection is unnecessary — useprocess.envdirectly.Playwright tests run in Node.js where
process.envis always available. TheglobalThiscast adds complexity for no benefit and defaults bypass totruewhen the env var is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 4 - 7, Replace the unnecessary globalThis indirection: remove the runtimeEnv variable and read process.env.APP_BASE_URL directly when computing baseURL (i.e., set baseURL using process.env.APP_BASE_URL ?? "http://127.0.0.1:4173"). Update the code that references runtimeEnv and ensure only the baseURL constant remains (no cast or optional chaining through globalThis).
98-99: Assertion still accepts/signupand/account/loginas success.This was flagged in a prior review. The assertion on line 99 passes even when the app never progressed past auth, making this check effectively useless. The hard failure on lines 129-133 already covers the real success condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 98 - 99, The current assertion on pathname incorrectly treats "/signup" and "/account/login" as valid success states; update the expect that references pathname and nucleusProductConfig.homePath so it only accepts "/" and `/${nucleusProductConfig.homePath}` (and the trailing-slash variant) by removing "/signup" and "/account/login" from the boolean expression, or simply delete this redundant expect entirely since the strict success check later already covers the real condition.
🧹 Nitpick comments (9)
apps/e2e-playwright/playwright.config.ts (1)
47-47: Inline comment on modified line violates coding guidelines.As per coding guidelines for
**/*.{ts,tsx,svelte,js,jsx}: "never introduce inline comments when modifying code." Move the explanation to a JSDoc or block comment above the property, or remove it.Proposed fix
+ // Keep video for every run (pass or fail) - video: "on", // keep video for every run (pass or fail) + video: "on",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/playwright.config.ts` at line 47, The inline comment on the Playwright config property "video" violates the repo guideline against inline comments; remove the inline comment after the video: "on" property and either move the explanation into a block/JSDoc comment immediately above the config property or delete the comment entirely. Locate the "video" property in the Playwright config (e.g., within the exported config object) and replace the inline comment with a short block comment or JSDoc above that line describing that videos are kept for every run.apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts (3)
1-2: Overall suite structure and execution time concern.With
workers: 1in the Playwright config and 15+ tests each with timeouts of 45–120 seconds, this regression suite could take 15–20 minutes to complete sequentially. TheensureInAppOnHomesetup alone involves multiple navigations and wait cycles per test. Consider:
- Grouping related tests that share setup state (e.g., goal creation + focus + timeline verification) to reduce redundant navigation
- Using
test.describe.configure({ mode: 'serial' })for dependent tests that can share a singleensureInAppOnHomecall viatest.beforeAllAlso applies to: 14-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around lines 1 - 2, The suite is running tests sequentially causing long runtime; refactor related tests in nucleus-command-and-nav.spec.ts to reduce redundant navigations by grouping dependent tests into a test.describe block and call ensureInAppOnHome once in test.beforeAll, and for groups that must run sequentially use test.describe.configure({ mode: 'serial' }); specifically identify tests that share goal creation/focus/timeline verification and move them into a shared describe so they reuse the single setup instead of calling ensureInAppOnHome per test.
149-219: Tests create persistent data (goals, tasks, focus sessions) without cleanup.Every test run creates entities with unique names (
E2E test goal ${Date.now()}), but nothing removes them afterward. Over time, this will pollute the test environment — the Library will accumulate hundreds ofE2E test goal …/E2E task …entries, degrading test reliability (search results become noisy) and the target environment's data. Consider addingtest.afterEachortest.afterAllhooks that clean up created entities, or running against an ephemeral environment that's reset between suites.Also applies to: 221-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around lines 149 - 219, The test creates persistent entities (goalName variable) and never cleans them up; add a cleanup hook (test.afterEach or test.afterAll) that locates and deletes entities created by this spec using the unique name pattern (goalName / `E2E test goal ${Date.now()}`) so the Library isn't polluted; implement the hook to reuse existing helpers used in the test (e.g., runCommand, runQuickFocusCommand or a new helper like deleteGoalByName) to find and remove the created goal and any related tasks/sessions after the test(s) complete, ensuring it runs even on failures.
486-494: Fragile parent-element traversal withlocator("..").Lines 491-493 use
.locator("..").locator("..")to walk up the DOM tree from a button to find a sibling panel row. This breaks if the DOM structure changes even slightly. Prefer a more stable selector — e.g., a data-testid on the panel, or scope by a known container.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around lines 486 - 494, The parent traversal using .locator("..").locator("..") is brittle; replace it by selecting the panel row via a stable selector scoped to the calendarColumn and filtered by the button presence. For example, add/use a stable attribute on the row (e.g. data-testid="panel-row") or a CSS class and then compute panelRow as calendarColumn.locator('[data-testid="panel-row"]').filter({ has: overviewInPanel }) (or .filter({ has: timelineInPanel }) when overview is absent) instead of walking up the DOM from overviewInPanel/timelineInPanel; update the code that uses calendarColumn, overviewInPanel, timelineInPanel and panelRow accordingly.apps/e2e-playwright/tests/regression/navigation.spec.ts (3)
30-30: Use Playwright'sbaseURLfixture instead of manually constructing absolute URLs.Playwright's config already defines
baseURL(line 25 ofplaywright.config.ts). When it's set,page.goto("/")automatically resolves against it. The manualbaseURLconstant on line 7 duplicates this and risks divergence. Use relative paths withpage.goto("/")andpage.goto(/${nucleusProductConfig.homePath})instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` at line 30, Replace the manual baseURL constant and any calls like page.goto(baseURL) with Playwright's built-in baseURL fixture: call page.goto("/") for the root and page.goto(`/${nucleusProductConfig.homePath}`) for the home path, and remove the duplicated baseURL constant to avoid divergence; update the tests in navigation.spec.ts to use page.goto("/") and page.goto(`/${nucleusProductConfig.homePath}`) instead of constructing absolute URLs.
163-177:testNavLabelis hardcoded — consider iterating over product config nav labels.The reviewer asked that the test use the product configuration to determine expected menu items. While
nucleusProductConfig.pathByNavLabelis used for the expected path, the navigation target is hardcoded to"Overview". Consider parameterizing the test to iterate overnucleusProductConfig.appMenuNavLabels(excluding the current home page) to verify all navigation items.Sketch — parameterized navigation check
- // Required action: click a different nav item (Overview) and verify route changed. - const testNavLabel = "Overview"; // Test navigation by clicking Overview - const navAction = page - .getByRole("button", { name: new RegExp(`^${testNavLabel}$`, "i") }) - .or(page.getByRole("link", { name: new RegExp(`^${testNavLabel}$`, "i") })) - .first(); - await expect(navAction).toBeVisible({ timeout: 20_000 }); - await navAction.click({ timeout: 5_000, force: true }); - const expectedPath = nucleusProductConfig.pathByNavLabel[testNavLabel]; - await page.waitForURL((u) => new RegExp(`^${expectedPath}(\\/.*)?$`).test(new URL(u).pathname), { - timeout: 20_000 - }); - - const finalPath = new URL(page.url()).pathname; - expect(finalPath === expectedPath || finalPath.startsWith(`${expectedPath}/`)).toBe(true); + for (const navLabel of nucleusProductConfig.appMenuNavLabels) { + if (navLabel === nucleusProductConfig.timelinePageLabel) continue; + const navAction = page + .getByRole("button", { name: new RegExp(`^${navLabel}$`, "i") }) + .or(page.getByRole("link", { name: new RegExp(`^${navLabel}$`, "i") })) + .first(); + await expect(navAction).toBeVisible({ timeout: 20_000 }); + await navAction.click({ timeout: 5_000, force: true }); + const expectedPath = nucleusProductConfig.pathByNavLabel[navLabel]; + await page.waitForURL( + (u) => new URL(u).pathname.startsWith(expectedPath), + { timeout: 20_000 } + ); + const finalPath = new URL(page.url()).pathname; + expect(finalPath === expectedPath || finalPath.startsWith(`${expectedPath}/`)).toBe(true); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 163 - 177, Replace the hardcoded testNavLabel ("Overview") with a parameterized loop over nucleusProductConfig.appMenuNavLabels (skipping the home/current page) and for each label compute navAction (using the same getByRole/or/first approach), assert visibility, click, then derive expectedPath from nucleusProductConfig.pathByNavLabel[label] and waitForURL/assert finalPath as currently done; update variable names (testNavLabel, navAction, expectedPath, finalPath) inside the loop so the test verifies every configured nav item instead of a single hardcoded one.
39-87: Extract offline-flow logic into a shared test utility, but account for implementation differences.Both specs contain similar offline-button-clicking helpers (
clickContinueOfflineIfVisibleat lines 34–70 innucleus-command-and-nav.spec.tsand lines 39–87 innavigation.spec.ts), but the implementations differ:
navigation.spec.tsincludes extra handling for the "Offline" tab and wraps the final logic in a block scope.nucleus-command-and-nav.spec.tsomits the tab check and uses a simpler return statement.Extract the core offline-flow logic into a shared module (e.g.,
apps/e2e-playwright/helpers/offline-flow.ts) and parameterize to accommodate both patterns, or align them first if the tab check is not needed in one spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/regression/navigation.spec.ts` around lines 39 - 87, Both tests duplicate the clickContinueOfflineIfVisible helper with minor differences (offline tab handling and final block behavior); extract the shared logic into a single helper module (e.g., export function clickContinueOfflineIfVisible(page, { checkOfflineTab = true } = {})) that encapsulates: locating continueOfflineMain/continueOfflineAny, optional offline-tab click when checkOfflineTab is true, the same waitFor/timeout logic, and a consistent return value indicating progress; update each spec to import that helper and call it with checkOfflineTab true or false as needed, preserving the existing timeouts and force/catch semantics used in the original clickContinueOfflineIfVisible function to avoid behavioral regressions.apps/e2e-playwright/config/nucleus-product.config.ts (2)
5-20: Config is Nucleus-only — consider parameterizing for multi-product support.The reviewer (thyaravind) noted that home page, app menu items, and top menu items are product-configurable (memotron, pointron, nucleus). This config is a good first step in extracting hardcoded values, but it only covers Nucleus. Consider defining a shared
ProductE2EConfiginterface and adding sibling configs (e.g.,memotron-product.config.ts) so the test suite can be driven by aVITE_PRODUCT-style env var, selecting the right config at runtime.Also, the
as Record<string, string>onpathByNavLabel(Line 19) erases the literal key types thatas const(Line 20) would otherwise provide. If dynamic key access is intended, this is fine, but it means typos inpathByNavLabel["Calender"]won't be caught at compile time. Based on learnings, agents must confirm the active product context before touching UI or workflow code by inspectingclient/products/*configurations.🤖 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 5 - 20, This file defines a Nucleus-specific config (nucleusProductConfig) but should be generalized: create a shared ProductE2EConfig interface and extract per-product configs (e.g., memotron-product.config.ts, pointron-product.config.ts) following nucleusProductConfig shape, and add runtime selection by reading VITE_PRODUCT (or similar env) to return the active config; also replace the loose "as Record<string,string>" on pathByNavLabel with a typed mapping that preserves literal keys (or explicitly document why dynamic keys are required) so callers like pathByNavLabel["Calendar"] are type-safe and typos are caught at compile time, and ensure runtime test setup checks client/products/* to confirm active product context before interacting with UI.
1-4: Address drift between E2E config and canonical source.The mirrored config at
apps/e2e-playwright/config/nucleus-product.config.tshas already diverged fromclient/products/product.config.ts:
appMenuNavLabelsuses PascalCase (["Calendar", "Overview", "Library"]) while the canonicalappMenuuses lowercase (["calendar", "overview", "library"])includeHomeInNavis hardcoded tofalse, but the canonical config conditionally includes "home" in dev mode viaisDev- Portrait mode variants (
appMenuPt,homePathPt) from the canonical source are not mirroredConsider establishing a single source of truth—either export test fixtures from the canonical config with case transformation, or add a CI-time comparison test to detect future divergence.
🤖 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 1 - 4, The E2E config has drift: adjust apps/e2e-playwright/config/nucleus-product.config.ts so its exported values match the canonical client/products/product.config.ts; specifically change appMenuNavLabels to use lowercase to match appMenu, make includeHomeInNav respect the canonical isDev-based conditional instead of hardcoding false, and add the missing portrait-mode exports (appMenuPt and homePathPt) or import them from the canonical file; as a longer-term option, consider re-exporting or importing the canonical config (with case transformation if needed) or add a CI comparison test to detect future divergence.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/e2e-playwright/config/nucleus-product.config.tsapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/tests/regression/navigation.spec.tsapps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.tsclient/layout/topNav/TopNavLeftMenuItem.svelteclient/products/pointron/focus/session.store.ts
✅ Files skipped from review due to trivial changes (1)
- client/products/pointron/focus/session.store.ts
🤖 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/regression/nucleus-command-and-nav.spec.ts`:
- Around line 30-116: The ensureInAppOnHome function duplicates offline-flow
logic; extract the inner clickContinueOfflineIfVisible logic into a shared
helper clickContinueOfflineIfVisible(page) and move the
retry-and-navigate-to-home pattern into a shared ensureInAppOnHome(page,
baseURL, nucleusProductConfig) in a new test-utilities module; also extract the
Google route interception from beforeEach into blockGoogleAuth(page) and replace
the duplicated implementations in this file and navigation.spec.ts with imports
from that module so both specs call clickContinueOfflineIfVisible and
ensureInAppOnHome instead of containing near-identical code.
- Around line 100-104: Replace the hardcoded navMarkers array with one built
from nucleusProductConfig.appMenuNavLabels: iterate
nucleusProductConfig.appMenuNavLabels and map each label to the corresponding
locator (e.g., page.getByRole("button", { name: new RegExp(`^${label}$`, "i")
}).first()) so markers are derived from config (similar to the approach in
navigation.spec.ts). Update any references to navMarkers to use the new
dynamically generated array.
- Around line 82-92: The waitForURL callback currently hardcodes "/calendar"
which breaks if nucleusProductConfig.homePath changes; compute the expected
pathname once from nucleusProductConfig.homePath (e.g., using new
URL(nucleusProductConfig.homePath, baseURL).pathname) and use that variable in
the callback used by page.waitForURL when comparing p (from new URL(u).pathname)
with equality and startsWith so the check references
nucleusProductConfig.homePath rather than the literal "/calendar".
In `@client/layout/topNav/TopNavLeftMenuItem.svelte`:
- Around line 47-50: The button currently sets aria-label unconditionally which
can override the visible label and uses mismatched nullish/falsy checks; change
logic so aria-label is only added when the visible label prop is absent (i.e.,
icon-only case) and compute its value using a nullish-coalescing chain (tooltip
?? data.label ?? undefined) to avoid empty-string edge cases; update the
hasTooltip/aria-label expressions around the button (referencing hasTooltip,
tooltip, label, data.label, and the span that renders visible text) so when
label is present no aria-label attribute is emitted, and when label is absent
aria-label uses tooltip ?? data.label.
---
Duplicate comments:
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts`:
- Around line 4-7: Replace the unnecessary globalThis indirection: remove the
runtimeEnv variable and read process.env.APP_BASE_URL directly when computing
baseURL (i.e., set baseURL using process.env.APP_BASE_URL ??
"http://127.0.0.1:4173"). Update the code that references runtimeEnv and ensure
only the baseURL constant remains (no cast or optional chaining through
globalThis).
- Around line 98-99: The current assertion on pathname incorrectly treats
"/signup" and "/account/login" as valid success states; update the expect that
references pathname and nucleusProductConfig.homePath so it only accepts "/" and
`/${nucleusProductConfig.homePath}` (and the trailing-slash variant) by removing
"/signup" and "/account/login" from the boolean expression, or simply delete
this redundant expect entirely since the strict success check later already
covers the real condition.
In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts`:
- Around line 4-7: The code introduces an unnecessary and fragile typed
indirection to access env vars; update the runtimeEnv/baseURL logic to use the
standard Node global (e.g. process.env) or a simple any-cast on globalThis.
Replace the current runtimeEnv declaration with something like const runtimeEnv
= process?.env ?? (globalThis as any).process?.env, and keep baseURL =
runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173" so runtimeEnv and baseURL no
longer rely on the awkward typed globalThis cast.
---
Nitpick comments:
In `@apps/e2e-playwright/config/nucleus-product.config.ts`:
- Around line 5-20: This file defines a Nucleus-specific config
(nucleusProductConfig) but should be generalized: create a shared
ProductE2EConfig interface and extract per-product configs (e.g.,
memotron-product.config.ts, pointron-product.config.ts) following
nucleusProductConfig shape, and add runtime selection by reading VITE_PRODUCT
(or similar env) to return the active config; also replace the loose "as
Record<string,string>" on pathByNavLabel with a typed mapping that preserves
literal keys (or explicitly document why dynamic keys are required) so callers
like pathByNavLabel["Calendar"] are type-safe and typos are caught at compile
time, and ensure runtime test setup checks client/products/* to confirm active
product context before interacting with UI.
- Around line 1-4: The E2E config has drift: adjust
apps/e2e-playwright/config/nucleus-product.config.ts so its exported values
match the canonical client/products/product.config.ts; specifically change
appMenuNavLabels to use lowercase to match appMenu, make includeHomeInNav
respect the canonical isDev-based conditional instead of hardcoding false, and
add the missing portrait-mode exports (appMenuPt and homePathPt) or import them
from the canonical file; as a longer-term option, consider re-exporting or
importing the canonical config (with case transformation if needed) or add a CI
comparison test to detect future divergence.
In `@apps/e2e-playwright/playwright.config.ts`:
- Line 47: The inline comment on the Playwright config property "video" violates
the repo guideline against inline comments; remove the inline comment after the
video: "on" property and either move the explanation into a block/JSDoc comment
immediately above the config property or delete the comment entirely. Locate the
"video" property in the Playwright config (e.g., within the exported config
object) and replace the inline comment with a short block comment or JSDoc above
that line describing that videos are kept for every run.
In `@apps/e2e-playwright/tests/regression/navigation.spec.ts`:
- Line 30: Replace the manual baseURL constant and any calls like
page.goto(baseURL) with Playwright's built-in baseURL fixture: call
page.goto("/") for the root and page.goto(`/${nucleusProductConfig.homePath}`)
for the home path, and remove the duplicated baseURL constant to avoid
divergence; update the tests in navigation.spec.ts to use page.goto("/") and
page.goto(`/${nucleusProductConfig.homePath}`) instead of constructing absolute
URLs.
- Around line 163-177: Replace the hardcoded testNavLabel ("Overview") with a
parameterized loop over nucleusProductConfig.appMenuNavLabels (skipping the
home/current page) and for each label compute navAction (using the same
getByRole/or/first approach), assert visibility, click, then derive expectedPath
from nucleusProductConfig.pathByNavLabel[label] and waitForURL/assert finalPath
as currently done; update variable names (testNavLabel, navAction, expectedPath,
finalPath) inside the loop so the test verifies every configured nav item
instead of a single hardcoded one.
- Around line 39-87: Both tests duplicate the clickContinueOfflineIfVisible
helper with minor differences (offline tab handling and final block behavior);
extract the shared logic into a single helper module (e.g., export function
clickContinueOfflineIfVisible(page, { checkOfflineTab = true } = {})) that
encapsulates: locating continueOfflineMain/continueOfflineAny, optional
offline-tab click when checkOfflineTab is true, the same waitFor/timeout logic,
and a consistent return value indicating progress; update each spec to import
that helper and call it with checkOfflineTab true or false as needed, preserving
the existing timeouts and force/catch semantics used in the original
clickContinueOfflineIfVisible function to avoid behavioral regressions.
In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts`:
- Around line 1-2: The suite is running tests sequentially causing long runtime;
refactor related tests in nucleus-command-and-nav.spec.ts to reduce redundant
navigations by grouping dependent tests into a test.describe block and call
ensureInAppOnHome once in test.beforeAll, and for groups that must run
sequentially use test.describe.configure({ mode: 'serial' }); specifically
identify tests that share goal creation/focus/timeline verification and move
them into a shared describe so they reuse the single setup instead of calling
ensureInAppOnHome per test.
- Around line 149-219: The test creates persistent entities (goalName variable)
and never cleans them up; add a cleanup hook (test.afterEach or test.afterAll)
that locates and deletes entities created by this spec using the unique name
pattern (goalName / `E2E test goal ${Date.now()}`) so the Library isn't
polluted; implement the hook to reuse existing helpers used in the test (e.g.,
runCommand, runQuickFocusCommand or a new helper like deleteGoalByName) to find
and remove the created goal and any related tasks/sessions after the test(s)
complete, ensuring it runs even on failures.
- Around line 486-494: The parent traversal using .locator("..").locator("..")
is brittle; replace it by selecting the panel row via a stable selector scoped
to the calendarColumn and filtered by the button presence. For example, add/use
a stable attribute on the row (e.g. data-testid="panel-row") or a CSS class and
then compute panelRow as
calendarColumn.locator('[data-testid="panel-row"]').filter({ has:
overviewInPanel }) (or .filter({ has: timelineInPanel }) when overview is
absent) instead of walking up the DOM from overviewInPanel/timelineInPanel;
update the code that uses calendarColumn, overviewInPanel, timelineInPanel and
panelRow accordingly.
Comment on lines
+30
to
+116
| async function ensureInAppOnHome(page: import("@playwright/test").Page) { | ||
| await page.goto(baseURL); | ||
| await page.waitForLoadState("domcontentloaded"); | ||
|
|
||
| const clickContinueOfflineIfVisible = async () => { | ||
| const continueOfflineMain = page | ||
| .getByRole("button", { name: /Continue (using )?offline/i }) | ||
| .filter({ hasText: /Single device|free forever|No signup/i }) | ||
| .first(); | ||
| const continueOfflineAny = page.getByRole("button", { | ||
| name: /Continue (using )?offline/i | ||
| }).first(); | ||
| const pathname = new URL(page.url()).pathname; | ||
| const waitMs = | ||
| pathname === "/signup" || pathname === "/account/login" ? 10_000 : 3_000; | ||
| let target = continueOfflineMain; | ||
| try { | ||
| await continueOfflineMain.waitFor({ state: "visible", timeout: waitMs }); | ||
| } catch { | ||
| try { | ||
| await continueOfflineAny.waitFor({ state: "visible", timeout: 2_000 }); | ||
| target = continueOfflineAny; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
| const beforePath = new URL(page.url()).pathname; | ||
| await target.click({ timeout: 5_000, force: true }).catch(() => null); | ||
| await page.waitForLoadState("domcontentloaded").catch(() => null); | ||
| const progressed = await page | ||
| .waitForURL( | ||
| (u) => { | ||
| const p = new URL(u).pathname; | ||
| return p !== "/signup" && p !== "/account/login"; | ||
| }, | ||
| { timeout: 8_000 } | ||
| ) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
| return progressed || new URL(page.url()).pathname !== beforePath; | ||
| }; | ||
|
|
||
| for (let i = 0; i < 4; i += 1) { | ||
| const handled = await clickContinueOfflineIfVisible(); | ||
| if (!handled) break; | ||
| } | ||
| for (let i = 0; i < 3; i += 1) { | ||
| const handled = await clickContinueOfflineIfVisible(); | ||
| if (!handled) break; | ||
| await page.waitForLoadState("domcontentloaded").catch(() => null); | ||
| } | ||
|
|
||
| const homeUrl = new URL(nucleusProductConfig.homePath, baseURL).toString(); | ||
| await page.goto(homeUrl, { waitUntil: "domcontentloaded" }); | ||
| await page | ||
| .waitForURL( | ||
| (u) => { | ||
| const p = new URL(u).pathname; | ||
| return p === "/calendar" || p.startsWith("/calendar/"); | ||
| }, | ||
| { timeout: 10_000 } | ||
| ) | ||
| .catch(() => null); | ||
|
|
||
| for (let i = 0; i < 3; i += 1) { | ||
| const handled = await clickContinueOfflineIfVisible(); | ||
| if (!handled) break; | ||
| await page.waitForLoadState("domcontentloaded").catch(() => null); | ||
| } | ||
|
|
||
| const navMarkers = [ | ||
| page.getByRole("button", { name: /^Overview$/i }).first(), | ||
| page.getByRole("button", { name: /^Calendar$/i }).first(), | ||
| page.getByRole("button", { name: "Today" }).first() | ||
| ]; | ||
| await expect | ||
| .poll( | ||
| async () => { | ||
| for (const marker of navMarkers) { | ||
| if (await marker.isVisible().catch(() => false)) return true; | ||
| } | ||
| return false; | ||
| }, | ||
| { timeout: 25_000 } | ||
| ) | ||
| .toBe(true); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ensureInAppOnHome duplicates the offline-flow logic from navigation.spec.ts.
This function contains a near-identical copy of clickContinueOfflineIfVisible plus the retry-and-navigate-to-home pattern. Combined with the Google-route-blocking in beforeEach (lines 15-24), there's substantial code shared between the two spec files. Consider extracting these into a shared test-utilities module:
clickContinueOfflineIfVisible(page)— offline flow helperensureInAppOnHome(page, baseURL, config)— full setupblockGoogleAuth(page)— route interception
This reduces maintenance burden when the signup/offline flow changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around
lines 30 - 116, The ensureInAppOnHome function duplicates offline-flow logic;
extract the inner clickContinueOfflineIfVisible logic into a shared helper
clickContinueOfflineIfVisible(page) and move the retry-and-navigate-to-home
pattern into a shared ensureInAppOnHome(page, baseURL, nucleusProductConfig) in
a new test-utilities module; also extract the Google route interception from
beforeEach into blockGoogleAuth(page) and replace the duplicated implementations
in this file and navigation.spec.ts with imports from that module so both specs
call clickContinueOfflineIfVisible and ensureInAppOnHome instead of containing
near-identical code.
Comment on lines
+82
to
+92
| const homeUrl = new URL(nucleusProductConfig.homePath, baseURL).toString(); | ||
| await page.goto(homeUrl, { waitUntil: "domcontentloaded" }); | ||
| await page | ||
| .waitForURL( | ||
| (u) => { | ||
| const p = new URL(u).pathname; | ||
| return p === "/calendar" || p.startsWith("/calendar/"); | ||
| }, | ||
| { timeout: 10_000 } | ||
| ) | ||
| .catch(() => null); |
Contributor
There was a problem hiding this comment.
Hardcoded /calendar path contradicts the purpose of nucleusProductConfig.
Line 88 checks p === "/calendar" || p.startsWith("/calendar/") directly instead of deriving from nucleusProductConfig.homePath. This will silently break if the home path changes in the config.
Proposed fix
+ const homePathPrefix = `/${nucleusProductConfig.homePath}`;
await page
.waitForURL(
(u) => {
const p = new URL(u).pathname;
- return p === "/calendar" || p.startsWith("/calendar/");
+ return p === homePathPrefix || p.startsWith(`${homePathPrefix}/`);
},
{ timeout: 10_000 }
)
.catch(() => null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around
lines 82 - 92, The waitForURL callback currently hardcodes "/calendar" which
breaks if nucleusProductConfig.homePath changes; compute the expected pathname
once from nucleusProductConfig.homePath (e.g., using new
URL(nucleusProductConfig.homePath, baseURL).pathname) and use that variable in
the callback used by page.waitForURL when comparing p (from new URL(u).pathname)
with equality and startsWith so the check references
nucleusProductConfig.homePath rather than the literal "/calendar".
Comment on lines
+100
to
+104
| const navMarkers = [ | ||
| page.getByRole("button", { name: /^Overview$/i }).first(), | ||
| page.getByRole("button", { name: /^Calendar$/i }).first(), | ||
| page.getByRole("button", { name: "Today" }).first() | ||
| ]; |
Contributor
There was a problem hiding this comment.
Nav markers are hardcoded instead of derived from product config.
navMarkers on lines 100-103 hardcodes "Overview", "Calendar", and "Today" labels. The whole point of nucleusProductConfig.appMenuNavLabels is to avoid this. Use the config array to build markers dynamically, similar to what navigation.spec.ts does on line 146.
Proposed fix
- const navMarkers = [
- page.getByRole("button", { name: /^Overview$/i }).first(),
- page.getByRole("button", { name: /^Calendar$/i }).first(),
- page.getByRole("button", { name: "Today" }).first()
- ];
+ const navMarkers = [
+ ...nucleusProductConfig.appMenuNavLabels.map(label =>
+ page.getByRole("button", { name: new RegExp(`^${label}$`, "i") }).first()
+ ),
+ page.getByRole("button", { name: "Today" }).first()
+ ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/regression/nucleus-command-and-nav.spec.ts` around
lines 100 - 104, Replace the hardcoded navMarkers array with one built from
nucleusProductConfig.appMenuNavLabels: iterate
nucleusProductConfig.appMenuNavLabels and map each label to the corresponding
locator (e.g., page.getByRole("button", { name: new RegExp(`^${label}$`, "i")
}).first()) so markers are derived from config (similar to the approach in
navigation.spec.ts). Update any references to navMarkers to use the new
dynamically generated array.
Comment on lines
47
to
+50
| {@const hasTooltip = data.label || tooltip || label} | ||
| {@const shortcut = keyboardShortcuts.resolveShortcutForAction(action)} | ||
| <button | ||
| aria-label={hasTooltip ? (tooltip ?? label ?? data.label) : undefined} |
Contributor
There was a problem hiding this comment.
aria-label can conflict with visible label text (WCAG 2.5.3 risk) and has a ||/?? mismatch edge case.
Two issues with the current expression:
-
WCAG 2.5.3 (Label in Name, Level A): When
labelis provided it renders as visible text inside<span>(line 96), giving the button a native accessible name. Settingaria-labeltotooltip(when both props are present and differ) completely overrides that accessible name — screen readers announce the tooltip string, not what users see.aria-labelis only needed for the icon-only case (labelis absent). -
||vs??mismatch:hasTooltipuses||(falsy guard), but thearia-labelvalue chain uses??(nullish guard). Iftooltipis""(empty string) whiledata.labelis non-empty,hasTooltipis truthy (skips the empty string), yettooltip ?? …resolves to"", producingaria-label=""— an empty accessible name.
♿ Proposed fix
- {`@const` hasTooltip = data.label || tooltip || label}
+ {`@const` hasTooltip = data.label || tooltip || label}
...
- aria-label={hasTooltip ? (tooltip ?? label ?? data.label) : undefined}
+ aria-label={!label ? (tooltip ?? data.label) ?? undefined : undefined}When label is absent (icon-only), expose tooltip or data.label as the accessible name. When label is present, omit aria-label entirely — the visible text already provides the accessible name, and the tooltip semantics are covered by the use:popover action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/layout/topNav/TopNavLeftMenuItem.svelte` around lines 47 - 50, The
button currently sets aria-label unconditionally which can override the visible
label and uses mismatched nullish/falsy checks; change logic so aria-label is
only added when the visible label prop is absent (i.e., icon-only case) and
compute its value using a nullish-coalescing chain (tooltip ?? data.label ??
undefined) to avoid empty-string edge cases; update the hasTooltip/aria-label
expressions around the button (referencing hasTooltip, tooltip, label,
data.label, and the span that renders visible text) so when label is present no
aria-label attribute is emitted, and when label is absent aria-label uses
tooltip ?? data.label.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (22)
apps/e2e-playwright/tests/smoke/home.spec.ts (2)
14-15: Prefer deterministic waits over hard timeouts.
waitForTimeout(2000)is prone to flakiness—tests may fail on slower environments or waste time on faster ones. Consider waiting for a specific element or network idle state instead.♻️ Suggested improvement
await page.waitForLoadState("domcontentloaded"); - await page.waitForTimeout(2000); + await page.waitForLoadState("networkidle");Or wait for a specific app element that indicates readiness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/smoke/home.spec.ts` around lines 14 - 15, Replace the hard 2s sleep (page.waitForTimeout) with a deterministic readiness check: remove page.waitForTimeout(2000) and instead wait for a specific app-ready condition such as page.waitForLoadState("networkidle") or page.waitForSelector(...) for a stable DOM element that indicates the home screen is ready (use an existing selector like a test-id or a unique heading/button). Update the test in apps/e2e-playwright/tests/smoke/home.spec.ts to use page.waitForSelector or page.waitForLoadState("networkidle") in place of page.waitForTimeout to make the test robust and non-flaky.
18-18: URL assertion is overly permissive.The regex
/\//matches virtually any URL. Consider asserting on a more meaningful pattern (e.g., verifying the app's base path is present) without hardcoding a specific page, in line with reviewer feedback about product-configurable home pages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/smoke/home.spec.ts` at line 18, The URL assertion using expect(page).toHaveURL(/\//) is too permissive; update the test to assert the app's configurable base path instead of a bare slash. Replace the regex with one that checks for the configured base path (e.g., using a runtime value like appConfig.basePath or a TEST_BASE_PATH env var) so expect(page).toHaveURL(...) verifies the base path is present while remaining agnostic to which specific product page is shown.client/components/shortcuts/settings/ShortcutItem.svelte (1)
187-195: Consider adding atestIdprop to the Button component instead of wrapping.The wrapper
<span>adds an extra DOM element solely for the test ID. If feasible, adding atestIdprop to the sharedButtoncomponent (similar toTextInputin this PR) would provide a cleaner solution and benefit other test scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/shortcuts/settings/ShortcutItem.svelte` around lines 187 - 195, The span wrapper around the accept Button exists only to attach a data-testid; instead add a testId prop to the shared Button component and pass it directly (e.g., replace <span data-testid="shortcut-accept"><Button ... /></span> with <Button testId="shortcut-accept" ... />). Update the Button component to accept a testId (string | undefined) and render it as a data-testid attribute on the root element, following the TextInput pattern; then remove the extra span in ShortcutItem.svelte and update any other Button usages that rely on wrapping for tests to use the new testId prop.apps/e2e-playwright/playwright.config.ts (2)
31-38: Clarify the relationship betweensharedUseand top-levelusesettings.The
sharedUseobject (lines 31-38) and the top-leveluseblock (lines 70-75) define overlapping settings with different values:
Setting sharedUseusevideo "on""retain-on-failure"trace "on-first-retry""on-first-retry"screenshot "only-on-failure""only-on-failure"Since each project explicitly uses
{ ...sharedUse, baseURL }, the top-levelusesettings forvideo,trace, andscreenshotbecome fallback defaults that likely won't apply. Consider removing duplicates from one location to avoid confusion about which values are in effect.Also applies to: 70-75
🤖 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 31 - 38, sharedUse currently defines video/trace/screenshot and the top-level use block defines the same settings with different values, causing confusion; remove the duplicate video, trace, and screenshot keys from the top-level use block so sharedUse remains the authoritative source (since projects spread with `{ ...sharedUse, baseURL }`), update any comments to note that sharedUse controls these media settings, and ensure only one place (sharedUse) sets video/trace/screenshot to avoid contradictory defaults.
26-28: Consider logging a warning instead of silently ignoring parse errors.The empty catch block silently swallows errors when parsing the auth state file. If the file exists but is malformed, tests may run with unexpected configuration without any indication.
💡 Proposed fix
} catch { - // ignore + console.warn("Failed to parse auth state file, using default baseURL"); }🤖 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 26 - 28, The empty catch in the auth-state parsing code should log a warning instead of swallowing errors; update the catch block around the JSON.parse of the auth state file (the code that reads authStatePath / authStateFile and calls JSON.parse) to call console.warn or the project logger (e.g., processLogger.warn) with a clear message and include the caught error and the auth state path so malformed files are visible, while still falling back to the default behavior.client/elements/input/TextInput.svelte (1)
110-133: InlineMarkdownTextInput does not currently support atestIdprop.The observation about the inconsistency is valid: when
isExperimentalMdInputis true,testIdis applied only to the wrapper<div>(line 111) rather than to theInlineMarkdownTextInputcomponent itself, unlike the other input variants wheretestIdis applied directly to the<input>elements. However,InlineMarkdownTextInputdoes not have atestIdexport, so propagating it would require modifying that component. If test specificity for the actual input element becomes important, consider either addingtestIdsupport toInlineMarkdownTextInputor documenting that tests should target the wrapper element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/elements/input/TextInput.svelte` around lines 110 - 133, InlineMarkdownTextInput doesn't accept a testId prop, so tests can't target the inner input when isExperimentalMdInput is true; add an exported prop (e.g., export let testId) to the InlineMarkdownTextInput component and forward that prop to the actual underlying input element (or root interactive element) inside InlineMarkdownTextInput, then in TextInput.svelte pass testId into <InlineMarkdownTextInput {testId} .../> (you may keep the wrapper div's data-testid for backwards compatibility) so tests can target the inner input element consistently with other input variants.apps/e2e-playwright/tests/utils/helpers.ts (2)
123-126: AvoidwaitForTimeoutfor test reliability.Using fixed
waitForTimeoutdelays (lines 123, 125) can cause flakiness. Prefer waiting for a specific condition, such as the command list appearing or an element becoming actionable.♻️ Suggested improvement
await cmdInput.fill("Quick focus"); - await page.waitForTimeout(500); + await page.getByRole("listbox").or(page.getByRole("option")).first().waitFor({ state: "visible", timeout: 2000 }).catch(() => null); await page.keyboard.press("ArrowDown"); - await page.waitForTimeout(100); await page.keyboard.press("Enter");Alternatively, wait for the specific command option to appear before pressing ArrowDown.
🤖 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 123 - 126, Replace the fixed delays around the keyboard interactions by waiting for the UI elements to be ready: instead of await page.waitForTimeout(500) and await page.waitForTimeout(100) before page.keyboard.press("ArrowDown") and "Enter", wait for the command list or the specific command option to appear/be actionable (e.g., use page.waitForSelector or locator.waitFor with the command list selector or the command option selector and ensure it is visible/enabled) and only then invoke page.keyboard.press("ArrowDown") and page.keyboard.press("Enter"); update the helper in helpers.ts where the waits appear so tests become deterministic and avoid flakiness.
4-7: UnusedbaseURLvariable.
baseURLis defined but never used in this file. Consider removing it to avoid dead code.🧹 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";If
runtimeEnvis not needed elsewhere in this file, remove both lines.🤖 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 4 - 7, Remove the dead variable by deleting the unused baseURL declaration; if runtimeEnv is also unused elsewhere in helpers.ts then remove both the runtimeEnv and baseURL lines to avoid dead code, otherwise keep runtimeEnv and only remove the baseURL line (symbols to look for: baseURL and runtimeEnv).apps/e2e-playwright/tests/core/memory/node/creation.spec.ts (1)
1-31: LGTM - Placeholder scaffold is consistent.The file follows the established pattern for E2E test scaffolding (SKIP_E2E guard, Google accounts blocking, skipped tests with TODOs). No issues with the structure.
The tests are marked as skipped with TODO placeholders. Would you like me to help draft the implementation for the node creation flows, or open an issue to track this work?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/memory/node/creation.spec.ts` around lines 1 - 31, The tests in apps/e2e-playwright/tests/core/memory/node/creation.spec.ts are placeholders marked with test.skip and TODOs; to fix, implement the two skipped tests ("create node via command bar" and "create node via UI") by removing test.skip, using ensureInAppOnHome(page) to reach the app state, then script the user interactions to create a node via the command bar and via the UI, asserting the expected post-creation state; alternatively, if you cannot implement now, open a tracked issue referencing this spec so the SKIP_E2E guard (runtimeEnv?.SKIP_E2E) and the Google auth route filter remain intact and future work can implement test logic around ensureInAppOnHome, the route interception block, and the two test names.apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts (2)
380-384: Fragile parent locator pattern using".."– consider a more robust selector.Using
.locator("..")to navigate to a parent element is brittle and will break if the DOM structure changes. Consider adding adata-testidto the shortcut row container or using a more specific selector.♻️ Suggested approach
Add a test ID to each shortcut row in the application:
<div data-testid="shortcut-row-command-bar"> <span>Command bar</span> <button>...</button> <input placeholder="record shortcut" /> </div>Then:
- const commandBarRow = panel - .getByText("Command bar", { exact: true }) - .locator(".."); + const commandBarRow = panel.getByTestId("shortcut-row-command-bar");Also applies to: 410-413
🤖 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 380 - 384, Replace the fragile parent traversal that uses panel.getByText("Command bar", { exact: true }).locator("..") by selecting the shortcut row with a stable selector: update the app to add a data-testid (e.g., data-testid="shortcut-row-command-bar") on the container and change the test to locate that element (instead of using locator("..")), then get the first button via that container (the variables to update are commandBarRow and shortcutBtn in this spec); apply the same change for the other occurrences around lines 410-413 so tests target data-testid-backed selectors rather than relying on DOM parent navigation.
27-36: DuplicatedopenSettingshelper across describe blocks.The
openSettingsfunction is defined identically in both test.describe blocks (lines 27-36 and 135-144). Extract it to file scope or to the shared helpers module.♻️ Move to file scope
+async function openSettings(page: import("@playwright/test").Page) { + const profileBtn = page.getByTestId("topnav-account-settings"); + const visible = await profileBtn.isVisible().catch(() => false); + if (visible) { + await profileBtn.click({ timeout: 5_000 }); + } else { + await runCommand(page, "Settings"); + } + await page.waitForTimeout(500); +} + test.describe("settings – open, close, navigate (no seed data) `@regression`", () => { test.beforeEach(async ({ page }) => { // ... }); - async function openSettings(page: import("@playwright/test").Page) { - // ... - }Also applies to: 135-144
🤖 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 27 - 36, Two identical openSettings helper functions are duplicated; remove the duplicate and move a single openSettings definition to file scope (above the describe blocks) or into the shared helpers module, then import or reference it from both describe blocks. Locate the function named openSettings (it uses runCommand and the test id "topnav-account-settings" via profileBtn) and consolidate that implementation into one top-level helper, delete the other copy, and update both describe blocks to call the single shared openSettings.apps/e2e-playwright/tests/core/memory/capture.spec.ts (1)
34-52: Consider extracting duplicated editor interaction logic.The editor selector construction (lines 34-44 and 78-88) and markdown toggle fallback (lines 45-52 and 89-96) are nearly identical. Extracting this into a helper would reduce duplication and improve maintainability.
♻️ Suggested helper extraction
async function getCaptureEditor(page: Page) { 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 editorVisible = await editor.isVisible().catch(() => false); if (!editorVisible) { const markdownBtn = page.getByRole("button", { name: /^Markdown$/i }).first(); await markdownBtn.click({ timeout: 5_000 }); await page.waitForTimeout(800); } await editor.waitFor({ state: "visible", timeout: 8_000 }); return editor; }Also applies to: 78-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/memory/capture.spec.ts` around lines 34 - 52, Extract the duplicated editor-selection and markdown-toggle logic into a helper function named getCaptureEditor(page) that builds the combined selector (using getByTestId("capture-editor").getByPlaceholder("Start typing to capture...").or(...).first()), checks visibility via editor.isVisible().catch(() => false), clicks the markdown toggle button (getByRole("button", { name: /^Markdown$/i }).first()) and waits if needed, then awaits editor.waitFor({ state: "visible", timeout: 8_000 }) and returns the editor; replace both duplicated blocks with calls to getCaptureEditor(page) to centralize behavior and remove repetition (references to editor and markdownBtn in the existing diff should be replaced by this helper).apps/e2e-playwright/tests/nucleus/overview/overview.spec.ts (1)
34-41: Consider extracting repeated Overview navigation pattern.The URL wait pattern with regex is repeated 7 times across tests. Extracting this into a helper would reduce duplication:
♻️ Suggested helper
async function navigateToOverview(page: Page) { await ensureInAppOnHome(page); await runCommand(page, "Overview"); await page.waitForURL( (u) => new RegExp(`^${nucleusProductConfig.pathByNavLabel.Overview}(\\/.*)?$`).test( new URL(u).pathname ), { timeout: 10_000 } ); await page.waitForTimeout(1_500); }Then each test becomes:
test("...", async ({ page }) => { test.setTimeout(45_000); await navigateToOverview(page); // ... assertions });Also applies to: 75-82, 115-122, 155-162, 185-192, 212-219, 254-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/overview/overview.spec.ts` around lines 34 - 41, Extract the repeated Overview navigation block into a helper function (e.g., navigateToOverview) that calls ensureInAppOnHome(page), runCommand(page, "Overview"), then waits for the URL using the existing RegExp against nucleusProductConfig.pathByNavLabel.Overview with the 10_000ms timeout and the 1_500ms waitForTimeout; replace the seven inline occurrences (the blocks around lines that perform page.waitForURL with the RegExp and page.waitForTimeout) with calls to navigateToOverview(page) in each test to remove duplication.apps/e2e-playwright/tests/core/focus/task/creation.spec.ts (1)
82-92: Fragile selector for plus icon – consider using a test ID.Using
.getByRole("button").last()on a filtered container is brittle and may break if the toolbar layout changes. A dedicateddata-testidon the "add task" button would be more reliable.♻️ Suggested approach
Add a test ID to the plus icon in the application code:
<button data-testid="task-list-add-button">+</button>Then use it directly:
- const taskListToolbar = page - .locator("div") - .filter({ - has: page.getByRole("button", { name: /By month/i }) - }) - .filter({ - hasNot: page.getByText("No tasks found") - }) - .first(); - const plusIconInHeader = taskListToolbar.getByRole("button").last(); - await plusIconInHeader.click({ timeout: 8_000 }); + await page.getByTestId("task-list-add-button").click({ timeout: 8_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/focus/task/creation.spec.ts` around lines 82 - 92, The selector for the add-task button is fragile (using taskListToolbar.getByRole("button").last()); add a stable data-testid attribute to the "add task" button in the UI (e.g., data-testid="task-list-add-button") and update the test to query that test id (use page.getByTestId or an equivalent locator scoped to taskListToolbar) instead of relying on .last() so the test targets the specific add button reliably; update references in the test to use the new test id and remove the .last() use of plusIconInHeader.apps/e2e-playwright/tests/shared/navigation.spec.ts (1)
15-195: Test is quite long – consider splitting into focused tests.This single test (~180 lines) covers multiple concerns: offline handling, navigation to home, verifying nav markers, and clicking Overview. Breaking it into smaller, focused tests would improve maintainability and make failures easier to diagnose.
🤖 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 15 - 195, The test "already logged in (Google auth state): handle old page if present, then verify in app `@smoke`" is too long and exercises multiple concerns; split it into focused tests: extract the clickContinueOfflineIfVisible helper into a shared helper function and call it from smaller tests, create one test that verifies offline recovery loops (use clickContinueOfflineIfVisible and the for-loops that retry reaching homePageUrl), another test that asserts presence of nav markers (use allMarkers/navMarkers/homeNav/todayButton and the expect.poll block), and a third that performs the navigation action to Overview (use navAction, testNavLabel, expectedPath and the final path assertions); ensure shared setup (page.route, goto baseURL, waitForLoadState) is moved to a beforeEach or test fixture so each new test remains focused and reuses the helper.apps/e2e-playwright/tests/nucleus/app-nav.spec.ts (2)
56-56: Prefer explicit waits overwaitForTimeout.Multiple hard waits (lines 56, 62, 76, 82, 140, 157, 174, 186, 212, 242, 248, 271) make tests flaky. Consider replacing with
waitForon specific element states or using Playwright's auto-waiting assertions where possible.🤖 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 56, Replace the hard-coded sleep calls (e.g., await page.waitForTimeout(1_000)) with explicit, deterministic waits: locate the element or state you expect after the delay (using the same page or Locator instances used in the test) and wait on that condition with page.waitForSelector, locator.waitFor, or Playwright assertions like expect(locator).toBeVisible()/toHaveText()/toBeEnabled(); update every occurrence noted (the waitForTimeout calls at lines referenced in the comment) so each pause is replaced by a wait for the specific element/state transition relevant to that test step, ensuring tests rely on element readiness rather than fixed timeouts.
8-8: Unused variablebaseURL.
baseURLis declared but only used in one fallback scenario (line 146). Consider removing if the Playwright config already provides the base URL, or document why the fallback is necessary.🤖 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 8, The variable baseURL declared as const baseURL = runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173" is unused except for a single fallback use later; remove this declaration and rely on Playwright's config-provided baseURL or, if a local fallback is required, inline the fallback where it’s actually used (replace the one-off reference with runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173") so there is no unused binding; update any comments to document why a fallback is needed if you keep it.apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts (1)
18-18: Avoid hard waits; prefer explicit conditions.
page.waitForTimeout(1_000)introduces flakiness. Consider usingwaitForon an expected element state orexpect.pollfor more reliable synchronization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts` at line 18, Replace the hard sleep call await page.waitForTimeout(1_000) in the test with an explicit wait for the expected condition: use Playwright's page.waitForSelector or locator-based assertions (e.g., await page.waitForSelector('<your-selector>') or await expect(page.locator('<your-selector>')).toBeVisible()) or expect.poll for polling, targeting the element/state the test needs before proceeding; locate the call to page.waitForTimeout in calendar.spec.ts and swap it for the appropriate explicit wait that matches the intended element or state.apps/e2e-playwright/tests/core/focus/goal/bulk-editor.spec.ts (1)
22-25: Placeholder test noted.The skipped test with TODO is acceptable scaffolding. Consider tracking incomplete E2E tests in an issue to ensure follow-up implementation.
Would you like me to open an issue to track the implementation of this bulk editor test?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/focus/goal/bulk-editor.spec.ts` around lines 22 - 25, The test marked with test.skip ("bulk edit goals (select multiple, apply action)") is a placeholder; either implement it or create a tracking issue. To fix: replace test.skip with test and implement the body starting with ensureInAppOnHome(page), then script selecting multiple goals in the library, opening the bulk editor UI, applying a concrete action (e.g., change status or delete), and assert the expected outcome; if you cannot implement now, create an issue referencing this test name and the need to "select multiple goals, open bulk editor, apply action and assert" so it is tracked for follow-up. Ensure you remove the TODO comment when implementing or add the issue ID in a comment if leaving skipped.apps/e2e-playwright/tests/core/focus/goal/browse.spec.ts (1)
45-45: Prefer deterministic waits over arbitrarywaitForTimeout.Multiple
waitForTimeoutcalls throughout this file (lines 45, 72, 79, 87, 116, 139, 146, 149, 159, 166, 169) add flakiness and slow down test execution. Replace with explicitwaitForconditions on elements or network idle states where possible.♻️ Example refactor
- await page.waitForTimeout(1_500); - await expect(page.getByText(goalName, { exact: true }).first()).toBeVisible({ timeout: 15_000 }); + await expect(page.getByText(goalName, { exact: true }).first()).toBeVisible({ timeout: 15_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/focus/goal/browse.spec.ts` at line 45, Replace arbitrary page.waitForTimeout calls with deterministic waits: find each use of page.waitForTimeout (currently 1_500 in this test) and replace it with an explicit condition appropriate to the next action — typically page.waitForSelector('<selector>') for an element you interact with immediately after, page.waitForResponse/route for a network call, or page.waitForLoadState('networkidle') if waiting for page/network quiescence. Update each occurrence of page.waitForTimeout in this test file to wait for the specific DOM selector or network response used by the subsequent assertions/interactions so the test becomes deterministic and less flaky.apps/e2e-playwright/tests/core/focus/goal/creation.spec.ts (1)
54-54: Same pattern: prefer deterministic waits overwaitForTimeout.This file has numerous
waitForTimeoutcalls (lines 54, 67, 79, 98, 105, 138, 162, 169, 178, 196, 205, 211, 217, 236, 243). As noted forbrowse.spec.ts, replace with explicit element or state waits where possible to reduce flakiness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/focus/goal/creation.spec.ts` at line 54, The tests in creation.spec.ts use many non-deterministic page.waitForTimeout calls (e.g., the call to page.waitForTimeout(800) and the other occurrences) which cause flakiness; replace each waitForTimeout with a deterministic wait that targets the actual condition: use page.waitForSelector or locator.waitFor (for elements appearing/visible), expect(locator).toBeVisible()/toHaveText() (for assertions), page.waitForResponse or page.waitForRequest (for network-driven transitions), or page.waitForURL (for navigation) depending on the step; locate each occurrence by searching for page.waitForTimeout in creation.spec.ts and update the surrounding test step (e.g., the action that triggers an element to appear or a navigation) to wait on that specific element/state instead of a fixed timeout so the test becomes deterministic.apps/e2e-playwright/tests/core/focus/goal/context-menu.spec.ts (1)
22-34: Address placeholder tests: implement or track as separate issue.Both tests are skipped placeholders with TODO comments. Either implement these tests as part of this PR, or create a tracking issue to ensure they don't remain forgotten.
Would you like me to help generate the test implementations for these context menu action tests, or open a new issue to track this work?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e-playwright/tests/core/focus/goal/context-menu.spec.ts` around lines 22 - 34, Two placeholder tests are left skipped: the test.skip blocks for "context menu actions from library goal row" and "context menu actions from goal record page" in the test.describe sections; either implement them or file a tracking issue and reference it in the TODO. To fix, remove test.skip and implement steps in the test bodies: use ensureInAppOnHome(page) then navigate to Library → Goals for the library test, locate a goal row (use the row selector or helper), open the row context menu, assert presence of each menu item and invoke each action and assert expected outcome; for the record-page test open a goal record page (via its link or helper), open its context menu, assert and run each action similarly; alternatively, if not implementing now, create a new issue and replace the TODO comments with a single-line TODO referencing that issue ID and keep test.skip until implementation. Reference the test names and helper ensureInAppOnHome to locate the code.
🤖 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/core/focus/goal/browse.spec.ts`:
- Line 38: The test is swallowing assertion failures by appending .catch(() =>
null) to the Playwright assertion; remove the .catch handler so the assertion
can fail the test as intended—replace the line using
expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null) with a
plain await expect(goalNameInput).toBeHidden({ timeout: 10_000 }); (or, if you
need a clearer failure message, await expect(goalNameInput).toBeHidden({
timeout: 10_000 }) inside a try/rethrow that adds context) to let the assertion
throw on failure.
- Line 64: The test is swallowing assertion failures by appending .catch(() =>
null) to the expect call for goalNameInput.toBeHidden; remove the silent catch
so failures surface (i.e., use await expect(goalNameInput).toBeHidden({ timeout:
10_000 }) without .catch), or if you need custom handling wrap the await
expect(...) in a try/catch that rethrows or fails with a clear message
referencing goalNameInput and the toBeHidden assertion.
- Around line 42-44: Replace hardcoded navigation labels in the test (e.g., the
page.getByRole calls that match "Library" and "Goals") to use the product config
so labels vary by product: import nucleusProductConfig (or the correct product
config) into the test and use nucleusProductConfig.appMenuNavLabels[index] (or a
named field if available) inside a case-insensitive RegExp for the name matcher
instead of literal strings; update all occurrences noted (the calls at the shown
snippet and the other places mentioned: lines 81, 107, 113, 141, 151, 153) and
if any label is missing add it to nucleus-product.config.ts so the tests
reference config-driven labels consistently.
In `@apps/e2e-playwright/tests/core/focus/goal/creation.spec.ts`:
- Line 45: The test is swallowing assertion failures by appending .catch(() =>
null) to the Playwright assertion; remove the .catch(() => null) so the
assertion on goalNameInput uses await expect(goalNameInput).toBeHidden({
timeout: 10_000 }) directly (no catch) to allow failures to propagate and fail
the test; update any similar occurrences (e.g., other tests like browse.spec.ts)
that wrap expect(...).toBeHidden or other expect(...) calls with .catch(() =>
null).
- Around line 128-137: The tests inconsistently hardcode navigation labels
("Library", "Goals", "Focus", "Today") instead of using nucleusProductConfig;
extend nucleusProductConfig to include libraryLabel, goalsLabel, focusLabel,
todayLabel (in addition to existing timelinePageLabel and homePath) and update
the test selectors (the calls using getByRole(..., { name: /^Library$/i }),
getByRole(..., { name: /^Goals(\s+\d+)?$/i }) and any similar occurrences) to
reference nucleusProductConfig.libraryLabel, .goalsLabel, .focusLabel,
.todayLabel (use the config values in the regex patterns if needed) so all
navigation labels come from the single product config.
In `@apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts`:
- Line 92: The test is swallowing assertion failures by appending .catch(() =>
null) to the Playwright assertion on goalNameInput; remove the .catch and let
await expect(goalNameInput).toBeHidden({ timeout: 10_000 }) throw on failure (or
replace with an explicit try/catch that rethrows a descriptive Error) so failing
expectations are not suppressed.
- Line 59: The test currently swallows assertion failures by appending
`.catch(() => null)` to `await expect(goalNameInput).toBeHidden({ timeout:
10_000 })`; either remove the entire assertion if the element may validly remain
visible, or remove the `.catch(() => null)` so the assertion can fail and
surface issues. Update the line referencing `goalNameInput`/`toBeHidden`
accordingly (or replace with an explicit conditional check like `await
goalNameInput.isHidden()` and assert its result) so failures are not silently
ignored.
In `@apps/e2e-playwright/tests/shared/navigation.spec.ts`:
- Around line 37-87: Remove the duplicated clickContinueOfflineIfVisible
implementation from navigation.spec.ts and import the shared helper from
helpers.ts instead (use the existing exported clickContinueOfflineIfVisible or
export a small variant from helpers.ts if this test needs a slightly different
behavior); update the test to call the imported function rather than the inline
implementation and ensure any test-specific timeout/behavior is passed via
parameters or a new exported helper in helpers.ts.
In `@apps/e2e-playwright/tests/utils/helpers.ts`:
- Around line 83-98: navMarkers currently hardcodes button labels ("Overview",
"Calendar", "Today") which violates the reviewer request to use
product-configurable labels; update the logic that builds navMarkers in
helpers.ts to derive labels from nucleusProductConfig.appMenuNavLabels (falling
back to "Today" only if that label is universally expected), i.e., iterate
nucleusProductConfig.appMenuNavLabels to create the page.getByRole(...)
selectors instead of hardcoded strings so tests follow the product config;
ensure navMarkers variable name and the expect.poll usage remain unchanged so
only the label source is swapped.
- Around line 66-75: The waitForURL logic hardcodes "/calendar"; update it to
derive the expected pathname from nucleusProductConfig.homePath instead: compute
the pathname from nucleusProductConfig.homePath (e.g., new
URL(nucleusProductConfig.homePath).pathname) and use that value in the
waitForURL predicate so the check uses the configured home path rather than
"/calendar"; update the predicate inside the waitForURL call that currently
references "/calendar" to compare against the computed pathname (and allow
subpaths with startsWith as before) while keeping the same timeout and error
handling around page.goto and waitForURL.
---
Nitpick comments:
In `@apps/e2e-playwright/playwright.config.ts`:
- Around line 31-38: sharedUse currently defines video/trace/screenshot and the
top-level use block defines the same settings with different values, causing
confusion; remove the duplicate video, trace, and screenshot keys from the
top-level use block so sharedUse remains the authoritative source (since
projects spread with `{ ...sharedUse, baseURL }`), update any comments to note
that sharedUse controls these media settings, and ensure only one place
(sharedUse) sets video/trace/screenshot to avoid contradictory defaults.
- Around line 26-28: The empty catch in the auth-state parsing code should log a
warning instead of swallowing errors; update the catch block around the
JSON.parse of the auth state file (the code that reads authStatePath /
authStateFile and calls JSON.parse) to call console.warn or the project logger
(e.g., processLogger.warn) with a clear message and include the caught error and
the auth state path so malformed files are visible, while still falling back to
the default behavior.
In `@apps/e2e-playwright/tests/core/focus/goal/browse.spec.ts`:
- Line 45: Replace arbitrary page.waitForTimeout calls with deterministic waits:
find each use of page.waitForTimeout (currently 1_500 in this test) and replace
it with an explicit condition appropriate to the next action — typically
page.waitForSelector('<selector>') for an element you interact with immediately
after, page.waitForResponse/route for a network call, or
page.waitForLoadState('networkidle') if waiting for page/network quiescence.
Update each occurrence of page.waitForTimeout in this test file to wait for the
specific DOM selector or network response used by the subsequent
assertions/interactions so the test becomes deterministic and less flaky.
In `@apps/e2e-playwright/tests/core/focus/goal/bulk-editor.spec.ts`:
- Around line 22-25: The test marked with test.skip ("bulk edit goals (select
multiple, apply action)") is a placeholder; either implement it or create a
tracking issue. To fix: replace test.skip with test and implement the body
starting with ensureInAppOnHome(page), then script selecting multiple goals in
the library, opening the bulk editor UI, applying a concrete action (e.g.,
change status or delete), and assert the expected outcome; if you cannot
implement now, create an issue referencing this test name and the need to
"select multiple goals, open bulk editor, apply action and assert" so it is
tracked for follow-up. Ensure you remove the TODO comment when implementing or
add the issue ID in a comment if leaving skipped.
In `@apps/e2e-playwright/tests/core/focus/goal/context-menu.spec.ts`:
- Around line 22-34: Two placeholder tests are left skipped: the test.skip
blocks for "context menu actions from library goal row" and "context menu
actions from goal record page" in the test.describe sections; either implement
them or file a tracking issue and reference it in the TODO. To fix, remove
test.skip and implement steps in the test bodies: use ensureInAppOnHome(page)
then navigate to Library → Goals for the library test, locate a goal row (use
the row selector or helper), open the row context menu, assert presence of each
menu item and invoke each action and assert expected outcome; for the
record-page test open a goal record page (via its link or helper), open its
context menu, assert and run each action similarly; alternatively, if not
implementing now, create a new issue and replace the TODO comments with a
single-line TODO referencing that issue ID and keep test.skip until
implementation. Reference the test names and helper ensureInAppOnHome to locate
the code.
In `@apps/e2e-playwright/tests/core/focus/goal/creation.spec.ts`:
- Line 54: The tests in creation.spec.ts use many non-deterministic
page.waitForTimeout calls (e.g., the call to page.waitForTimeout(800) and the
other occurrences) which cause flakiness; replace each waitForTimeout with a
deterministic wait that targets the actual condition: use page.waitForSelector
or locator.waitFor (for elements appearing/visible),
expect(locator).toBeVisible()/toHaveText() (for assertions),
page.waitForResponse or page.waitForRequest (for network-driven transitions), or
page.waitForURL (for navigation) depending on the step; locate each occurrence
by searching for page.waitForTimeout in creation.spec.ts and update the
surrounding test step (e.g., the action that triggers an element to appear or a
navigation) to wait on that specific element/state instead of a fixed timeout so
the test becomes deterministic.
In `@apps/e2e-playwright/tests/core/focus/task/creation.spec.ts`:
- Around line 82-92: The selector for the add-task button is fragile (using
taskListToolbar.getByRole("button").last()); add a stable data-testid attribute
to the "add task" button in the UI (e.g., data-testid="task-list-add-button")
and update the test to query that test id (use page.getByTestId or an equivalent
locator scoped to taskListToolbar) instead of relying on .last() so the test
targets the specific add button reliably; update references in the test to use
the new test id and remove the .last() use of plusIconInHeader.
In `@apps/e2e-playwright/tests/core/memory/capture.spec.ts`:
- Around line 34-52: Extract the duplicated editor-selection and markdown-toggle
logic into a helper function named getCaptureEditor(page) that builds the
combined selector (using getByTestId("capture-editor").getByPlaceholder("Start
typing to capture...").or(...).first()), checks visibility via
editor.isVisible().catch(() => false), clicks the markdown toggle button
(getByRole("button", { name: /^Markdown$/i }).first()) and waits if needed, then
awaits editor.waitFor({ state: "visible", timeout: 8_000 }) and returns the
editor; replace both duplicated blocks with calls to getCaptureEditor(page) to
centralize behavior and remove repetition (references to editor and markdownBtn
in the existing diff should be replaced by this helper).
In `@apps/e2e-playwright/tests/core/memory/node/creation.spec.ts`:
- Around line 1-31: The tests in
apps/e2e-playwright/tests/core/memory/node/creation.spec.ts are placeholders
marked with test.skip and TODOs; to fix, implement the two skipped tests
("create node via command bar" and "create node via UI") by removing test.skip,
using ensureInAppOnHome(page) to reach the app state, then script the user
interactions to create a node via the command bar and via the UI, asserting the
expected post-creation state; alternatively, if you cannot implement now, open a
tracked issue referencing this spec so the SKIP_E2E guard (runtimeEnv?.SKIP_E2E)
and the Google auth route filter remain intact and future work can implement
test logic around ensureInAppOnHome, the route interception block, and the two
test names.
In `@apps/e2e-playwright/tests/nucleus/app-nav.spec.ts`:
- Line 56: Replace the hard-coded sleep calls (e.g., await
page.waitForTimeout(1_000)) with explicit, deterministic waits: locate the
element or state you expect after the delay (using the same page or Locator
instances used in the test) and wait on that condition with
page.waitForSelector, locator.waitFor, or Playwright assertions like
expect(locator).toBeVisible()/toHaveText()/toBeEnabled(); update every
occurrence noted (the waitForTimeout calls at lines referenced in the comment)
so each pause is replaced by a wait for the specific element/state transition
relevant to that test step, ensuring tests rely on element readiness rather than
fixed timeouts.
- Line 8: The variable baseURL declared as const baseURL =
runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173" is unused except for a
single fallback use later; remove this declaration and rely on Playwright's
config-provided baseURL or, if a local fallback is required, inline the fallback
where it’s actually used (replace the one-off reference with
runtimeEnv?.APP_BASE_URL ?? "http://127.0.0.1:4173") so there is no unused
binding; update any comments to document why a fallback is needed if you keep
it.
In `@apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts`:
- Line 18: Replace the hard sleep call await page.waitForTimeout(1_000) in the
test with an explicit wait for the expected condition: use Playwright's
page.waitForSelector or locator-based assertions (e.g., await
page.waitForSelector('<your-selector>') or await
expect(page.locator('<your-selector>')).toBeVisible()) or expect.poll for
polling, targeting the element/state the test needs before proceeding; locate
the call to page.waitForTimeout in calendar.spec.ts and swap it for the
appropriate explicit wait that matches the intended element or state.
In `@apps/e2e-playwright/tests/nucleus/overview/overview.spec.ts`:
- Around line 34-41: Extract the repeated Overview navigation block into a
helper function (e.g., navigateToOverview) that calls ensureInAppOnHome(page),
runCommand(page, "Overview"), then waits for the URL using the existing RegExp
against nucleusProductConfig.pathByNavLabel.Overview with the 10_000ms timeout
and the 1_500ms waitForTimeout; replace the seven inline occurrences (the blocks
around lines that perform page.waitForURL with the RegExp and
page.waitForTimeout) with calls to navigateToOverview(page) in each test to
remove duplication.
In `@apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts`:
- Around line 380-384: Replace the fragile parent traversal that uses
panel.getByText("Command bar", { exact: true }).locator("..") by selecting the
shortcut row with a stable selector: update the app to add a data-testid (e.g.,
data-testid="shortcut-row-command-bar") on the container and change the test to
locate that element (instead of using locator("..")), then get the first button
via that container (the variables to update are commandBarRow and shortcutBtn in
this spec); apply the same change for the other occurrences around lines 410-413
so tests target data-testid-backed selectors rather than relying on DOM parent
navigation.
- Around line 27-36: Two identical openSettings helper functions are duplicated;
remove the duplicate and move a single openSettings definition to file scope
(above the describe blocks) or into the shared helpers module, then import or
reference it from both describe blocks. Locate the function named openSettings
(it uses runCommand and the test id "topnav-account-settings" via profileBtn)
and consolidate that implementation into one top-level helper, delete the other
copy, and update both describe blocks to call the single shared openSettings.
In `@apps/e2e-playwright/tests/shared/navigation.spec.ts`:
- Around line 15-195: The test "already logged in (Google auth state): handle
old page if present, then verify in app `@smoke`" is too long and exercises
multiple concerns; split it into focused tests: extract the
clickContinueOfflineIfVisible helper into a shared helper function and call it
from smaller tests, create one test that verifies offline recovery loops (use
clickContinueOfflineIfVisible and the for-loops that retry reaching
homePageUrl), another test that asserts presence of nav markers (use
allMarkers/navMarkers/homeNav/todayButton and the expect.poll block), and a
third that performs the navigation action to Overview (use navAction,
testNavLabel, expectedPath and the final path assertions); ensure shared setup
(page.route, goto baseURL, waitForLoadState) is moved to a beforeEach or test
fixture so each new test remains focused and reuses the helper.
In `@apps/e2e-playwright/tests/smoke/home.spec.ts`:
- Around line 14-15: Replace the hard 2s sleep (page.waitForTimeout) with a
deterministic readiness check: remove page.waitForTimeout(2000) and instead wait
for a specific app-ready condition such as page.waitForLoadState("networkidle")
or page.waitForSelector(...) for a stable DOM element that indicates the home
screen is ready (use an existing selector like a test-id or a unique
heading/button). Update the test in apps/e2e-playwright/tests/smoke/home.spec.ts
to use page.waitForSelector or page.waitForLoadState("networkidle") in place of
page.waitForTimeout to make the test robust and non-flaky.
- Line 18: The URL assertion using expect(page).toHaveURL(/\//) is too
permissive; update the test to assert the app's configurable base path instead
of a bare slash. Replace the regex with one that checks for the configured base
path (e.g., using a runtime value like appConfig.basePath or a TEST_BASE_PATH
env var) so expect(page).toHaveURL(...) verifies the base path is present while
remaining agnostic to which specific product page is shown.
In `@apps/e2e-playwright/tests/utils/helpers.ts`:
- Around line 123-126: Replace the fixed delays around the keyboard interactions
by waiting for the UI elements to be ready: instead of await
page.waitForTimeout(500) and await page.waitForTimeout(100) before
page.keyboard.press("ArrowDown") and "Enter", wait for the command list or the
specific command option to appear/be actionable (e.g., use page.waitForSelector
or locator.waitFor with the command list selector or the command option selector
and ensure it is visible/enabled) and only then invoke
page.keyboard.press("ArrowDown") and page.keyboard.press("Enter"); update the
helper in helpers.ts where the waits appear so tests become deterministic and
avoid flakiness.
- Around line 4-7: Remove the dead variable by deleting the unused baseURL
declaration; if runtimeEnv is also unused elsewhere in helpers.ts then remove
both the runtimeEnv and baseURL lines to avoid dead code, otherwise keep
runtimeEnv and only remove the baseURL line (symbols to look for: baseURL and
runtimeEnv).
In `@client/components/shortcuts/settings/ShortcutItem.svelte`:
- Around line 187-195: The span wrapper around the accept Button exists only to
attach a data-testid; instead add a testId prop to the shared Button component
and pass it directly (e.g., replace <span data-testid="shortcut-accept"><Button
... /></span> with <Button testId="shortcut-accept" ... />). Update the Button
component to accept a testId (string | undefined) and render it as a data-testid
attribute on the root element, following the TextInput pattern; then remove the
extra span in ShortcutItem.svelte and update any other Button usages that rely
on wrapping for tests to use the new testId prop.
In `@client/elements/input/TextInput.svelte`:
- Around line 110-133: InlineMarkdownTextInput doesn't accept a testId prop, so
tests can't target the inner input when isExperimentalMdInput is true; add an
exported prop (e.g., export let testId) to the InlineMarkdownTextInput component
and forward that prop to the actual underlying input element (or root
interactive element) inside InlineMarkdownTextInput, then in TextInput.svelte
pass testId into <InlineMarkdownTextInput {testId} .../> (you may keep the
wrapper div's data-testid for backwards compatibility) so tests can target the
inner input element consistently with other input variants.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
apps/e2e-playwright/README.mdapps/e2e-playwright/global-setup.tsapps/e2e-playwright/playwright.config.tsapps/e2e-playwright/tests/core/focus/goal/browse.spec.tsapps/e2e-playwright/tests/core/focus/goal/bulk-editor.spec.tsapps/e2e-playwright/tests/core/focus/goal/context-menu.spec.tsapps/e2e-playwright/tests/core/focus/goal/creation.spec.tsapps/e2e-playwright/tests/core/focus/goal/record-page.spec.tsapps/e2e-playwright/tests/core/focus/task/browse.spec.tsapps/e2e-playwright/tests/core/focus/task/bulk-editor.spec.tsapps/e2e-playwright/tests/core/focus/task/context-menu.spec.tsapps/e2e-playwright/tests/core/focus/task/creation.spec.tsapps/e2e-playwright/tests/core/focus/task/record-page.spec.tsapps/e2e-playwright/tests/core/memory/capture.spec.tsapps/e2e-playwright/tests/core/memory/node/browse.spec.tsapps/e2e-playwright/tests/core/memory/node/bulk-editor.spec.tsapps/e2e-playwright/tests/core/memory/node/context-menu.spec.tsapps/e2e-playwright/tests/core/memory/node/creation.spec.tsapps/e2e-playwright/tests/core/memory/node/record-page.spec.tsapps/e2e-playwright/tests/memotron/.gitkeepapps/e2e-playwright/tests/nucleus/app-nav.spec.tsapps/e2e-playwright/tests/nucleus/calendar/calendar.spec.tsapps/e2e-playwright/tests/nucleus/collection/collection.spec.tsapps/e2e-playwright/tests/nucleus/overview/overview.spec.tsapps/e2e-playwright/tests/nucleus/settings/settings.spec.tsapps/e2e-playwright/tests/pointron/.gitkeepapps/e2e-playwright/tests/shared/navigation.spec.tsapps/e2e-playwright/tests/smoke/home.spec.tsapps/e2e-playwright/tests/utils/helpers.tsapps/e2e-playwright/tsconfig.jsonclient/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/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/products/memotron/capture/Writer.svelteclient/products/pointron/focus/quickstart/QuickStart.svelte
✅ Files skipped from review due to trivial changes (2)
- client/layout/leftPanel/LeftNavSettingsPopover.svelte
- client/components/commandBar/CommandBar.svelte
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
Remove .catch(() => null) from assertions.
Swallowing assertion failures defeats the test's purpose. If goalNameInput fails to become hidden, the test should fail rather than silently continue.
🔧 Proposed fix
- await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); | |
| await expect(goalNameInput).toBeHidden({ 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/core/focus/goal/browse.spec.ts` at line 38, The
test is swallowing assertion failures by appending .catch(() => null) to the
Playwright assertion; remove the .catch handler so the assertion can fail the
test as intended—replace the line using expect(goalNameInput).toBeHidden({
timeout: 10_000 }).catch(() => null) with a plain await
expect(goalNameInput).toBeHidden({ timeout: 10_000 }); (or, if you need a
clearer failure message, await expect(goalNameInput).toBeHidden({ timeout:
10_000 }) inside a try/rethrow that adds context) to let the assertion throw on
failure.
Comment on lines
+42
to
+44
| await page.getByRole("button", { name: /^Library$/i }).click({ timeout: 5_000 }); | ||
| await page.waitForURL((u) => /^\/library(\/.*)?$/.test(new URL(u).pathname), { timeout: 10_000 }); | ||
| await page.getByRole("button", { name: /^Goals(\s+\d+)?$/i }).first().click({ timeout: 5_000 }); |
Contributor
There was a problem hiding this comment.
Use product config for navigation labels instead of hardcoded strings.
Per reviewer feedback, navigation labels like "Library" and "Goals" are product-configurable and vary by product (memotron, pointron, nucleus). Import and use nucleusProductConfig.appMenuNavLabels or similar configuration rather than hardcoding these strings.
This applies throughout this file (lines 42-44, 81, 107, 113, 141, 151, 153).
🔧 Suggested approach
+import { nucleusProductConfig } from "../../../../config/nucleus-product.config";
import {
ensureInAppOnHome,
runCommand
} from "../../../utils/helpers";Then reference navigation labels from the config:
// Example: instead of hardcoded "Library"
await page.getByRole("button", { name: new RegExp(`^${nucleusProductConfig.appMenuNavLabels[2]}$`, "i") }).click();Consider extending nucleus-product.config.ts to include all needed labels if not already present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/core/focus/goal/browse.spec.ts` around lines 42 -
44, Replace hardcoded navigation labels in the test (e.g., the page.getByRole
calls that match "Library" and "Goals") to use the product config so labels vary
by product: import nucleusProductConfig (or the correct product config) into the
test and use nucleusProductConfig.appMenuNavLabels[index] (or a named field if
available) inside a case-insensitive RegExp for the name matcher instead of
literal strings; update all occurrences noted (the calls at the shown snippet
and the other places mentioned: lines 81, 107, 113, 141, 151, 153) and if any
label is missing add it to nucleus-product.config.ts so the tests reference
config-driven labels consistently.
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
Same issue: assertion failure silently swallowed.
Line 64 also catches and ignores assertion failures.
🔧 Proposed fix
- await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); | |
| await expect(goalNameInput).toBeHidden({ 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/core/focus/goal/browse.spec.ts` at line 64, The
test is swallowing assertion failures by appending .catch(() => null) to the
expect call for goalNameInput.toBeHidden; remove the silent catch so failures
surface (i.e., use await expect(goalNameInput).toBeHidden({ timeout: 10_000 })
without .catch), or if you need custom handling wrap the await expect(...) in a
try/catch that rethrows or fails with a clear message referencing goalNameInput
and the toBeHidden assertion.
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
|
|
||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
Remove .catch(() => null) from assertion.
Same issue as in browse.spec.ts—swallowing assertion failures allows false passes.
🔧 Proposed fix
- await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); | |
| await expect(goalNameInput).toBeHidden({ 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/core/focus/goal/creation.spec.ts` at line 45, The
test is swallowing assertion failures by appending .catch(() => null) to the
Playwright assertion; remove the .catch(() => null) so the assertion on
goalNameInput uses await expect(goalNameInput).toBeHidden({ timeout: 10_000 })
directly (no catch) to allow failures to propagate and fail the test; update any
similar occurrences (e.g., other tests like browse.spec.ts) that wrap
expect(...).toBeHidden or other expect(...) calls with .catch(() => null).
Comment on lines
+128
to
+137
| await page | ||
| .getByRole("button", { name: /^Library$/i }) | ||
| .click({ timeout: 5_000 }); | ||
| await page.waitForURL( | ||
| (u) => /^\/library(\/.*)?$/.test(new URL(u).pathname), | ||
| { timeout: 10_000 } | ||
| ); | ||
| await page.getByRole("button", { name: /^Goals(\s+\d+)?$/i }).first().click({ | ||
| timeout: 5_000 | ||
| }); |
Contributor
There was a problem hiding this comment.
Inconsistent use of product config for navigation labels.
This file imports nucleusProductConfig and uses it for timelinePageLabel and homePath, but still hardcodes "Library", "Goals", "Focus", and "Today" strings. For consistency and to support multiple products as requested by the reviewer, use the product configuration throughout.
🔧 Suggested approach
Extend nucleus-product.config.ts to include all navigation labels needed by tests, then use them consistently:
// In nucleus-product.config.ts, add labels for Library, Goals, Focus, etc.
export const nucleusProductConfig = {
// ... existing config
libraryLabel: "Library",
goalsLabel: "Goals",
focusLabel: "Focus",
// ...
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/core/focus/goal/creation.spec.ts` around lines 128
- 137, The tests inconsistently hardcode navigation labels ("Library", "Goals",
"Focus", "Today") instead of using nucleusProductConfig; extend
nucleusProductConfig to include libraryLabel, goalsLabel, focusLabel, todayLabel
(in addition to existing timelinePageLabel and homePath) and update the test
selectors (the calls using getByRole(..., { name: /^Library$/i }),
getByRole(..., { name: /^Goals(\s+\d+)?$/i }) and any similar occurrences) to
reference nucleusProductConfig.libraryLabel, .goalsLabel, .focusLabel,
.todayLabel (use the config values in the regex patterns if needed) so all
navigation labels come from the single product config.
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
Swallowing assertion failures defeats the test purpose.
Using .catch(() => null) on expect(...).toBeHidden() silently ignores failures. If the element not becoming hidden is acceptable, remove the assertion; otherwise, let it fail to surface real issues.
Proposed fix
- await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); | |
| await expect(goalNameInput).toBeHidden({ 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/calendar/calendar.spec.ts` at line 59, The
test currently swallows assertion failures by appending `.catch(() => null)` to
`await expect(goalNameInput).toBeHidden({ timeout: 10_000 })`; either remove the
entire assertion if the element may validly remain visible, or remove the
`.catch(() => null)` so the assertion can fail and surface issues. Update the
line referencing `goalNameInput`/`toBeHidden` accordingly (or replace with an
explicit conditional check like `await goalNameInput.isHidden()` and assert its
result) so failures are not silently ignored.
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
Same issue: swallowed assertion on line 92.
This .catch(() => null) also suppresses potential test failures.
Proposed fix
- await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); | |
| await expect(goalNameInput).toBeHidden({ 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/calendar/calendar.spec.ts` at line 92, The
test is swallowing assertion failures by appending .catch(() => null) to the
Playwright assertion on goalNameInput; remove the .catch and let await
expect(goalNameInput).toBeHidden({ timeout: 10_000 }) throw on failure (or
replace with an explicit try/catch that rethrows a descriptive Error) so failing
expectations are not suppressed.
Comment on lines
+37
to
+87
| const clickContinueOfflineIfVisible = async () => { | ||
| const offlineTab = page.getByRole("button", { name: "Offline" }).first(); | ||
| const continueOfflineMain = page | ||
| .getByRole("button", { name: /Continue (using )?offline/i }) | ||
| .filter({ hasText: /Single device|free forever|No signup/i }) | ||
| .first(); | ||
| const continueOfflineAny = page.getByRole("button", { | ||
| name: /Continue (using )?offline/i | ||
| }).first(); | ||
|
|
||
| const hasOfflineTab = await offlineTab.isVisible().catch(() => false); | ||
| if (hasOfflineTab) { | ||
| await offlineTab.click({ timeout: 5_000 }).catch(() => null); | ||
| } | ||
|
|
||
| const pathname = new URL(page.url()).pathname; | ||
| const waitMs = | ||
| pathname === "/signup" || pathname === "/account/login" ? 10_000 : 3_000; | ||
| let continueOffline = continueOfflineMain; | ||
| try { | ||
| await continueOfflineMain.waitFor({ state: "visible", timeout: waitMs }); | ||
| } catch { | ||
| try { | ||
| await continueOfflineAny.waitFor({ state: "visible", timeout: 2_000 }); | ||
| continueOffline = continueOfflineAny; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| { | ||
| const beforePath = new URL(page.url()).pathname; | ||
| await continueOffline.click({ timeout: 5_000, force: true }).catch(() => null); | ||
| await page.waitForLoadState("domcontentloaded").catch(() => null); | ||
|
|
||
| const progressed = await page | ||
| .waitForURL( | ||
| (u) => { | ||
| const p = new URL(u).pathname; | ||
| return p !== "/signup" && p !== "/account/login"; | ||
| }, | ||
| { timeout: 8_000 } | ||
| ) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
|
|
||
| if (progressed) return true; | ||
| const afterPath = new URL(page.url()).pathname; | ||
| return beforePath !== afterPath; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: clickContinueOfflineIfVisible is duplicated from helpers.ts.
This helper function is nearly identical to the one in apps/e2e-playwright/tests/utils/helpers.ts (lines 19-52). The test file should import and use the existing helper rather than duplicating it, or extract it to a shared location if the test needs a slightly different variant.
♻️ Suggested refactor
Import the shared helper instead of duplicating:
import { test, expect } from "@playwright/test";
import { nucleusProductConfig } from "../../config/nucleus-product.config";
+import { ensureInAppOnHome } from "../utils/helpers";Or if the test needs the offline click logic without full navigation, consider extracting clickContinueOfflineIfVisible as a separate export from helpers.ts.
🤖 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 37 - 87,
Remove the duplicated clickContinueOfflineIfVisible implementation from
navigation.spec.ts and import the shared helper from helpers.ts instead (use the
existing exported clickContinueOfflineIfVisible or export a small variant from
helpers.ts if this test needs a slightly different behavior); update the test to
call the imported function rather than the inline implementation and ensure any
test-specific timeout/behavior is passed via parameters or a new exported helper
in helpers.ts.
Comment on lines
+66
to
+75
| await page.goto(nucleusProductConfig.homePath, { waitUntil: "domcontentloaded" }); | ||
| await page | ||
| .waitForURL( | ||
| (u) => { | ||
| const p = new URL(u).pathname; | ||
| return p === "/calendar" || p.startsWith("/calendar/"); | ||
| }, | ||
| { timeout: 10_000 } | ||
| ) | ||
| .catch(() => null); |
Contributor
There was a problem hiding this comment.
Hardcoded /calendar path contradicts reviewer feedback.
The reviewer explicitly requested that tests must not hardcode "Calendar" as the default home page. While nucleusProductConfig.homePath is used for navigation, the waitForURL check on lines 70-71 hardcodes /calendar. This should derive from the config to support different products (memotron, pointron, nucleus).
🔧 Proposed fix
+ const expectedHomePath = `/${nucleusProductConfig.homePath}`;
await page.goto(nucleusProductConfig.homePath, { waitUntil: "domcontentloaded" });
await page
.waitForURL(
(u) => {
const p = new URL(u).pathname;
- return p === "/calendar" || p.startsWith("/calendar/");
+ return p === expectedHomePath || p.startsWith(`${expectedHomePath}/`);
},
{ timeout: 10_000 }
)
.catch(() => null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await page.goto(nucleusProductConfig.homePath, { waitUntil: "domcontentloaded" }); | |
| await page | |
| .waitForURL( | |
| (u) => { | |
| const p = new URL(u).pathname; | |
| return p === "/calendar" || p.startsWith("/calendar/"); | |
| }, | |
| { timeout: 10_000 } | |
| ) | |
| .catch(() => null); | |
| const expectedHomePath = `/${nucleusProductConfig.homePath}`; | |
| await page.goto(nucleusProductConfig.homePath, { waitUntil: "domcontentloaded" }); | |
| await page | |
| .waitForURL( | |
| (u) => { | |
| const p = new URL(u).pathname; | |
| return p === expectedHomePath || p.startsWith(`${expectedHomePath}/`); | |
| }, | |
| { timeout: 10_000 } | |
| ) | |
| .catch(() => null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e-playwright/tests/utils/helpers.ts` around lines 66 - 75, The
waitForURL logic hardcodes "/calendar"; update it to derive the expected
pathname from nucleusProductConfig.homePath instead: compute the pathname from
nucleusProductConfig.homePath (e.g., new
URL(nucleusProductConfig.homePath).pathname) and use that value in the
waitForURL predicate so the check uses the configured home path rather than
"/calendar"; update the predicate inside the waitForURL call that currently
references "/calendar" to compare against the computed pathname (and allow
subpaths with startsWith as before) while keeping the same timeout and error
handling around page.goto and waitForURL.
Comment on lines
+83
to
+98
| const navMarkers = [ | ||
| page.getByRole("button", { name: /^Overview$/i }).first(), | ||
| page.getByRole("button", { name: /^Calendar$/i }).first(), | ||
| page.getByRole("button", { name: "Today" }).first() | ||
| ]; | ||
| await expect | ||
| .poll( | ||
| async () => { | ||
| for (const marker of navMarkers) { | ||
| if (await marker.isVisible().catch(() => false)) return true; | ||
| } | ||
| return false; | ||
| }, | ||
| { timeout: 25_000 } | ||
| ) | ||
| .toBe(true); |
Contributor
There was a problem hiding this comment.
Hardcoded nav markers contradict reviewer feedback.
The nav markers hardcode "Overview", "Calendar", and "Today" instead of deriving from nucleusProductConfig.appMenuNavLabels. Per reviewer feedback, app menu items are product-configurable and should not be hardcoded.
🔧 Proposed fix
- const navMarkers = [
- page.getByRole("button", { name: /^Overview$/i }).first(),
- page.getByRole("button", { name: /^Calendar$/i }).first(),
- page.getByRole("button", { name: "Today" }).first()
- ];
+ const navMarkers = nucleusProductConfig.appMenuNavLabels.map((label) =>
+ page.getByRole("button", { name: new RegExp(`^${label}$`, "i") }).first()
+ );This uses the config-driven labels while keeping "Today" as a fallback marker if needed (can be added separately if it's a universal element).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const navMarkers = [ | |
| page.getByRole("button", { name: /^Overview$/i }).first(), | |
| page.getByRole("button", { name: /^Calendar$/i }).first(), | |
| page.getByRole("button", { name: "Today" }).first() | |
| ]; | |
| await expect | |
| .poll( | |
| async () => { | |
| for (const marker of navMarkers) { | |
| if (await marker.isVisible().catch(() => false)) return true; | |
| } | |
| return false; | |
| }, | |
| { timeout: 25_000 } | |
| ) | |
| .toBe(true); | |
| const navMarkers = nucleusProductConfig.appMenuNavLabels.map((label) => | |
| page.getByRole("button", { name: new RegExp(`^${label}$`, "i") }).first() | |
| ); | |
| await expect | |
| .poll( | |
| async () => { | |
| for (const marker of navMarkers) { | |
| if (await marker.isVisible().catch(() => false)) return true; | |
| } | |
| return false; | |
| }, | |
| { timeout: 25_000 } | |
| ) | |
| .toBe(true); |
🤖 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 83 - 98, navMarkers
currently hardcodes button labels ("Overview", "Calendar", "Today") which
violates the reviewer request to use product-configurable labels; update the
logic that builds navMarkers in helpers.ts to derive labels from
nucleusProductConfig.appMenuNavLabels (falling back to "Today" only if that
label is universally expected), i.e., iterate
nucleusProductConfig.appMenuNavLabels to create the page.getByRole(...)
selectors instead of hardcoded strings so tests follow the product config;
ensure navMarkers variable name and the expect.poll usage remain unchanged so
only the label source is swapped.
Contributor
There was a problem hiding this comment.
9 issues found across 51 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/core/focus/goal/browse.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/core/focus/goal/browse.spec.ts:38">
P2: Avoid swallowing assertion failures in the test; it hides UI regressions and can let the test pass even when the goal dialog never closes.</violation>
</file>
<file name="apps/e2e-playwright/tests/core/memory/node/record-page.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/core/memory/node/record-page.spec.ts:22">
P3: These new tests are unconditionally skipped, so this suite will never execute and provides no regression coverage. Convert them to active `test(...)` (or gate with a meaningful condition) once implemented so the added file exercises the flow.</violation>
</file>
<file name="apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/nucleus/settings/settings.spec.ts:27">
P3: `openSettings` is duplicated in the same spec file. Extract it once (e.g., top-level helper) and reuse it to avoid drift.</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:30">
P2: Avoid duplicating the timeout and initial navigation in every test. Configure block timeouts using `test.describe.configure()` and move `ensureInAppOnHome` to the `test.beforeEach` hook.</violation>
<violation number="2" location="apps/e2e-playwright/tests/nucleus/app-nav.spec.ts:56">
P2: Avoid using hardcoded `page.waitForTimeout()` calls as they cause flakiness and slow down test execution. Rely on Playwright's auto-waiting web-first assertions instead.</violation>
<violation number="3" location="apps/e2e-playwright/tests/nucleus/app-nav.spec.ts:71">
P1: Avoid using `isVisible()` for control flow as it evaluates immediately and causes flaky tests on slow renders. Use `locator.or()` instead.</violation>
</file>
<file name="apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts:25">
P2: Avoid swallowing click failures; this masks broken UI behavior and can let the test pass without selecting a duration.</violation>
<violation number="2" location="apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts:59">
P2: Don’t swallow assertion failures; it prevents the test from detecting the UI regression it’s meant to catch.</violation>
</file>
<file name="apps/e2e-playwright/tests/core/focus/task/browse.spec.ts">
<violation number="1" location="apps/e2e-playwright/tests/core/focus/task/browse.spec.ts:17">
P2: Await or return route.abort()/route.continue() in the route handler to ensure Playwright waits for the routing decision.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const timelineInPanel = calendarColumn | ||
| .getByRole("button", { name: /Timeline/i }) | ||
| .first(); | ||
| const hasOverview = await overviewInPanel.isVisible().catch(() => false); |
Contributor
There was a problem hiding this comment.
P1: Avoid using isVisible() for control flow as it evaluates immediately and causes flaky tests on slow renders. Use locator.or() instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/nucleus/app-nav.spec.ts, line 71:
<comment>Avoid using `isVisible()` for control flow as it evaluates immediately and causes flaky tests on slow renders. Use `locator.or()` instead.</comment>
<file context>
@@ -0,0 +1,281 @@
+ const timelineInPanel = calendarColumn
+ .getByRole("button", { name: /Timeline/i })
+ .first();
+ const hasOverview = await overviewInPanel.isVisible().catch(() => false);
+ const panelRow = hasOverview
+ ? overviewInPanel.locator("..").locator("..")
</file context>
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
P2: Avoid swallowing assertion failures in the test; it hides UI regressions and can let the test pass even when the goal dialog never closes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/core/focus/goal/browse.spec.ts, line 38:
<comment>Avoid swallowing assertion failures in the test; it hides UI regressions and can let the test pass even when the goal dialog never closes.</comment>
<file context>
@@ -0,0 +1,179 @@
+ await goalNameInput.waitFor({ state: "visible", timeout: 15_000 });
+ await goalNameInput.fill(goalName);
+ await page.keyboard.press("Enter");
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await page.keyboard.press("Escape");
+ await page.keyboard.press("Escape");
</file context>
| @@ -0,0 +1,281 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
Contributor
There was a problem hiding this comment.
P2: Avoid duplicating the timeout and initial navigation in every test. Configure block timeouts using test.describe.configure() and move ensureInAppOnHome to the test.beforeEach hook.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/nucleus/app-nav.spec.ts, line 30:
<comment>Avoid duplicating the timeout and initial navigation in every test. Configure block timeouts using `test.describe.configure()` and move `ensureInAppOnHome` to the `test.beforeEach` hook.</comment>
<file context>
@@ -0,0 +1,281 @@
+ test("open Logs via command bar (See Logs), then assert Logs view visible", async ({
+ page
+ }) => {
+ test.setTimeout(45_000);
+ await ensureInAppOnHome(page);
+ await runCommand(page, "See Logs");
</file context>
| @@ -0,0 +1,281 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
Contributor
There was a problem hiding this comment.
P2: Avoid using hardcoded page.waitForTimeout() calls as they cause flakiness and slow down test execution. Rely on Playwright's auto-waiting web-first assertions instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/nucleus/app-nav.spec.ts, line 56:
<comment>Avoid using hardcoded `page.waitForTimeout()` calls as they cause flakiness and slow down test execution. Rely on Playwright's auto-waiting web-first assertions instead.</comment>
<file context>
@@ -0,0 +1,281 @@
+ ).test(new URL(u).pathname),
+ { timeout: 10_000 }
+ );
+ await page.waitForTimeout(1_000);
+
+ await page
</file context>
| await goalNameInput.waitFor({ state: "visible", timeout: 15_000 }); | ||
| await goalNameInput.fill(goalName); | ||
| await page.keyboard.press("Enter"); | ||
| await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
P2: Don’t swallow assertion failures; it prevents the test from detecting the UI regression it’s meant to catch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts, line 59:
<comment>Don’t swallow assertion failures; it prevents the test from detecting the UI regression it’s meant to catch.</comment>
<file context>
@@ -0,0 +1,114 @@
+ await goalNameInput.waitFor({ state: "visible", timeout: 15_000 });
+ await goalNameInput.fill(goalName);
+ await page.keyboard.press("Enter");
+ await expect(goalNameInput).toBeHidden({ timeout: 10_000 }).catch(() => null);
+ await page.keyboard.press("Escape");
+ await page.keyboard.press("Escape");
</file context>
| .getByRole("button", { name: /last\s+10\s*min/i }) | ||
| .first(); | ||
| const hasQuick = await quickDurationBtn.isVisible().catch(() => false); | ||
| if (hasQuick) await quickDurationBtn.click({ timeout: 2_000 }).catch(() => null); |
Contributor
There was a problem hiding this comment.
P2: Avoid swallowing click failures; this masks broken UI behavior and can let the test pass without selecting a duration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/nucleus/calendar/calendar.spec.ts, line 25:
<comment>Avoid swallowing click failures; this masks broken UI behavior and can let the test pass without selecting a duration.</comment>
<file context>
@@ -0,0 +1,114 @@
+ .getByRole("button", { name: /last\s+10\s*min/i })
+ .first();
+ const hasQuick = await quickDurationBtn.isVisible().catch(() => false);
+ if (hasQuick) await quickDurationBtn.click({ timeout: 2_000 }).catch(() => null);
+ await page.waitForTimeout(300);
+
</file context>
| test.beforeEach(async ({ page }) => { | ||
| await page.route("**/*", (route) => { | ||
| const reqUrl = route.request().url(); | ||
| if (/accounts\.google\.com/i.test(reqUrl)) route.abort(); |
Contributor
There was a problem hiding this comment.
P2: Await or return route.abort()/route.continue() in the route handler to ensure Playwright waits for the routing decision.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/core/focus/task/browse.spec.ts, line 17:
<comment>Await or return route.abort()/route.continue() in the route handler to ensure Playwright waits for the routing decision.</comment>
<file context>
@@ -0,0 +1,35 @@
+ 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();
+ });
</file context>
| }); | ||
| }); | ||
|
|
||
| test.skip("open node record and assert panels/content visible", async ({ page }) => { |
Contributor
There was a problem hiding this comment.
P3: These new tests are unconditionally skipped, so this suite will never execute and provides no regression coverage. Convert them to active test(...) (or gate with a meaningful condition) once implemented so the added file exercises the flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/e2e-playwright/tests/core/memory/node/record-page.spec.ts, line 22:
<comment>These new tests are unconditionally skipped, so this suite will never execute and provides no regression coverage. Convert them to active `test(...)` (or gate with a meaningful condition) once implemented so the added file exercises the flow.</comment>
<file context>
@@ -0,0 +1,31 @@
+ });
+ });
+
+ test.skip("open node record and assert panels/content visible", async ({ page }) => {
+ await ensureInAppOnHome(page);
+ // TODO
</file context>
| }); | ||
|
|
||
| /** Open Settings: try profile icon first, fallback to command bar "Settings". */ | ||
| async function openSettings(page: import("@playwright/test").Page) { |
Contributor
There was a problem hiding this comment.
P3: openSettings is duplicated in the same spec file. Extract it once (e.g., top-level helper) and reuse it to avoid 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/nucleus/settings/settings.spec.ts, line 27:
<comment>`openSettings` is duplicated in the same spec file. Extract it once (e.g., top-level helper) and reuse it to avoid drift.</comment>
<file context>
@@ -0,0 +1,423 @@
+ });
+
+ /** Open Settings: try profile icon first, fallback to command bar "Settings". */
+ async function openSettings(page: import("@playwright/test").Page) {
+ const profileBtn = page.getByTestId("topnav-account-settings");
+ const visible = await profileBtn.isVisible().catch(() => false);
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add an end-to-end Playwright regression test for the login/signup redirect flow and make appearance typeface usage null-safe across the client.
New Features:
Enhancements:
Build:
Tests:
Summary by cubic
Adds a robust Playwright E2E suite for Google auth and core Nucleus flows, with stable test selectors and per-product runs, fulfilling TIDY-399. Also makes appearance typeface usage null-safe and improves nav accessibility.
New Features
Bug Fixes
Written for commit e7900e1. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Tests
Accessibility