test: create end-to-end playwright suite#1648
Conversation
WalkthroughAdds a Playwright-based end-to-end test workspace under e2e/, including project config, fixtures (auth/logger), page objects, helpers, toast/reporting utilities, multiple test specs, workspace scripts, sample env/gitignore, documentation, and orchestration (justfile). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as pnpm (e2e)
participant Playwright as Playwright Runner
participant LoggerFx as Logger Fixture
participant AuthFx as Auth Fixture
participant LoginPO as LoginPage
participant Server as Unraid Server
Dev->>CLI: pnpm --filter @unraid/e2e test
CLI->>Playwright: start tests
Playwright->>LoggerFx: init logger (attachToTest)
Playwright->>AuthFx: setup auth fixture
AuthFx->>Server: GET /
alt login form present
AuthFx->>LoginPO: fill credentials & submit
LoginPO->>Server: POST /login
Server-->>AuthFx: redirect/ok
AuthFx->>Playwright: save storageState (auth.json)
else no login needed
AuthFx-->>Playwright: proceed without login
end
Playwright->>Server: execute test actions via POs/helpers
Playwright->>LoggerFx: detach logger, attach logs to test results
Playwright-->>Dev: test results and reports
sequenceDiagram
autonumber
participant Test as Playwright Test
participant SettingsPO as SettingsPage
participant UI as Unraid WebGUI
participant ToastH as Toast Helpers
participant Reporter as reporting.ts
Test->>SettingsPO: goto /Settings
SettingsPO->>UI: request page
UI-->>SettingsPO: settings rendered
Test->>SettingsPO: setTheme(light/dark)
SettingsPO->>UI: apply theme change
UI-->>Test: UI updated
Test->>ToastH: triggerToast(type, message)
ToastH->>UI: window.toast(...)
UI-->>ToastH: toast element appears
Test->>ToastH: getToastStyles()
ToastH-->>Test: bg/text rgb + luminance
Test->>Reporter: report/annotate contrast metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @pujitm's task —— View job Code Review for E2E Playwright SuiteReviewed the diff for critical issues in the newly added E2E test suite. Todo List:
Review Results✅ No critical issues found in changes The E2E test suite addition contains well-structured Playwright test code with the following observations: Security:
Bugs/Logic:
Breaking Changes:
Data Loss Risks:
The implementation follows Playwright best practices with page object models, proper fixtures, and comprehensive test coverage for authentication, navigation, and UI functionality. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
+ Coverage 53.85% 54.58% +0.72%
==========================================
Files 797 809 +12
Lines 44310 45445 +1135
Branches 4493 4675 +182
==========================================
+ Hits 23864 24804 +940
- Misses 20410 20604 +194
- Partials 36 37 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
♻️ Duplicate comments (1)
e2e/tests/auth.spec.ts (1)
1-3: Same import-extension concern as dashboard.spec.tsEnsure tsconfig NodeNext/Bundler resolution is configured so .js extensions in TS imports compile cleanly.
Reuse the script from the dashboard.spec.ts comment.
🧹 Nitpick comments (35)
e2e/.gitignore (1)
1-14: Broaden ignores for Playwright cache and env variants.Consider ignoring the actual Playwright cache path and common env variants to avoid churn.
node_modules/ /test-results/ /playwright-report/ /blob-report/ -/playwright/.cache/ +/node_modules/.cache/ms-playwright/ .env .env.local +.env.* *.log screenshots/ videos/ traces/ # specific to each individual developer, not the project auth.json +.DS_Store +*.trace.zip +junit*.xmle2e/.env.example (1)
1-18: Address dotenv lint nits and clarify credential handling.Minor ordering/formatting nits and a safety note.
+# Do NOT commit real credentials. Copy to .env and keep it local. # Unraid Server Configuration UNRAID_URL=http://tower.local # Authentication (if needed) -UNRAID_USERNAME=root -UNRAID_PASSWORD= +UNRAID_PASSWORD= +UNRAID_USERNAME=root # Test Configuration -TEST_TIMEOUT=60000 -RETRY_COUNT=2 +RETRY_COUNT=2 +TEST_TIMEOUT=60000 # Browser Configuration HEADLESS=true SLOW_MO=0 # Debugging DEBUG=false -TRACE=false +TRACE=false +e2e/tsconfig.json (1)
1-24: TS config is solid for Playwright; optional: use Bundler resolution to align with ESM.Current config will work. If you hit ESM/extension issues, switch moduleResolution to "Bundler" and enable verbatimModuleSyntax.
"compilerOptions": { "target": "ESNext", "module": "ESNext", - "moduleResolution": "node", + "moduleResolution": "Bundler", "strict": true, + "verbatimModuleSyntax": true, "esModuleInterop": true, "skipLibCheck": true, "forceConsistentCasingInFileNames": true, "resolveJsonModule": true, "allowSyntheticDefaultImports": true, "lib": ["ESNext", "DOM"], "types": ["@playwright/test", "node"],e2e/README.md (6)
7-16: Add language to fenced code blocks (markdownlint MD040).Specify bash for shell snippets.
- ```bash + ```bash cp .env.example .env ```
18-23: Mark env snippet as properties and mask password placeholder.Improves readability and avoids bare credential examples.
- ``` - UNRAID_URL=http://tower.local - UNRAID_USERNAME=root - UNRAID_PASSWORD=your_password - ``` + ```properties + UNRAID_URL=http://tower.local + UNRAID_USERNAME=root + UNRAID_PASSWORD=******** + ```
27-45: Ensure workspace commands are unambiguous.Use filter when running from repo root; retain local commands when cd e2e/.
-# Run all tests -pnpm test +# Run all tests (from e2e/): pnpm test +# Or from repo root: +pnpm --filter @unraid/e2e test-UNRAID_URL=http://192.168.1.100 pnpm test:e2e +UNRAID_URL=http://192.168.1.100 pnpm --filter @unraid/e2e test
49-57: Add language to tree snippet (markdownlint MD040).-``` +```text e2e/ ├── fixtures/ # Test fixtures and setup ├── tests/ # Test specifications ├── utils/ # Helper functions and page objects │ ├── pages/ # Page object models │ └── helpers.ts # Utility functions └── playwright.config.ts--- `95-102`: **CI snippet: ensure correct script name and workspace filter.** Aligns with workspace usage. ```diff run: pnpm test:e2e + # or + # run: pnpm --filter @unraid/e2e test
111-120: Expand env table and wrap default URL in code formatting (MD034).Document all supported vars for parity with .env.example.
-| UNRAID_URL | Unraid server URL | http://tower.local | +| UNRAID_URL | Unraid server URL | `http://tower.local` | | UNRAID_USERNAME | Username for auth | root | | UNRAID_PASSWORD | Password for auth | - | | HEADLESS | Run in headless mode | true | | SLOW_MO | Slow down actions (ms) | 0 | | DEBUG | Enable debug mode | false | +| TRACE | Record traces on retry | false | +| TEST_TIMEOUT | Per-test timeout (ms) | 60000 | +| RETRY_COUNT | Retries per test | 2 |package.json (1)
12-16: Prevent e2e from running with the default monorepo test commandBecause the root "test" runs "pnpm -r test", adding a "test" script in @unraid/e2e means CI/devs invoking just "pnpm test" will now execute heavy Playwright runs. Recommend gating e2e behind an explicit opt-in or separate filter to avoid surprising slow pipelines.
Proposed approaches (pick one):
- Keep @unraid/e2e "test" as-is but change root "test" to exclude e2e by default:
- "test": "pnpm -r test", + "test": "pnpm -r --filter '...[!@unraid/e2e]' test",
- Or keep root "test" unchanged, and in e2e/package.json change "test" to a no-op with guidance, and rely on the new root wrappers to actually run e2e:
- "test": "playwright test", + "test": "echo \"Skipping by default. Use: pnpm test:e2e\"",Also consider adding a CI-friendly installer:
+ "playwright:install:ci": "pnpm --filter @unraid/e2e exec playwright install --with-deps"e2e/playwright.config.ts (2)
31-37: Consider aligning global timeout/expect timeouts with action/navigationGlobal timeout is 60s while action/navigation is 30s and expect is 20s; this may mask slowness by failing at different layers inconsistently. Optional: normalize to a single budget (e.g., 30s) unless you have a specific reason to diverge.
55-58: Mobile project naming consistencyName projects consistently (e.g., "mobile-chromium") to simplify shard filters like "--project mobile-chromium".
- name: 'Mobile Chrome', + name: 'mobile-chromium',e2e/utils/reporting.ts (1)
13-16: Annotations: consider attaching structured data separatelyPushing the entire message (including data) is fine structurally, but Playwright only renders "type" and "description". If you need rich data, attach it for later inspection.
Example:
if (message.data) { await testInfo.attach('report-data.json', { contentType: 'application/json', body: Buffer.from(JSON.stringify(message.data, null, 2)), }); } testInfo.annotations.push({ type: message.type, description: message.description });e2e/package.json (1)
1-21: Add CI-friendly install and an explicit "clean"Playwright on Linux often needs system deps; provide a CI install script and a clean target for local runs.
Suggested additions:
"scripts": { - "test": "playwright test", + "test": "playwright test", "test:headed": "playwright test --headed", "test:debug": "playwright test --debug", "test:ui": "playwright test --ui", "test:report": "playwright show-report", - "playwright:install": "playwright install", + "playwright:install": "playwright install", + "playwright:install:ci": "playwright install --with-deps", + "clean": "rimraf playwright-report test-results || true", "codegen": "playwright codegen" },If you prefer to avoid adding rimraf as a dep, replace "clean" with a portable Node one-liner.
e2e/utils/pages/settings.page.ts (2)
20-29: Harden navigation fallback with visibility checksIf the Display Settings link isn’t visible, the click will flake. Guard with visibility and a more deterministic URL check.
- const currentUrl = this.page.url(); - if (!currentUrl.includes('DisplaySettings')) { + if (!/\/Settings\/DisplaySettings\b/.test(this.page.url())) { await this.goto(); - await this.displaySettingsLink.click(); + await this.displaySettingsLink.waitFor({ state: 'visible', timeout: 10_000 }); + await this.displaySettingsLink.click(); await this.page.waitForLoadState('networkidle'); }
11-13: Stabilize selectors with data attributes if availableText-based selectors are brittle. If the WebGUI can expose data-testid/data-qa, switch to those for the theme select, Apply button, and Display Settings link.
e2e/utils/helpers.ts (2)
51-53: Use slice instead of deprecated substrMinor cleanup; slice is preferred and avoids deprecation.
-export function generateTestId(): string { - return `test-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; -} +export function generateTestId(): string { + return `test-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`; +}
3-5: Optional: avoid networkidle as the default waitnetworkidle can be brittle with long-lived connections. Consider domcontentloaded or a specific UI-ready locator in call sites.
e2e/utils/pages/login.page.ts (1)
10-16: Prefer role-based selectors for resilienceWhere possible, use getByRole/getByLabel over CSS. Improves accessibility alignment and reduces selector churn.
e2e/utils/toast-helpers.ts (2)
25-33: Use role-based fallback for toastsAdd ARIA roles commonly used for toasts to reduce selector brittleness.
export async function waitForToast(page: Page) { // Look for toast elements in the Unraid toaster structure - const toastLocator = page.locator('ol.toaster li').first(); + const toastLocator = page.locator('ol.toaster li, [role="status"], [role="alert"]').first();
53-58: Unify luminance computation to a single, correct routineYou compute “luminance” two different ways. Reuse the WCAG relative luminance function for consistency.
-export function isLightBackground(rgb: number[]): boolean { - if (rgb.length < 3) return true; - // Calculate luminance - const luminance = (0.299 * rgb[0] + 0.587 * rgb[1] + 0.114 * rgb[2]) / 255; - return luminance > 0.5; -} +export function isLightBackground(rgb: number[]): boolean { + if (rgb.length < 3) return true; + return getLuminance(rgb) > 0.5; +} @@ -export function getLuminance(rgb: number[]): number { - if (rgb.length < 3) return 0.5; - // Calculate luminance (0 = dark, 1 = light) - return (0.299 * rgb[0] + 0.587 * rgb[1] + 0.114 * rgb[2]) / 255; -} +export function getLuminance(rgb: number[]): number { + if (rgb.length < 3) return 0.5; + const toLinear = (v: number) => { + const s = v / 255; + return s <= 0.03928 ? s / 12.92 : Math.pow((s + 0.055) / 1.055, 2.4); + }; + const [r, g, b] = rgb.map(toLinear); + return 0.2126 * r + 0.7152 * g + 0.0722 * b; +}Also applies to: 94-97
e2e/utils/pages/dashboard.page.ts (2)
22-28: Stabilize post-navigation stateAfter goto, wait for a visible, page-specific marker to reduce flakiness.
async goto() { await this.page.goto('/Dashboard'); + await this.mainContent.waitFor({ state: 'visible' }); }
44-46: Prefer role-based navigation for resiliencegetByRole('link', { name: menuItem }) tends to be more stable than text CSS. Consider switching if the DOM exposes accessible names.
- await this.navigationMenu.locator(`a:has-text("${menuItem}")`).first().click(); + await this.navigationMenu.getByRole('link', { name: menuItem }).first().click();e2e/tests/docker.spec.ts (1)
26-27: Prefer test.info().log over console.log in testsPlaywright surfaces test.info logs in reports and traces; replace console.log for better CI visibility.
Also applies to: 32-33, 46-47, 56-57, 62-63, 76-77, 92-93, 115-116
e2e/tests/dashboard.spec.ts (1)
95-101: Drop fixed waits after viewport changesThe explicit 500ms sleep can be flaky and slow. Rely on visibility assertion with default timeout.
- await page.waitForTimeout(500); - await expect(dashboard.mainContent).toBeVisible();e2e/tests/auth.spec.ts (2)
24-27: Prefer declarative skip over imperative returnMinor: you can gate the test with a single call and drop the manual return.
- if (!password) { - test.skip(); - return; - } + test.skip(!password, 'Set UNRAID_PASSWORD to run authenticated login tests');
67-77: Speed up authenticated tests by reusing storage stateYou already persist auth.json in the fixture. Consider using it at project/test level to avoid logging in every test.
Example (playwright.config):
use: { baseURL: process.env.BASE_URL || 'http://localhost:3000', storageState: process.env.UNRAID_PASSWORD ? 'auth.json' : undefined, }e2e/tests/theme-toast.spec.ts (2)
46-50: Avoid double-waiting for the same toastgetToastStyles already waits for a toast. The explicit waitForToast + visible check is redundant.
- const toastElement = await waitForToast(page); - await expect(toastElement).toBeVisible(); - - const styles = await getToastStyles(page); + const styles = await getToastStyles(page);If you prefer to reuse the located element, add a helper overload (in toast-helpers.ts):
export async function getToastStylesFrom(toast: Locator) { /* same body, using provided element */ }Also applies to: 121-125
100-101: Replace fixed sleeps with state-based waitsUse locator counts/visibility instead of arbitrary timeouts between toasts.
- await page.waitForTimeout(1000); + await expect(page.locator('ol.toaster li')).toHaveCount(0);- await page.waitForTimeout(2000); + await expect(page.locator('ol.toaster li')).toHaveCount(0);Also applies to: 173-174, 199-201
e2e/fixtures/auth.fixture.ts (6)
16-17: Prefer nullish coalescing for env defaultsThis avoids treating empty strings as “unset.”
-const username = process.env.UNRAID_USERNAME || 'root'; -const password = process.env.UNRAID_PASSWORD || ''; +const username = process.env.UNRAID_USERNAME ?? 'root'; +const password = process.env.UNRAID_PASSWORD ?? '';
23-23: Avoid networkidle; it’s flaky with long‑polling/WSUse a lighter wait and then wait on concrete UI conditions.
- await page.waitForLoadState('networkidle'); + await page.waitForLoadState('domcontentloaded'); @@ - await page.waitForLoadState('networkidle'); + await page.waitForLoadState('domcontentloaded');Also applies to: 39-39
29-33: Reuse the LoginPage POM to avoid selector driftUse the page object you already have for filling and login state checks.
- // Try different selector combinations for username/password fields - const usernameInput = page.locator('input[name="username"], input#user').first(); - const passwordInput = page.locator('input[name="password"], input#pass').first(); - const submitButton = page.locator('button[type="submit"], input[type="submit"], button:has-text("Login")').first(); - - await usernameInput.fill(username); - await passwordInput.fill(password); - await submitButton.click(); + const lp = new LoginPage(page); + await lp.login(username, password); @@ - // Check if we successfully logged in - const stillOnLogin = page.url().includes('login'); - if (!stillOnLogin) { + const loggedIn = await lp.isLoggedIn(); + if (loggedIn) { await page.context().storageState({ path: 'auth.json' }); }Also applies to: 41-45
41-45: Fail fast with a clear error when login failsSurface the UI error so the test suite exits early with context.
// Check if we successfully logged in - const stillOnLogin = page.url().includes('login'); + const loginError = page.locator('.error, .alert-error, [role="alert"]'); + if (await loginError.isVisible()) { + throw new Error(`Login failed: ${await loginError.textContent()}`); + } + const stillOnLogin = page.url().includes('login');
44-44: Persist auth state in a repo-local path and wire it in configStore under e2e/.auth and reference via test.use({ storageState }) so other tests can reuse it.
- await page.context().storageState({ path: 'auth.json' }); + await page.context().storageState({ path: 'e2e/.auth/auth.json' });Outside this file (playwright.config.ts):
import { defineConfig } from '@playwright/test'; export default defineConfig({ // ... use: { baseURL: process.env.BASE_URL ?? 'http://localhost:3000', }, projects: [ { name: 'chromium', use: { storageState: 'e2e/.auth/auth.json' }, }, // other browsers... ], });
6-6: Fixture naming nitauthenticatedPage does not provide a Page; consider login or authenticated to avoid confusion in test code completion.
- authenticatedPage: void; + authenticated: void; @@ - authenticatedPage: async ({ page }, use) => { + authenticated: async ({ page }, use) => {Also applies to: 15-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
e2e/.env.example(1 hunks)e2e/.gitignore(1 hunks)e2e/README.md(1 hunks)e2e/fixtures/auth.fixture.ts(1 hunks)e2e/package.json(1 hunks)e2e/playwright.config.ts(1 hunks)e2e/tests/auth.spec.ts(1 hunks)e2e/tests/dashboard.spec.ts(1 hunks)e2e/tests/docker.spec.ts(1 hunks)e2e/tests/theme-toast.spec.ts(1 hunks)e2e/tsconfig.json(1 hunks)e2e/utils/helpers.ts(1 hunks)e2e/utils/pages/dashboard.page.ts(1 hunks)e2e/utils/pages/login.page.ts(1 hunks)e2e/utils/pages/settings.page.ts(1 hunks)e2e/utils/reporting.ts(1 hunks)e2e/utils/toast-helpers.ts(1 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
e2e/fixtures/auth.fixture.tse2e/utils/reporting.tse2e/tests/dashboard.spec.tse2e/tests/theme-toast.spec.tse2e/tests/docker.spec.tse2e/utils/pages/login.page.tse2e/utils/pages/settings.page.tse2e/tests/auth.spec.tse2e/playwright.config.tse2e/utils/pages/dashboard.page.tse2e/utils/helpers.tse2e/utils/toast-helpers.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/__test__/**/*.@(ts|tsx|js|jsx) : Web tests are located under web/__test__
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/**/*.{spec,test}.@(ts|tsx|js|jsx) : Test component behavior and rendered output, not internal implementation details
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Run terminal commands with pnpm within the web directory (e.g., pnpm test)
Applied to files:
package.json
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Run API tests with: pnpm --filter ./api test
Applied to files:
package.json
📚 Learning: 2025-08-11T15:07:39.222Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-08-11T15:07:39.222Z
Learning: Run tests with pnpm --filter ./api test
Applied to files:
package.json
📚 Learning: 2025-08-11T15:07:39.222Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-08-11T15:07:39.222Z
Learning: Use pnpm only (do not use npm/yarn)
Applied to files:
package.json
📚 Learning: 2025-02-05T14:43:48.568Z
Learnt from: elibosley
PR: unraid/api#1120
File: plugin/package.json:0-0
Timestamp: 2025-02-05T14:43:48.568Z
Learning: In Node.js projects, npm scripts should be organized with clear namespacing (e.g., build:*, docker:*, env:*) and include proper environment validation and error handling. Each script should follow the single responsibility principle.
Applied to files:
package.json
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Use pnpm only for package management
Applied to files:
package.json
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Always run scripts from api/package.json unless requested otherwise
Applied to files:
package.json
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
package.json
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/__test__/**/*.@(ts|tsx|js|jsx) : Web tests are located under web/__test__
Applied to files:
e2e/tsconfig.jsonpnpm-workspace.yaml
📚 Learning: 2025-05-23T21:59:29.632Z
Learnt from: elibosley
PR: unraid/api#1408
File: web/components/ApiKey/PermissionCounter.vue:6-6
Timestamp: 2025-05-23T21:59:29.632Z
Learning: This codebase uses ESM (ECMAScript Modules) and requires .js extensions in import statements, even when importing from TypeScript files, as the imports refer to the compiled JavaScript output.
Applied to files:
e2e/tsconfig.json
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to api/src/**/*.ts : In API TypeScript, use .js extensions in import specifiers for ESM compatibility
Applied to files:
e2e/tsconfig.json
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to {api,web}/**/*.{spec,test}.@(ts|tsx|js|jsx) : Avoid brittle tests that depend on non-essential details (e.g., log formats)
Applied to files:
e2e/tsconfig.jsone2e/tests/theme-toast.spec.tse2e/tests/auth.spec.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to api/**/vitest.config.@(js|ts|mjs|cjs) : API test runner is Vitest
Applied to files:
e2e/tsconfig.jsone2e/playwright.config.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/**/{store,stores}/**/*.{spec,test}.@(ts|js) : Use factory functions for module mocks to avoid hoisting issues
Applied to files:
e2e/fixtures/auth.fixture.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Applied to files:
e2e/fixtures/auth.fixture.tse2e/utils/helpers.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Applied to files:
e2e/fixtures/auth.fixture.tse2e/utils/helpers.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Assert rendered text with wrapper.text(), check attributes via element.attributes(), and verify element existence with expect(element.exists()).toBe(true)
Applied to files:
e2e/tests/theme-toast.spec.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/**/*.{spec,test}.@(ts|tsx|js|jsx) : Use await nextTick() for DOM updates before assertions
Applied to files:
e2e/utils/helpers.ts
🧬 Code graph analysis (6)
e2e/fixtures/auth.fixture.ts (1)
e2e/utils/pages/login.page.ts (1)
LoginPage(3-36)
e2e/tests/dashboard.spec.ts (3)
e2e/fixtures/auth.fixture.ts (1)
test(9-51)e2e/utils/pages/dashboard.page.ts (1)
DashboardPage(3-47)e2e/utils/helpers.ts (1)
waitForPageLoad(3-5)
e2e/tests/theme-toast.spec.ts (4)
e2e/fixtures/auth.fixture.ts (1)
test(9-51)e2e/utils/pages/settings.page.ts (1)
SettingsPage(3-44)e2e/utils/toast-helpers.ts (8)
triggerToast(3-23)waitForToast(25-33)getToastStyles(35-51)getLuminance(94-98)getContrastRatio(64-87)isLightBackground(53-58)hasGoodContrast(89-92)isDarkBackground(60-62)e2e/utils/reporting.ts (2)
report(9-11)reportAndAnnotate(13-16)
e2e/tests/docker.spec.ts (2)
e2e/fixtures/auth.fixture.ts (2)
test(9-51)expect(53-53)e2e/utils/helpers.ts (2)
waitForPageLoad(3-5)isElementVisible(34-36)
e2e/utils/pages/settings.page.ts (1)
api/src/unraid-api/graph/resolvers/customization/customization.resolver.ts (1)
theme(58-60)
e2e/tests/auth.spec.ts (2)
e2e/fixtures/auth.fixture.ts (1)
test(9-51)e2e/utils/pages/login.page.ts (2)
LoginPage(3-36)isLoggedIn(28-31)
🪛 LanguageTool
e2e/README.md
[grammar] ~107-~107: There might be a mistake here.
Context: ...- Videos:test-results/(on failure) - Traces:test-results/` (on retry) - HTM...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...re) - Traces: test-results/ (on retry) - HTML Report: `pnpm --filter @unraid/e2e ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Bare URL used
(MD034, no-bare-urls)
🪛 dotenv-linter (3.3.0)
e2e/.env.example
[warning] 6-6: [UnorderedKey] The UNRAID_PASSWORD key should go before the UNRAID_USERNAME key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The RETRY_COUNT key should go before the TEST_TIMEOUT key
(UnorderedKey)
[warning] 18-18: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (7)
pnpm-workspace.yaml (1)
8-8: e2e workspace configuration verified e2e/package.json has"private": true, and root-level scripts (test:e2e&playwright:install) correctly reference@unraid/e2e.e2e/utils/pages/settings.page.ts (1)
31-31: Verify theme option values match the appConfirm the select options are literally "white" and "black". If the app uses "light"/"dark" or localized values, this will fail. Consider deriving valid values at runtime.
Example:
const options = await this.themeSelector.locator('option').allTextContents();e2e/utils/pages/login.page.ts (1)
28-31: Login-state check is fragileInferring “logged in” from the absence of username/password inputs may be false-positive on other forms. Prefer a positive signal (e.g., presence of user menu, dashboard header) or a URL predicate.
Do you have a stable selector (e.g., a user menu or dashboard breadcrumb) we can key off? If yes, I can update isLoggedIn() accordingly.
e2e/utils/pages/dashboard.page.ts (1)
3-20: Page Object structure looks goodClear, composable locators and narrow-scoped helpers.
e2e/tests/theme-toast.spec.ts (1)
30-76: Nice accessibility telemetry; keep assertions minimal and actionableThe reporting plus soft expectations are a good balance for visual styling checks.
Also applies to: 106-151
e2e/fixtures/auth.fixture.ts (2)
10-13: loginPage fixture looks goodCreates and exposes the page object cleanly.
2-2: Verify ESM import specifier for TSImporting a TS module with a .js suffix requires tsconfig moduleResolution NodeNext/bundler. If not configured, this will fail. Consider extensionless import.
-import { LoginPage } from '../utils/pages/login.page.js'; +import { LoginPage } from '../utils/pages/login.page';
| if (password) { | ||
| await page.goto('/'); | ||
|
|
There was a problem hiding this comment.
Don’t skip login when password is empty
Gating the whole flow on truthy password breaks default “root with blank password” setups and will silently skip auth.
- if (password) {
- await page.goto('/');
+ await page.goto('/');
@@
- }
- }
+ }Also applies to: 46-47
🤖 Prompt for AI Agents
In e2e/fixtures/auth.fixture.ts around lines 19-21 (and also update the similar
check at lines 46-47): the current conditional uses a truthy check (if
(password)) which skips the login when password is an empty string; change the
condition to explicitly check for undefined (e.g., if (password !== undefined)
or if (typeof password !== 'undefined') ) so the login flow runs when password
is an empty string (blank root password) but is skipped only when password is
truly undefined.
| const usernameInput = page.locator('input[name="username"], input#user').first(); | ||
| const passwordInput = page.locator('input[name="password"], input#pass').first(); | ||
| const submitButton = page.locator('button[type="submit"], input[type="submit"], button:has-text("Login")').first(); | ||
|
|
||
| await usernameInput.fill(username); | ||
| await passwordInput.fill(password); | ||
| await submitButton.click(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize the login sequence (wait for visibility and assert redirect)
Wait for fields, click, then assert we left the login route. This reduces flakiness across CI.
- await usernameInput.fill(username);
- await passwordInput.fill(password);
- await submitButton.click();
+ await usernameInput.waitFor({ state: 'visible' });
+ await passwordInput.waitFor({ state: 'visible' });
+ await submitButton.waitFor({ state: 'attached' });
+ await usernameInput.fill(username);
+ await passwordInput.fill(password);
+ await Promise.all([
+ submitButton.click(),
+ page.waitForLoadState('domcontentloaded'),
+ ]);
@@
- const stillOnLogin = page.url().includes('login');
- if (!stillOnLogin) {
+ await expect(page).not.toHaveURL(/login/i);
+ if (!/login/i.test(page.url())) {
await page.context().storageState({ path: 'auth.json' });
}Also applies to: 38-45
🤖 Prompt for AI Agents
In e2e/fixtures/auth.fixture.ts around lines 30-37 (and similarly apply to
38-45), the login sequence should wait for elements to be visible before
interacting and assert we left the login route after submit to reduce flakiness;
modify the flow to waitFor (or expect.toBeVisible) on usernameInput,
passwordInput and submitButton before fill/click, perform the click, then wait
for navigation or assert the page.url() is not the login path (or
waitForSelector unique to the post-login page) to confirm redirect completed.
| } | ||
| } | ||
|
|
||
| await use(); |
There was a problem hiding this comment.
Call use(undefined) for void fixtures
TypeScript expects one arg even when the fixture type is void; omitting it can cause a compile error.
- await use();
+ await use(undefined);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await use(); | |
| await use(undefined); |
🤖 Prompt for AI Agents
In e2e/fixtures/auth.fixture.ts around line 49, the fixture teardown invokes
use() with no arguments but TypeScript expects an argument for fixtures typed as
void; change the call to use(undefined) so the fixture is called with an
explicit undefined value to satisfy the TypeScript signature and prevent compile
errors.
| reporter: [ | ||
| ['html'], | ||
| ['list'], | ||
| process.env.CI ? ['github'] : null, | ||
| ].filter(Boolean) as any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove "as any" on reporters; use a type-safe conditional instead
The cast hides type issues and violates the TS guideline to avoid assertions. You can avoid filter(Boolean) and keep types intact by building the reporters array conditionally.
Apply:
- reporter: [
- ['html'],
- ['list'],
- process.env.CI ? ['github'] : null,
- ].filter(Boolean) as any,
+ reporter: process.env.CI
+ ? [['html'], ['list'], ['github']]
+ : [['html'], ['list']],📝 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.
| reporter: [ | |
| ['html'], | |
| ['list'], | |
| process.env.CI ? ['github'] : null, | |
| ].filter(Boolean) as any, | |
| reporter: process.env.CI | |
| ? [['html'], ['list'], ['github']] | |
| : [['html'], ['list']], |
🤖 Prompt for AI Agents
In e2e/playwright.config.ts around lines 12 to 16, remove the "as any" cast and
the filter(Boolean) call by constructing the reporters array with proper types:
declare the reporters variable as the correct Playwright ReporterDescription[]
type, include the mandatory ['html'] and ['list'] entries, and conditionally
push ['github'] only when process.env.CI is truthy; then assign that typed
reporters array to the config. This preserves type safety and eliminates the
need for assertions or filtering.
| use: { | ||
| baseURL: process.env.UNRAID_URL || 'http://tower.local', | ||
| trace: 'on-first-retry', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify baseURL and remove unsupported webServer option
- Define baseURL once and reuse it in both "use" and "webServer" to avoid divergence.
- webServer.ignoreHTTPSErrors is not a supported field on the webServer config; HTTPS error handling belongs under "use.ignoreHTTPSErrors" (already present).
Apply:
+const baseURL = process.env.UNRAID_URL || 'http://tower.local';
export default defineConfig({
...
use: {
- baseURL: process.env.UNRAID_URL || 'http://tower.local',
+ baseURL,
ignoreHTTPSErrors: true,
...
},
...
- webServer: process.env.NO_WEB_SERVER ? undefined : {
- command: 'echo "Using external Unraid server"',
- url: process.env.UNRAID_URL || 'http://tower.local',
- reuseExistingServer: true,
- ignoreHTTPSErrors: true,
- },
+ webServer: process.env.NO_WEB_SERVER ? undefined : {
+ // External server expected; we just validate it's reachable.
+ command: 'node -e "console.log(\\"Using external Unraid server\\")"',
+ url: baseURL,
+ reuseExistingServer: true
+ },Also applies to: 66-71
🤖 Prompt for AI Agents
In e2e/playwright.config.ts around lines 17-19 (and also apply to 66-71), the
baseURL is set inline in use but duplicated for webServer and webServer contains
an unsupported ignoreHTTPSErrors field; define a single const baseURL =
process.env.UNRAID_URL || 'http://tower.local' near the top and reference that
const in use.baseURL and in webServer.url (or webServer.launchOptions.args/url
as appropriate), and remove webServer.ignoreHTTPSErrors so ignoreHTTPSErrors
remains only under use.
| async login(username: string, password: string) { | ||
| await this.usernameInput.fill(username); | ||
| await this.passwordInput.fill(password); | ||
| await this.submitButton.click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await post-submit readiness to avoid race conditions
After clicking submit, explicitly wait for navigation or a logged-in UI marker to stabilize tests.
async login(username: string, password: string) {
await this.usernameInput.fill(username);
await this.passwordInput.fill(password);
- await this.submitButton.click();
+ await Promise.all([
+ this.page.waitForNavigation({ waitUntil: 'domcontentloaded' }).catch(() => this.page.waitForURL(/^(?!.*login)/)),
+ this.submitButton.click(),
+ ]);
}📝 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.
| async login(username: string, password: string) { | |
| await this.usernameInput.fill(username); | |
| await this.passwordInput.fill(password); | |
| await this.submitButton.click(); | |
| } | |
| async login(username: string, password: string) { | |
| await this.usernameInput.fill(username); | |
| await this.passwordInput.fill(password); | |
| await Promise.all([ | |
| this.page.waitForNavigation({ waitUntil: 'domcontentloaded' }).catch(() => this.page.waitForURL(/^(?!.*login)/)), | |
| this.submitButton.click(), | |
| ]); | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/pages/login.page.ts around lines 22 to 26, the login method clicks
submit but doesn't wait for post-submit readiness which can cause race
conditions; update the method to await a stable post-login signal (for example,
await Promise.all([page.waitForNavigation({ waitUntil: 'networkidle' }),
this.submitButton.click()]) or await a visible logged-in UI marker like
page.waitForSelector('selector-for-profile-or-dashboard')) so tests only proceed
once navigation or the logged-in UI has stabilized.
| async setTheme(theme: 'white' | 'black') { | ||
| await this.goToDisplaySettings(); | ||
| await this.themeSelector.selectOption(theme); | ||
| await this.applyButton.click(); | ||
| await this.page.waitForLoadState('networkidle'); | ||
| // Wait a bit for theme to apply | ||
| await this.page.waitForTimeout(1_000); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid fixed sleeps; wait for the theme change deterministically
A 1s timeout is flaky across environments. Assert the selected value or wait for a theme-specific DOM signal.
async setTheme(theme: 'white' | 'black') {
await this.goToDisplaySettings();
await this.themeSelector.selectOption(theme);
await this.applyButton.click();
await this.page.waitForLoadState('networkidle');
- // Wait a bit for theme to apply
- await this.page.waitForTimeout(1_000);
+ // Wait until the select reflects the chosen theme
+ await this.page.waitForFunction(
+ (sel, value) => (document.querySelector(sel) as HTMLSelectElement)?.value === value,
+ ['select[name="theme"]', theme],
+ { timeout: 10_000 }
+ );If the app toggles a DOM attribute/class (e.g., html[data-theme]), prefer waiting on that instead.
📝 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.
| async setTheme(theme: 'white' | 'black') { | |
| await this.goToDisplaySettings(); | |
| await this.themeSelector.selectOption(theme); | |
| await this.applyButton.click(); | |
| await this.page.waitForLoadState('networkidle'); | |
| // Wait a bit for theme to apply | |
| await this.page.waitForTimeout(1_000); | |
| } | |
| async setTheme(theme: 'white' | 'black') { | |
| await this.goToDisplaySettings(); | |
| await this.themeSelector.selectOption(theme); | |
| await this.applyButton.click(); | |
| await this.page.waitForLoadState('networkidle'); | |
| // Wait until the select reflects the chosen theme | |
| await this.page.waitForFunction( | |
| (sel, value) => (document.querySelector(sel) as HTMLSelectElement)?.value === value, | |
| ['select[name="theme"]', theme], | |
| { timeout: 10_000 } | |
| ); | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/pages/settings.page.ts around lines 31 to 38, replace the fixed 1s
wait with a deterministic wait that verifies the theme actually applied: after
selecting the option and clicking apply, wait for a reliable DOM signal (for
example waitForSelector or waitForFunction that checks
html[data-theme="white"|"black"] or a specific body/class change) or assert the
themeSelector's selected value, then proceed; remove the
page.waitForTimeout(1_000) and use the explicit wait for the
attribute/class/value instead.
| await page.evaluate(([msg, toastType]) => { | ||
| if ((window as any).toast) { | ||
| if (toastType === 'default') { | ||
| // Simple toast call | ||
| (window as any).toast(msg, { duration: 5000 }); | ||
| } else { | ||
| // Typed toast call (success, info, warning, error) | ||
| (window as any).toast[toastType](msg, { duration: 5000 }); | ||
| } | ||
| } else { | ||
| console.error('Toast function not available'); | ||
| } | ||
| }, [toastMessage, type]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove any and add proper typing for window.toast
Avoid (window as any). Define the toast API type and rely on optional chaining. Also return a boolean to let callers assert availability instead of logging inside the browser context.
-export async function triggerToast(page: Page, type: 'success' | 'info' | 'warning' | 'error' | 'default' = 'default', message?: string) {
+type ToastFn = (msg: string, opts?: { duration?: number }) => void;
+type ToastApi = ToastFn & { success: ToastFn; info: ToastFn; warning: ToastFn; error: ToastFn };
+
+export async function triggerToast(
+ page: Page,
+ type: 'success' | 'info' | 'warning' | 'error' | 'default' = 'default',
+ message?: string
+) {
const toastMessage = message || `Test ${type} notification`;
- // Execute JavaScript to trigger a toast using the Unraid toast system
- await page.evaluate(([msg, toastType]) => {
- if ((window as any).toast) {
- if (toastType === 'default') {
- // Simple toast call
- (window as any).toast(msg, { duration: 5000 });
- } else {
- // Typed toast call (success, info, warning, error)
- (window as any).toast[toastType](msg, { duration: 5000 });
- }
- } else {
- console.error('Toast function not available');
- }
- }, [toastMessage, type]);
+ // Trigger toast via the Unraid toast system
+ const ok = await page.evaluate(
+ ({ msg, toastType }) => {
+ const t = (window as unknown as { toast?: ToastApi }).toast;
+ if (!t) return false;
+ if (toastType === 'default') t(msg, { duration: 5000 });
+ else t[toastType](msg, { duration: 5000 });
+ return true;
+ },
+ { msg: toastMessage, toastType: type }
+ );
+ if (!ok) {
+ throw new Error('window.toast not available');
+ }
// Give the toast a moment to appear
await page.waitForTimeout(500);
}If you prefer zero assertions in helpers, we can drop the throw and let tests decide.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.evaluate(([msg, toastType]) => { | |
| if ((window as any).toast) { | |
| if (toastType === 'default') { | |
| // Simple toast call | |
| (window as any).toast(msg, { duration: 5000 }); | |
| } else { | |
| // Typed toast call (success, info, warning, error) | |
| (window as any).toast[toastType](msg, { duration: 5000 }); | |
| } | |
| } else { | |
| console.error('Toast function not available'); | |
| } | |
| }, [toastMessage, type]); | |
| type ToastFn = (msg: string, opts?: { duration?: number }) => void; | |
| type ToastApi = ToastFn & { success: ToastFn; info: ToastFn; warning: ToastFn; error: ToastFn }; | |
| export async function triggerToast( | |
| page: Page, | |
| type: 'success' | 'info' | 'warning' | 'error' | 'default' = 'default', | |
| message?: string | |
| ) { | |
| const toastMessage = message || `Test ${type} notification`; | |
| // Trigger toast via the Unraid toast system | |
| const ok = await page.evaluate( | |
| ({ msg, toastType }) => { | |
| const t = (window as unknown as { toast?: ToastApi }).toast; | |
| if (!t) return false; | |
| if (toastType === 'default') t(msg, { duration: 5000 }); | |
| else t[toastType](msg, { duration: 5000 }); | |
| return true; | |
| }, | |
| { msg: toastMessage, toastType: type } | |
| ); | |
| if (!ok) { | |
| throw new Error('window.toast not available'); | |
| } | |
| // Give the toast a moment to appear | |
| await page.waitForTimeout(500); | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/toast-helpers.ts around lines 7 to 19, the code uses (window as
any) and logs to console inside the browser context; replace this by declaring a
proper Toast API type and augmenting the Window type (e.g., interface ToastAPI {
(msg: string, opts?: any): void; success?: Function; info?: Function; warning?:
Function; error?: Function } and declare global { interface Window { toast?:
ToastAPI } }), then inside page.evaluate use optional chaining
(window.toast?.(...) or window.toast?.[toastType]?.(...)) instead of any, and
return a boolean indicating whether a toast was invoked (true) or not available
(false) so callers can assert availability instead of logging in the browser
context.
| const styles = await toast.evaluate((el) => { | ||
| const computed = window.getComputedStyle(el); | ||
| return { | ||
| backgroundColor: computed.backgroundColor, | ||
| color: computed.color, | ||
| // Convert rgb to a more readable format | ||
| backgroundRgb: computed.backgroundColor.match(/\d+/g)?.map(Number) || [], | ||
| textRgb: computed.color.match(/\d+/g)?.map(Number) || [] | ||
| }; | ||
| }); | ||
|
|
||
| return styles; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strongly type getToastStyles result and normalize rgb parsing
Add a return type and ensure we only take the first three channels, ignoring alpha if present.
-export async function getToastStyles(page: Page) {
+export interface ToastStyles {
+ backgroundColor: string;
+ color: string;
+ backgroundRgb: number[];
+ textRgb: number[];
+}
+
+export async function getToastStyles(page: Page): Promise<ToastStyles> {
const toast = await waitForToast(page);
// Get computed styles of the toast
- const styles = await toast.evaluate((el) => {
+ const styles = await toast.evaluate<ToastStyles>((el) => {
const computed = window.getComputedStyle(el);
+ const toRgb = (css: string) => (css.match(/\d+/g) || []).slice(0, 3).map(Number);
return {
backgroundColor: computed.backgroundColor,
color: computed.color,
- // Convert rgb to a more readable format
- backgroundRgb: computed.backgroundColor.match(/\d+/g)?.map(Number) || [],
- textRgb: computed.color.match(/\d+/g)?.map(Number) || []
+ backgroundRgb: toRgb(computed.backgroundColor),
+ textRgb: toRgb(computed.color),
};
});📝 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.
| const styles = await toast.evaluate((el) => { | |
| const computed = window.getComputedStyle(el); | |
| return { | |
| backgroundColor: computed.backgroundColor, | |
| color: computed.color, | |
| // Convert rgb to a more readable format | |
| backgroundRgb: computed.backgroundColor.match(/\d+/g)?.map(Number) || [], | |
| textRgb: computed.color.match(/\d+/g)?.map(Number) || [] | |
| }; | |
| }); | |
| return styles; | |
| } | |
| export interface ToastStyles { | |
| backgroundColor: string; | |
| color: string; | |
| backgroundRgb: number[]; | |
| textRgb: number[]; | |
| } | |
| export async function getToastStyles(page: Page): Promise<ToastStyles> { | |
| const toast = await waitForToast(page); | |
| // Get computed styles of the toast | |
| const styles = await toast.evaluate<ToastStyles>((el) => { | |
| const computed = window.getComputedStyle(el); | |
| const toRgb = (css: string) => (css.match(/\d+/g) || []).slice(0, 3).map(Number); | |
| return { | |
| backgroundColor: computed.backgroundColor, | |
| color: computed.color, | |
| backgroundRgb: toRgb(computed.backgroundColor), | |
| textRgb: toRgb(computed.color), | |
| }; | |
| }); | |
| return styles; | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/toast-helpers.ts around lines 39 to 51, the anonymous evaluate
return is untyped and the rgb extraction can include an alpha channel; add a
concrete return interface (e.g., { backgroundColor: string; color: string;
backgroundRgb: number[]; textRgb: number[] }) as the function's return type and
update the rgb parsing to take only the first three channels by using
match(/\d+/g)?.slice(0,3).map(Number) || [], ensuring alpha is ignored and the
arrays are strictly numeric arrays of up to three elements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
e2e/.env.example (4)
12-14: Add HEADLESS to make CI/headed toggles explicitPlaywright defaults can differ locally vs CI. Add a HEADLESS flag so users don’t rely on implicit behavior.
# Browser Configuration SLOW_MO=0 +HEADLESS=true
8-11: Confirm these vars are actually consumed in playwright.config.tsIf TEST_TIMEOUT and RETRY_COUNT aren’t read and parsed (ms vs s, Number vs string), they’ll be confusing no-ops.
Would you like me to wire them into use: { actionTimeout, navigationTimeout }, and retries in the config?
4-6: Nit: satisfy dotenv-linter ordering or disable itThe linter is flagging key order and it’ll keep failing in CI if enabled. Either reorder or ignore.
# Authentication (if needed) -UNRAID_USERNAME=root -UNRAID_PASSWORD= +UNRAID_PASSWORD= +UNRAID_USERNAME=rootAlso add a trailing newline at EOF.
16-17: Clarify TRACE/DEBUG behaviorIf TRACE controls Playwright tracing (on/retain-on-failure/off), document accepted values; boolean-only may be too coarse. Consider TRACE=on-first-retry to match docs, with config mapping.
e2e/README.md (5)
19-23: Add language to fenced block and backticks for valuesFix MD040 and improve readability by marking this as dotenv and using backticks for literals.
-``` -UNRAID_URL=http://tower.local -UNRAID_USERNAME=root -UNRAID_PASSWORD=your_password -``` +```dotenv +UNRAID_URL=http://tower.local +UNRAID_USERNAME=root +UNRAID_PASSWORD=your_password +```
53-61: Add language to directory tree blockSatisfy MD040; use text for the tree.
-``` +```text e2e/ ├── fixtures/ # Test fixtures and setup ├── tests/ # Test specifications ├── utils/ # Helper functions and page objects │ ├── pages/ # Page object models │ └── helpers.ts # Utility functions └── playwright.config.ts--- `117-123`: **Align env table with .env.example (add TEST_TIMEOUT, RETRY_COUNT, TRACE, HEADLESS; avoid bare URL)** Keep docs consistent and fix MD034 by wrapping URL. ```diff | Variable | Description | Default | |----------|-------------|---------| -| UNRAID_URL | Unraid server URL | http://tower.local | +| UNRAID_URL | Unraid server URL | `http://tower.local` | | UNRAID_USERNAME | Username for auth | root | | UNRAID_PASSWORD | Password for auth | - | +| TEST_TIMEOUT | Per-test timeout (ms) | 60000 | +| RETRY_COUNT | Playwright retries | 2 | | SLOW_MO | Slow down actions (ms) | 0 | | DEBUG | Enable debug mode | false | +| TRACE | Playwright trace mode (e.g., `off`, `on-first-retry`, `on`) | false | +| HEADLESS | Run browser headless | true |
41-42: Clarify single-file invocationPlaywright expects a path; passing a bare filename may fail depending on cwd and scripts. Suggest forwarding args with “--”.
-pnpm test auth.spec.ts +pnpm test -- e2e/tests/auth.spec.ts
101-106: Include UNRAID_USERNAME in CI sample or note defaultAvoid surprises if username isn’t root on target environments.
env: UNRAID_URL: ${{ secrets.UNRAID_URL }} + UNRAID_USERNAME: ${{ secrets.UNRAID_USERNAME }} UNRAID_PASSWORD: ${{ secrets.UNRAID_PASSWORD }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
e2e/.env.example(1 hunks)e2e/.gitignore(1 hunks)e2e/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/.gitignore
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
e2e/.env.example
[warning] 6-6: [UnorderedKey] The UNRAID_PASSWORD key should go before the UNRAID_USERNAME key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The RETRY_COUNT key should go before the TEST_TIMEOUT key
(UnorderedKey)
[warning] 17-17: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 LanguageTool
e2e/README.md
[grammar] ~111-~111: There might be a mistake here.
Context: ...- Videos:test-results/(on failure) - Traces:test-results/` (on retry) - HTM...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...re) - Traces: test-results/ (on retry) - HTML Report: `pnpm --filter @unraid/e2e ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
119-119: Bare URL used
(MD034, no-bare-urls)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
e2e/.env.example (2)
5-6: Quiet dotenv-linter: order keys and keep sections tidy.Reordering avoids the warnings and keeps the example clean.
-UNRAID_USERNAME=root -UNRAID_PASSWORD= +UNRAID_PASSWORD= +UNRAID_USERNAME=root @@ -TEST_TIMEOUT=60000 -RETRY_COUNT=2 +RETRY_COUNT=2 +TEST_TIMEOUT=60000Also applies to: 9-10
17-17: Add trailing newline at EOF.Prevents minor lint diffs across tools.
-TRACE=false +TRACE=false +e2e/README.md (6)
19-23: Add a language to the fenced block showing .env contents.Satisfies MD040 and improves rendering.
- ``` + ```dotenv UNRAID_URL=http://tower.local UNRAID_USERNAME=root UNRAID_PASSWORD=your_password ```
53-61: Specify a language for the directory tree block.Silences MD040.
-``` +```text e2e/ ├── fixtures/ # Test fixtures and setup ├── tests/ # Test specifications ├── utils/ # Helper functions and page objects │ ├── pages/ # Page object models │ └── helpers.ts # Utility functions └── playwright.config.ts--- `117-123`: **Avoid a bare URL in the table (MD034) and document all envs used.** Wrap the default URL and consider adding any missing vars (TEST_TIMEOUT, RETRY_COUNT, TRACE) if they are actually used. ```diff -| UNRAID_URL | Unraid server URL | http://tower.local | +| UNRAID_URL | Unraid server URL | `http://tower.local` | @@ | DEBUG | Enable debug mode | false | +| TEST_TIMEOUT | Per-test timeout (ms) | `60000` | +| RETRY_COUNT | Retry failed tests | `2` | +| TRACE | Enable trace on retry | `false` |
101-106: Include UNRAID_USERNAME in CI or note the default.Prevents surprises if the default changes.
env: UNRAID_URL: ${{ secrets.UNRAID_URL }} + UNRAID_USERNAME: ${{ secrets.UNRAID_USERNAME }} # optional; defaults to 'root' UNRAID_PASSWORD: ${{ secrets.UNRAID_PASSWORD }} run: pnpm test:e2e
46-49: Prefer npx for dotenvx to avoid a global install requirement.Keeps setup simpler on CI and local.
-dotenvx run -f .env.dev-server-2 -- pnpm test +npx dotenvx run -f .env.dev-server-2 -- pnpm test
1-114: Minor: add trailing newline at EOF.Keeps diffs clean across tools.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
e2e/.env.example(1 hunks)e2e/.gitignore(1 hunks)e2e/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/.gitignore
🧰 Additional context used
🪛 LanguageTool
e2e/README.md
[grammar] ~111-~111: There might be a mistake here.
Context: ...- Videos:test-results/(on failure) - Traces:test-results/` (on retry) - HTM...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...re) - Traces: test-results/ (on retry) - HTML Report: `pnpm --filter @unraid/e2e ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
119-119: Bare URL used
(MD034, no-bare-urls)
🪛 dotenv-linter (3.3.0)
e2e/.env.example
[warning] 6-6: [UnorderedKey] The UNRAID_PASSWORD key should go before the UNRAID_USERNAME key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The RETRY_COUNT key should go before the TEST_TIMEOUT key
(UnorderedKey)
[warning] 17-17: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
| # Test Configuration | ||
| TEST_TIMEOUT=60000 | ||
| RETRY_COUNT=2 | ||
|
|
||
| # Browser Configuration | ||
| SLOW_MO=0 | ||
|
|
||
| # Debugging | ||
| DEBUG=false |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm these env vars are actually consumed (TEST_TIMEOUT, RETRY_COUNT, TRACE).
If unused, drop them from the example to reduce confusion; if used, document them in README.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scanning for env var usage under e2e/"
rg -nP 'process\.env\.(UNRAID_URL|UNRAID_USERNAME|UNRAID_PASSWORD|TEST_TIMEOUT|RETRY_COUNT|SLOW_MO|DEBUG|TRACE)' e2e || trueLength of output: 880
Remove unused env vars from e2e/.env.example
The following variables aren’t referenced anywhere under e2e/ (TEST_TIMEOUT, RETRY_COUNT, SLOW_MO, DEBUG, TRACE); drop them to keep the example in sync with actual usage.
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 10-10: [UnorderedKey] The RETRY_COUNT key should go before the TEST_TIMEOUT key
(UnorderedKey)
🤖 Prompt for AI Agents
In e2e/.env.example around lines 8 to 16, remove the unused environment
variables TEST_TIMEOUT, RETRY_COUNT, SLOW_MO, DEBUG and TRACE so the example
matches actual e2e usage; delete those variable lines and any related comments
or empty rows, leaving only environment keys that are referenced under e2e/ and
keep the file minimal and in sync.
| ```bash | ||
| # Run all tests | ||
| pnpm test | ||
|
|
||
| # Run tests in headed mode (see browser) | ||
| pnpm test:headed | ||
|
|
||
| # Debug tests interactively | ||
| pnpm test:debug | ||
|
|
||
| # Open Playwright UI | ||
| pnpm test:ui | ||
|
|
||
| # Run specific test file | ||
| pnpm test auth.spec.ts | ||
|
|
||
| # Run tests against different server | ||
| UNRAID_URL=http://192.168.1.100 pnpm test:e2e | ||
|
|
||
| # Run tests against different "profiles" | ||
| # -> Make sure not to track your profiles in git | ||
| dotenvx run -f .env.dev-server-2 -- pnpm test | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify scripts listed here exist in e2e/package.json and match names.
Catches drift between docs and scripts (test, test:e2e, test:headed, test:debug, test:ui, test:report).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pkg="e2e/package.json"
jq -r '.scripts' "$pkg"
echo "Expecting keys: test, test:e2e, test:headed, test:debug, test:ui, test:report"Length of output: 522
Add missing test:e2e script to e2e/package.json or update the README accordingly
The README references pnpm test:e2e, but there is no "test:e2e" entry in e2e/package.json.
🤖 Prompt for AI Agents
In e2e/README.md around lines 27-49, the README mentions a pnpm test:e2e command
that does not exist in e2e/package.json; either add a "test:e2e" script to
e2e/package.json that invokes the project’s Playwright test command (matching
how other scripts like test:test:headed/debug/ui are implemented) or update the
README to reference the actual existing script name (e.g., pnpm test or pnpm
test:e2e:whatever) and include any required env usage; ensure the package.json
script name and README example are consistent and that the script runs the
intended Playwright test runner.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,36 @@ | |||
| import { test as base, TestInfo } from '@playwright/test'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
The best way to fix this unused import is to simply remove TestInfo from the import statement on line 1 of e2e/fixtures/logger.fixture.ts, leaving only the required test as base import. No other changes are necessary, as the rest of the code does not rely on TestInfo directly or indirectly.
| @@ -1,4 +1,4 @@ | ||
| import { test as base, TestInfo } from '@playwright/test'; | ||
| import { test as base } from '@playwright/test'; | ||
| import { TestLogger, interceptConsole } from '../utils/logger'; | ||
|
|
||
| export type LoggerFixtures = { |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
e2e/playwright.config.ts (1)
21-23: Unify baseURL and remove unsupported webServer.ignoreHTTPSErrorsAvoid duplication and fix an invalid field (
ignoreHTTPSErrorsis not supported on webServer; keep it underuseonly).dotenvConfig(); +const baseURL = process.env.UNRAID_URL || 'http://tower.local'; + export default defineConfig({ @@ - use: { - baseURL: process.env.UNRAID_URL || 'http://tower.local', + use: { + baseURL, @@ - webServer: process.env.NO_WEB_SERVER ? undefined : { - command: 'echo "Using external Unraid server"', - url: process.env.UNRAID_URL || 'http://tower.local', - reuseExistingServer: true, - ignoreHTTPSErrors: true, - }, + webServer: process.env.NO_WEB_SERVER ? undefined : { + // With reuseExistingServer=true, Playwright will only run command if URL isn't reachable. + command: 'node -e "console.log(\\"Using external Unraid server\\")"', + url: baseURL, + reuseExistingServer: true + },Also applies to: 70-75
e2e/README.md (1)
73-75: Fix script name: README references pnpm test:e2e which may not existDocs show
pnpm test:e2e, but earlier reviews flagged this script missing. Align docs or add the script.Option A (update README):
-UNRAID_URL=http://192.168.1.100 pnpm test:e2e +UNRAID_URL=http://192.168.1.100 pnpm test @@ - run: pnpm test:e2e + run: pnpm testOption B (add script):
- In e2e/package.json add:
"test:e2e": "playwright test"Also applies to: 191-198
e2e/utils/reporting.ts (1)
4-8: AvoidanyinReport.dataPer TS guidelines in this repo, replace
anywithunknown(or a concrete shape).type Report = { type: "info" | "warning" | "error"; description: string; - data?: Record<string, any>; + data?: Record<string, unknown>; };e2e/fixtures/logger.fixture.ts (1)
1-1: Remove unusedTestInfoimportKeeps the file clean and resolves the CodeQL notice.
-import { test as base, TestInfo } from '@playwright/test'; +import { test as base } from '@playwright/test';e2e/fixtures/auth.fixture.ts (1)
15-56: Don’t gate login on password truthiness; stabilize flow and avoid auth.json write races.Current
if (password)skips auth for blank passwords (e.g., defaultroot), and the click → idle sequence is flaky. Also, writing to a fixedauth.jsonis racy under parallel workers.- authenticatedPage: async ({ page, logger }, use) => { - const username = process.env.UNRAID_USERNAME || 'root'; - const password = process.env.UNRAID_PASSWORD || ''; - - if (password) { - logger.info('Attempting authentication'); - await page.goto('/'); - - // Wait for page to fully load - await page.waitForLoadState('networkidle'); - - // Check if we need to authenticate - const needsAuth = await page.locator('input[name="username"], input[name="password"], input#user, input#pass').count() > 0; - - if (needsAuth) { - logger.debug('Authentication required, filling credentials'); - // Try different selector combinations for username/password fields - const usernameInput = page.locator('input[name="username"], input#user').first(); - const passwordInput = page.locator('input[name="password"], input#pass').first(); - const submitButton = page.locator('button[type="submit"], input[type="submit"], button:has-text("Login")').first(); - - await usernameInput.fill(username); - await passwordInput.fill(password); - await submitButton.click(); - - // Wait for navigation to complete - await page.waitForLoadState('networkidle'); - - // Check if we successfully logged in - const stillOnLogin = page.url().includes('login'); - if (!stillOnLogin) { - logger.info('Authentication successful, saving storage state'); - await page.context().storageState({ path: 'auth.json' }); - } else { - logger.warn('Authentication may have failed - still on login page'); - } - } else { - logger.debug('No authentication needed'); - } - } else { - logger.debug('No password configured, skipping authentication'); - } - - await use(); - }, + authenticatedPage: async ({ page, logger }, use, testInfo) => { + const username = process.env.UNRAID_USERNAME ?? 'root'; + const password = process.env.UNRAID_PASSWORD ?? ''; // allow blank password + + await page.goto('/'); + await page.waitForLoadState('domcontentloaded'); + + const needsAuth = + (await page.locator('input[name="username"], input[name="password"], input#user, input#pass').count()) > 0; + + if (needsAuth) { + logger.info('Authentication required, filling credentials'); + const usernameInput = page.locator('input[name="username"], input#user').first(); + const passwordInput = page.locator('input[name="password"], input#pass').first(); + const submitButton = page.locator('button[type="submit"], input[type="submit"], button:has-text("Login")').first(); + + await usernameInput.waitFor({ state: 'visible' }); + await passwordInput.waitFor({ state: 'visible' }); + await submitButton.waitFor({ state: 'attached' }); + + await usernameInput.fill(username); + await passwordInput.fill(password); + await Promise.all([ + submitButton.click(), + page.waitForLoadState('domcontentloaded'), + ]); + + await expect(page).not.toHaveURL(/login/i); + logger.info('Authentication successful, saving storage state'); + await page.context().storageState({ path: testInfo.outputPath('auth.json') }); + } else { + logger.debug('No authentication needed'); + } + + await use(undefined); + },
🧹 Nitpick comments (12)
e2e/playwright.config.ts (1)
8-11: Parallelism vs single shared server: confirm no state collisionsLocally this will parallelize across many workers; against one external server that can cause flakiness/state bleed. Keep CI at 1 worker (good) but consider gating
fullyParallelbehind an env or defaulting to false.e2e/README.md (2)
19-23: Add languages to fenced code blocks (markdownlint MD040)-``` +```ini UNRAID_URL=http://tower.local @@ -``` +```-``` +```text e2e/ ├── fixtures/ # Test fixtures and setup @@ └── playwright.config.ts -``` +```-``` +```text test-results/logs/ ├── chromium/ @@ └── timestamp.log -``` +```-``` +```text [2025-09-04 15:54:14.774] [INFO] [chromium] [dashboard.spec.ts > Dashboard > should display navigation menu] Found menu item: Main -``` +```Also applies to: 145-154, 218-227, 239-241
266-272: Avoid bare URL in table (markdownlint MD034)-| UNRAID_URL | Unraid server URL | http://tower.local | +| UNRAID_URL | Unraid server URL | `http://tower.local` |e2e/justfile (2)
12-22: Handle env filenames with spaces/newlines robustly in test-all-envs loopUsing word-splitting on
env_filescan break with odd filenames. Prefer null-delimited find + while-read.- env_files=$(find . -maxdepth 1 -name ".env.*" -type f | grep -v ".env.example" | sort) - ... - for env_file in $env_files; do + while IFS= read -r -d '' env_file; do # Extract environment name from filename env_name=$(basename "$env_file" | sed 's/^\.env\.//') @@ - done + done < <(find . -maxdepth 1 -name ".env.*" -type f ! -name ".env.example" -print0 | sort -z)Also applies to: 24-46
71-76: Propagate NO_WEB_SERVER when running tests via dotenvx (keep external server)- TEST_RESULTS_DIR="test-results-$env_name" \ - dotenvx run --env-file="{{env_file}}" -- pnpm test {{args}} + TEST_RESULTS_DIR="test-results-$env_name" \ + NO_WEB_SERVER=1 \ + dotenvx run --env-file="{{env_file}}" -- pnpm test {{args}} @@ - TEST_RESULTS_DIR="test-results-$env_name" \ - dotenvx run --env-file="{{env_file}}" -- pnpm test:headed {{args}} + TEST_RESULTS_DIR="test-results-$env_name" \ + NO_WEB_SERVER=1 \ + dotenvx run --env-file="{{env_file}}" -- pnpm test:headed {{args}}Also applies to: 108-113
e2e/utils/logger.ts (2)
41-69: Fix timestamp filename formatting and avoid shadowing importedtimestampYour current
slice(0, -5)trims too much and leaves a trailing dash in filenames (e.g.,...12-.log). Also, the localtimestampidentifier shadows the imported formatter. Use a clearertimeTag.- const browserName = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase(); - const timestamp = new Date().toISOString().replace(/[:.]/g, '-').slice(0, -5); + const browserSegment = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase(); + const timeTag = new Date().toISOString().replace(/[:.]/g, '-').replace('Z', ''); @@ - const pathParts = [outputDir, 'logs', browserName]; + const pathParts = [outputDir, 'logs', browserSegment]; @@ - return path.join(logDir, `${timestamp}.log`); + return path.join(logDir, `${timeTag}.log`);
89-94: PrefertestInfo.attach()over mutatingattachmentsarrayPlaywright exposes
testInfo.attach(); pushing intoattachmentsworks today but is less future-proof. Making this method async keeps API alignment.- attachToTest(testInfo: TestInfo): void { + async attachToTest(testInfo: TestInfo): Promise<void> { @@ - testInfo.attachments.push({ - name: 'test-logs', - path: this.logFilePath, - contentType: 'text/plain' - }); + await testInfo.attach('test-logs', { + path: this.logFilePath!, + contentType: 'text/plain' + });And call site change in the fixture (see
e2e/fixtures/logger.fixture.tsLines 10-13):- logger.attachToTest(testInfo); + await logger.attachToTest(testInfo);Also applies to: 71-74
e2e/utils/reporting.ts (1)
10-29: Mark unusedtestInfoparam to satisfy noUnusedParameters
reportdoesn’t usetestInfo. Prefix with_to avoid lints without changing the signature.-export function report(testInfo: TestInfo, message: Report, logger?: TestLogger) { +export function report(_testInfo: TestInfo, message: Report, logger?: TestLogger) {e2e/fixtures/logger.fixture.ts (2)
9-16: IfattachToTestbecomes async, await it hereOnly needed if you adopt the optional refactor in the logger to use
testInfo.attach().- const logger = new TestLogger(); - logger.attachToTest(testInfo); + const logger = new TestLogger(); + await logger.attachToTest(testInfo);
23-29: Log errors with richer contextConsider logging each error individually to preserve stacks in CI logs.
- if (testInfo.errors.length > 0) { - logger.error('Test errors:', { errors: testInfo.errors }); - } + for (const err of testInfo.errors) { + logger.error(err); + }e2e/fixtures/auth.fixture.ts (2)
1-1: Confirmexpectexport source (or import directly from Playwright).If
logger.fixturedoesn’t re-exportexpect, this import will fail. Prefer importingexpectstraight from Playwright to decouple fixtures.-import { test as loggerTest, expect, LoggerFixtures } from './logger.fixture'; +import { test as loggerTest, LoggerFixtures } from './logger.fixture'; +import { expect } from '@playwright/test';
4-7: Minor: prefer interface extension over intersection for fixtures.Interface extending
LoggerFixturesis a bit clearer and avoids intersections.-type AuthFixtures = { - loginPage: LoginPage; - authenticatedPage: void; -} & LoggerFixtures; +interface AuthFixtures extends LoggerFixtures { + loginPage: LoginPage; + authenticatedPage: void; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
e2e/.gitignore(1 hunks)e2e/README.md(1 hunks)e2e/fixtures/auth.fixture.ts(1 hunks)e2e/fixtures/logger.fixture.ts(1 hunks)e2e/justfile(1 hunks)e2e/package.json(1 hunks)e2e/playwright.config.ts(1 hunks)e2e/utils/logger.ts(1 hunks)e2e/utils/reporting.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/.gitignore
- e2e/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
e2e/playwright.config.tse2e/fixtures/logger.fixture.tse2e/utils/logger.tse2e/utils/reporting.tse2e/fixtures/auth.fixture.ts
🧠 Learnings (5)
📚 Learning: 2025-02-03T18:57:28.442Z
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Applied to files:
e2e/fixtures/logger.fixture.tse2e/utils/logger.tse2e/fixtures/auth.fixture.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
e2e/README.md
📚 Learning: 2025-03-07T19:29:12.540Z
Learnt from: pujitm
PR: unraid/api#1219
File: api/src/unraid-api/main.ts:18-25
Timestamp: 2025-03-07T19:29:12.540Z
Learning: When handling type mismatches between dependencies in TypeScript (such as between NestJS and Fastify), prefer using `ts-expect-error` with a detailed explanation comment instead of `as any` type assertions. This better documents the reason for the type bypass and makes the issue more visible for future maintenance.
Applied to files:
e2e/utils/reporting.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/**/{store,stores}/**/*.{spec,test}.@(ts|js) : Use factory functions for module mocks to avoid hoisting issues
Applied to files:
e2e/fixtures/auth.fixture.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : Thoroughly test async store actions: assert intermediate loading state, await completion, and check final state
Applied to files:
e2e/fixtures/auth.fixture.ts
🧬 Code graph analysis (3)
e2e/fixtures/logger.fixture.ts (2)
e2e/utils/logger.ts (2)
TestLogger(8-143)interceptConsole(154-205)e2e/fixtures/auth.fixture.ts (1)
test(9-60)
e2e/utils/reporting.ts (1)
e2e/utils/logger.ts (1)
TestLogger(8-143)
e2e/fixtures/auth.fixture.ts (2)
e2e/utils/pages/login.page.ts (1)
LoginPage(3-36)e2e/fixtures/logger.fixture.ts (2)
LoggerFixtures(4-6)test(8-34)
🪛 GitHub Check: CodeQL
e2e/fixtures/logger.fixture.ts
[notice] 1-1: Unused variable, import, function or class
Unused import TestInfo.
🪛 LanguageTool
e2e/README.md
[grammar] ~231-~231: There might be a mistake here.
Context: ...### Log Format Each log entry includes: - Timestamp (millisecond precision) - Log ...
(QB_NEW_EN)
[grammar] ~232-~232: There might be a mistake here.
Context: ...des: - Timestamp (millisecond precision) - Log level (INFO, WARN, ERROR) - Browser ...
(QB_NEW_EN)
[grammar] ~233-~233: There might be a mistake here.
Context: ...ecision) - Log level (INFO, WARN, ERROR) - Browser name - Full test path - Message ...
(QB_NEW_EN)
[grammar] ~234-~234: There might be a mistake here.
Context: ...level (INFO, WARN, ERROR) - Browser name - Full test path - Message and metadata E...
(QB_NEW_EN)
[grammar] ~235-~235: There might be a mistake here.
Context: ..., ERROR) - Browser name - Full test path - Message and metadata Example log entry:...
(QB_NEW_EN)
[grammar] ~259-~259: There might be a mistake here.
Context: ...- Videos:test-results/(on failure) - Traces:test-results/` (on retry) - **L...
(QB_NEW_EN)
[grammar] ~260-~260: There might be a mistake here.
Context: ...re) - Traces: test-results/ (on retry) - **Logs: test-results/logs/ (organized by...
(QB_NEW_EN)
[grammar] ~261-~261: There might be a mistake here.
Context: ...sults/logs/(organized by browser/test)** - HTML Report:pnpm --filter @unraid/e2e ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
239-239: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
e2e/utils/logger.ts (1)
27-37: Confirm console transport level is intentionally limited toerrorOnly errors reach stdout/stderr, which hides info/warn during local debugging. If unintentional, bump to
infowith level-controlled gating viaLOG_LEVEL.e2e/fixtures/auth.fixture.ts (2)
2-2: Verify ESM/TS resolution for.jsextension import.
login.page.tsis TypeScript; importing with.jsrequires NodeNext/Bundler module resolution. If not configured, switch to extensionless import.-import { LoginPage } from '../utils/pages/login.page.js'; +import { LoginPage } from '../utils/pages/login.page';
10-13: LGTM: page object fixture is clean and composable.No issues; good separation of concerns.
| reporter: [ | ||
| ['html', { | ||
| outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', | ||
| open: process.env.PLAYWRIGHT_HTML_OPEN as 'always' | 'never' | 'on-failure' || 'on-failure' | ||
| }], | ||
| ['line'], | ||
| process.env.CI ? ['github'] : null, | ||
| ].filter(Boolean) as any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove "as any" and avoid assertions; build a typed reporters array
This violates the TS guideline to avoid any/type assertions. Construct reporters with proper types and a validated open value.
-import { defineConfig, devices } from '@playwright/test';
+import { defineConfig, devices, type ReporterDescription } from '@playwright/test';
@@
-export default defineConfig({
+const isHtmlOpen = (v: string): v is 'always' | 'never' | 'on-failure' =>
+ v === 'always' || v === 'never' || v === 'on-failure';
+const htmlOpenEnv = (process.env.PLAYWRIGHT_HTML_OPEN ?? 'on-failure').toLowerCase();
+const htmlOpen: 'always' | 'never' | 'on-failure' = isHtmlOpen(htmlOpenEnv) ? htmlOpenEnv : 'on-failure';
+
+const reporters: ReporterDescription[] = process.env.CI
+ ? [['html', { outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', open: htmlOpen }], ['line'], ['github']]
+ : [['html', { outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', open: htmlOpen }], ['line']];
+
+export default defineConfig({
@@
- reporter: [
- ['html', {
- outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report',
- open: process.env.PLAYWRIGHT_HTML_OPEN as 'always' | 'never' | 'on-failure' || 'on-failure'
- }],
- ['line'],
- process.env.CI ? ['github'] : null,
- ].filter(Boolean) as any,
+ reporter: reporters,📝 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.
| reporter: [ | |
| ['html', { | |
| outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', | |
| open: process.env.PLAYWRIGHT_HTML_OPEN as 'always' | 'never' | 'on-failure' || 'on-failure' | |
| }], | |
| ['line'], | |
| process.env.CI ? ['github'] : null, | |
| ].filter(Boolean) as any, | |
| // e2e/playwright.config.ts | |
| import { defineConfig, devices, type ReporterDescription } from '@playwright/test'; | |
| // Ensure the 'open' option is one of the allowed values | |
| const isHtmlOpen = (v: string): v is 'always' | 'never' | 'on-failure' => | |
| v === 'always' || v === 'never' || v === 'on-failure'; | |
| const htmlOpenEnv = (process.env.PLAYWRIGHT_HTML_OPEN ?? 'on-failure').toLowerCase(); | |
| const htmlOpen: 'always' | 'never' | 'on-failure' = | |
| isHtmlOpen(htmlOpenEnv) ? htmlOpenEnv : 'on-failure'; | |
| // Build a properly typed reporters array instead of using `filter(Boolean) as any` | |
| const reporters: ReporterDescription[] = process.env.CI | |
| ? [ | |
| ['html', { | |
| outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', | |
| open: htmlOpen, | |
| }], | |
| ['line'], | |
| ['github'], | |
| ] | |
| : [ | |
| ['html', { | |
| outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', | |
| open: htmlOpen, | |
| }], | |
| ['line'], | |
| ]; | |
| export default defineConfig({ | |
| - reporter: [ | |
| - ['html', { | |
| - outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', | |
| - open: process.env.PLAYWRIGHT_HTML_OPEN as 'always' | 'never' | 'on-failure' || 'on-failure' | |
| - }], | |
| - ['line'], | |
| - process.env.CI ? ['github'] : null, | |
| reporter: reporters, | |
| // ... the rest of your Playwright configuration | |
| }); |
| log(message: string, meta?: any): void { | ||
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| info(message: string, meta?: any): void { | ||
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| warn(message: string, meta?: any): void { | ||
| this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| error(message: string | Error, meta?: any): void { | ||
| const msg = message instanceof Error ? message.message : message; | ||
| const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta; | ||
| this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta }); | ||
| } | ||
|
|
||
| debug(message: string, meta?: any): void { | ||
| this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| verbose(message: string, meta?: any): void { | ||
| this.logger.verbose(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| http(message: string, meta?: any): void { | ||
| this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unsafe any with a safer meta type per TS guidelines
Follow the repo rule “Never use any” for TS/TSX. Narrow meta to a record.
- log(message: string, meta?: any): void {
+ log(message: string, meta?: Record<string, unknown>): void {
@@
- info(message: string, meta?: any): void {
+ info(message: string, meta?: Record<string, unknown>): void {
@@
- warn(message: string, meta?: any): void {
+ warn(message: string, meta?: Record<string, unknown>): void {
@@
- error(message: string | Error, meta?: any): void {
+ error(message: string | Error, meta?: Record<string, unknown>): void {
@@
- debug(message: string, meta?: any): void {
+ debug(message: string, meta?: Record<string, unknown>): void {
@@
- verbose(message: string, meta?: any): void {
+ verbose(message: string, meta?: Record<string, unknown>): void {
@@
- http(message: string, meta?: any): void {
+ http(message: string, meta?: Record<string, unknown>): void {📝 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.
| log(message: string, meta?: any): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| info(message: string, meta?: any): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| warn(message: string, meta?: any): void { | |
| this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| error(message: string | Error, meta?: any): void { | |
| const msg = message instanceof Error ? message.message : message; | |
| const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta; | |
| this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta }); | |
| } | |
| debug(message: string, meta?: any): void { | |
| this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| verbose(message: string, meta?: any): void { | |
| this.logger.verbose(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| http(message: string, meta?: any): void { | |
| this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| log(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| info(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| warn(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| error(message: string | Error, meta?: Record<string, unknown>): void { | |
| const msg = message instanceof Error ? message.message : message; | |
| const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta; | |
| this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta }); | |
| } | |
| debug(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| verbose(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.verbose(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| http(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/logger.ts around lines 114 to 142, the method signatures currently
use unsafe any for meta; change all occurrences of meta?: any to meta?:
Record<string, unknown> (or a specific interface if you have known keys) and
update local variables accordingly (e.g. ensure errorMeta is typed as
Record<string, unknown> | undefined and use object spreads safely). Adjust the
error method so the stack and other properties are merged into a Record<string,
unknown> (casting when necessary) to satisfy TypeScript without any, and run
type checks to fix any resulting type errors.
| export function interceptConsole(logger: TestLogger): () => void { | ||
| const originalConsole = { | ||
| log: console.log, | ||
| info: console.info, | ||
| warn: console.warn, | ||
| error: console.error, | ||
| debug: console.debug, | ||
| }; | ||
|
|
||
| console.log = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.log(message); | ||
| }; | ||
|
|
||
| console.info = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.info(message); | ||
| }; | ||
|
|
||
| console.warn = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.warn(message); | ||
| }; | ||
|
|
||
| console.error = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.error(message); | ||
| }; | ||
|
|
||
| console.debug = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.debug(message); | ||
| }; | ||
|
|
||
| return () => { | ||
| console.log = originalConsole.log; | ||
| console.info = originalConsole.info; | ||
| console.warn = originalConsole.warn; | ||
| console.error = originalConsole.error; | ||
| console.debug = originalConsole.debug; | ||
| }; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Console interception: keep Error stacks, handle BigInt/circulars, and drop any
Right now console.error(new Error(...)) loses the stack (stringified to {}). Also JSON.stringify on bigint throws. Tighten typing and formatting.
-export function interceptConsole(logger: TestLogger): () => void {
+export function interceptConsole(logger: TestLogger): () => void {
@@
- console.log = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ const toStr = (arg: unknown): string => {
+ if (typeof arg === 'bigint') return `${arg}n`;
+ if (arg instanceof Error) return arg.stack || arg.message || String(arg);
+ if (typeof arg === 'object' && arg !== null) {
+ try { return JSON.stringify(arg, null, 2); } catch { return '[Unserializable Object]'; }
+ }
+ return String(arg);
+ };
+
+ console.log = (...args: unknown[]) => {
+ const message = args.map(toStr).join(' ');
logger.log(message);
};
- console.info = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.info = (...args: unknown[]) => {
+ const message = args.map(toStr).join(' ');
logger.info(message);
};
- console.warn = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.warn = (...args: unknown[]) => {
+ const message = args.map(toStr).join(' ');
logger.warn(message);
};
- console.error = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
- logger.error(message);
+ console.error = (...args: unknown[]) => {
+ if (args.length === 1 && args[0] instanceof Error) {
+ logger.error(args[0]);
+ return;
+ }
+ const message = args.map(toStr).join(' ');
+ logger.error(message);
};
- console.debug = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.debug = (...args: unknown[]) => {
+ const message = args.map(toStr).join(' ');
logger.debug(message);
};📝 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.
| export function interceptConsole(logger: TestLogger): () => void { | |
| const originalConsole = { | |
| log: console.log, | |
| info: console.info, | |
| warn: console.warn, | |
| error: console.error, | |
| debug: console.debug, | |
| }; | |
| console.log = (...args: any[]) => { | |
| const message = args.map(arg => | |
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | |
| ).join(' '); | |
| logger.log(message); | |
| }; | |
| console.info = (...args: any[]) => { | |
| const message = args.map(arg => | |
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | |
| ).join(' '); | |
| logger.info(message); | |
| }; | |
| console.warn = (...args: any[]) => { | |
| const message = args.map(arg => | |
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | |
| ).join(' '); | |
| logger.warn(message); | |
| }; | |
| console.error = (...args: any[]) => { | |
| const message = args.map(arg => | |
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | |
| ).join(' '); | |
| logger.error(message); | |
| }; | |
| console.debug = (...args: any[]) => { | |
| const message = args.map(arg => | |
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | |
| ).join(' '); | |
| logger.debug(message); | |
| }; | |
| return () => { | |
| console.log = originalConsole.log; | |
| console.info = originalConsole.info; | |
| console.warn = originalConsole.warn; | |
| console.error = originalConsole.error; | |
| console.debug = originalConsole.debug; | |
| }; | |
| } | |
| export function interceptConsole(logger: TestLogger): () => void { | |
| const originalConsole = { | |
| log: console.log, | |
| info: console.info, | |
| warn: console.warn, | |
| error: console.error, | |
| debug: console.debug, | |
| }; | |
| // Unified serializer: preserves Error stacks, handles bigint, | |
| // and safely falls back on circular/unserializable objects | |
| const toStr = (arg: unknown): string => { | |
| if (typeof arg === 'bigint') { | |
| return `${arg}n`; | |
| } | |
| if (arg instanceof Error) { | |
| return arg.stack || arg.message || String(arg); | |
| } | |
| if (typeof arg === 'object' && arg !== null) { | |
| try { | |
| return JSON.stringify(arg, null, 2); | |
| } catch { | |
| return '[Unserializable Object]'; | |
| } | |
| } | |
| return String(arg); | |
| }; | |
| console.log = (...args: unknown[]) => { | |
| const message = args.map(toStr).join(' '); | |
| logger.log(message); | |
| }; | |
| console.info = (...args: unknown[]) => { | |
| const message = args.map(toStr).join(' '); | |
| logger.info(message); | |
| }; | |
| console.warn = (...args: unknown[]) => { | |
| const message = args.map(toStr).join(' '); | |
| logger.warn(message); | |
| }; | |
| console.error = (...args: unknown[]) => { | |
| // If it’s a lone Error, preserve its full object (stack, etc.) | |
| if (args.length === 1 && args[0] instanceof Error) { | |
| logger.error(args[0]); | |
| return; | |
| } | |
| const message = args.map(toStr).join(' '); | |
| logger.error(message); | |
| }; | |
| console.debug = (...args: unknown[]) => { | |
| const message = args.map(toStr).join(' '); | |
| logger.debug(message); | |
| }; | |
| return () => { | |
| console.log = originalConsole.log; | |
| console.info = originalConsole.info; | |
| console.warn = originalConsole.warn; | |
| console.error = originalConsole.error; | |
| console.debug = originalConsole.debug; | |
| }; | |
| } |
| export function reportAndAnnotate(testInfo: TestInfo, message: Report, logger?: TestLogger) { | ||
| report(testInfo, message, logger); | ||
| testInfo.annotations.push(message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Push a Playwright-compatible annotation shape
TestInfo.annotations expects { type: string; description?: string }. Pushing the whole Report (with data) may not conform to types in some versions.
-export function reportAndAnnotate(testInfo: TestInfo, message: Report, logger?: TestLogger) {
+export function reportAndAnnotate(testInfo: TestInfo, message: Report, logger?: TestLogger) {
report(testInfo, message, logger);
- testInfo.annotations.push(message);
+ testInfo.annotations.push({ type: message.type, description: message.description });
}📝 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.
| export function reportAndAnnotate(testInfo: TestInfo, message: Report, logger?: TestLogger) { | |
| report(testInfo, message, logger); | |
| testInfo.annotations.push(message); | |
| } | |
| export function reportAndAnnotate(testInfo: TestInfo, message: Report, logger?: TestLogger) { | |
| report(testInfo, message, logger); | |
| testInfo.annotations.push({ type: message.type, description: message.description }); | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/reporting.ts around lines 31 to 34, the code pushes the entire
Report object into testInfo.annotations which can include a data field and not
match Playwright's expected annotation shape ({ type: string; description?:
string }); change the push to add only an object with type: message.type and
description: a string (for example message.message or
JSON.stringify(message.data) or a combined string) so the annotation conforms to
Playwright types and retains useful info.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
e2e/playwright.config.ts (2)
12-19: Remove “as any” and filter(Boolean); build a typed reporters arrayThis violates our TS guideline to avoid assertions and using
any. Construct aReporterDescription[]conditionally instead. (Using retrieved learning: avoid type assertions/casting in TS.)Apply:
- reporter: [ - ['html', { - outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', - open: process.env.PLAYWRIGHT_HTML_OPEN as 'always' | 'never' | 'on-failure' || 'on-failure' - }], - ['line'], - process.env.CI ? ['github'] : null, - ].filter(Boolean) as any, + reporter: reporters,Add (outside the selection, near imports/top):
import { defineConfig, devices, type ReporterDescription } from '@playwright/test'; const htmlOpenEnv = process.env.PLAYWRIGHT_HTML_OPEN; const htmlOpen: 'always' | 'never' | 'on-failure' = htmlOpenEnv === 'always' || htmlOpenEnv === 'never' || htmlOpenEnv === 'on-failure' ? htmlOpenEnv : 'on-failure'; const reporters: ReporterDescription[] = [ ['html', { outputFolder: process.env.PLAYWRIGHT_HTML_OUTPUT_DIR || 'playwright-report', open: htmlOpen, }], ['line'], ]; if (process.env.CI) reporters.push(['github']);
21-23: Unify baseURL and remove unsupported webServer.ignoreHTTPSErrors
- Define
baseURLonce and reuse.webServer.ignoreHTTPSErrorsis not a supported option and will type-error.Apply:
+const baseURL = process.env.UNRAID_URL || 'http://tower.local'; export default defineConfig({ ... use: { - baseURL: process.env.UNRAID_URL || 'http://tower.local', + baseURL, trace: 'on-first-retry', screenshot: 'only-on-failure', video: 'retain-on-failure',- webServer: process.env.NO_WEB_SERVER ? undefined : { - command: 'echo "Using external Unraid server"', - url: process.env.UNRAID_URL || 'http://tower.local', - reuseExistingServer: true, - ignoreHTTPSErrors: true, - }, + webServer: process.env.NO_WEB_SERVER ? undefined : { + // External Unraid server expected; just emit a message. + command: 'node -e "console.log(\\"Using external Unraid server\\")"', + url: baseURL, + reuseExistingServer: true, + },Also applies to: 70-75
e2e/fixtures/logger.fixture.ts (1)
1-1: Remove unusedTestInfoimportAvoid unused imports to keep the fixture clean.
Apply this diff:
-import { test as base, TestInfo } from '@playwright/test'; +import { test as base } from '@playwright/test';e2e/fixtures/auth.fixture.ts (2)
58-59: Calluse(undefined)for void fixtureMatches Playwright’s types for a void fixture.
Apply this diff:
- await use(); + await use(undefined);
16-18: Don’t skip login when password is blank; stabilize flow and assert redirectHandle blank root password, wait for visibility, and assert we left the login route. Save storage state into the test’s output dir.
Apply this diff:
- authenticatedPage: async ({ page, logger }, use) => { - const username = process.env.UNRAID_USERNAME || 'root'; - const password = process.env.UNRAID_PASSWORD || ''; + authenticatedPage: async ({ page, logger }, use, testInfo) => { + const username = process.env.UNRAID_USERNAME ?? 'root'; + const password = process.env.UNRAID_PASSWORD ?? ''; @@ - if (password) { + if (typeof password !== 'undefined') { logger.info('Attempting authentication'); - await page.goto('/'); + await page.goto('/', { waitUntil: 'domcontentloaded' }); @@ - await page.waitForLoadState('networkidle'); + await page.waitForLoadState('domcontentloaded'); @@ - await usernameInput.fill(username); - await passwordInput.fill(password); - await submitButton.click(); + await usernameInput.waitFor({ state: 'visible' }); + await passwordInput.waitFor({ state: 'visible' }); + await submitButton.waitFor({ state: 'attached' }); + await usernameInput.fill(username); + await passwordInput.fill(password); + await Promise.all([ + submitButton.click(), + page.waitForLoadState('domcontentloaded'), + ]); @@ - // Check if we successfully logged in - const stillOnLogin = page.url().includes('login'); - if (!stillOnLogin) { + // Check if we successfully logged in + await expect(page).not.toHaveURL(/login/i); + if (!/login/i.test(page.url())) { logger.info('Authentication successful, saving storage state'); - await page.context().storageState({ path: 'auth.json' }); + const authPath = testInfo.outputPath('auth.json'); + await page.context().storageState({ path: authPath }); } else { logger.warn('Authentication may have failed - still on login page'); }Also applies to: 19-56
🧹 Nitpick comments (8)
e2e/playwright.config.ts (1)
21-35: Optional: wire SLOW_MO env to launch optionsMakes local debugging easier while keeping CI fast.
Apply:
use: { baseURL, trace: 'on-first-retry', screenshot: 'only-on-failure', video: 'retain-on-failure', + launchOptions: { + slowMo: Number(process.env.SLOW_MO || 0), + },e2e/README.md (2)
19-23: Add languages to fenced code blocks (markdownlint MD040)Improves readability and fixes lint errors.
Apply:
-``` +```dotenv UNRAID_URL=http://tower.local UNRAID_USERNAME=root UNRAID_PASSWORD=your_password```diff -``` +```text e2e/ ├── fixtures/ # Test fixtures and setup ├── tests/ # Test specifications ├── utils/ # Helper functions and page objects │ ├── pages/ # Page object models │ └── helpers.ts # Utility functions └── playwright.config.ts```diff -``` +```text test-results/logs/ ├── chromium/ ├── firefox/ └── mobile-chrome/ └── test-file/ └── test-suite/ └── test-name/ └── timestamp.log```diff -``` +```text [2025-09-04 15:54:14.774] [INFO] [chromium] [dashboard.spec.ts > Dashboard > should display navigation menu] Found menu item: MainAlso applies to: 145-153, 218-227, 239-241 --- `266-272`: **Wrap bare URL and document NO_WEB_SERVER** Fix MD034 (bare URL) and surface the env used by the config. Apply: ```diff -| UNRAID_URL | Unraid server URL | http://tower.local | +| UNRAID_URL | Unraid server URL | `http://tower.local` | | UNRAID_USERNAME | Username for auth | root | | UNRAID_PASSWORD | Password for auth | - | | SLOW_MO | Slow down actions (ms) | 0 | | DEBUG | Enable debug mode | false | +| NO_WEB_SERVER | Skip starting Playwright webServer (use external Unraid) | false |e2e/justfile (2)
12-14: Use find’s negation instead of piping to grepSlightly simpler and avoids a subshell.
Apply:
- env_files=$(find . -maxdepth 1 -name ".env.*" -type f | grep -v ".env.example" | sort) + env_files=$(find . -maxdepth 1 -name ".env.*" -type f ! -name ".env.example" | sort)
85-87: Same simplification for listing env filesKeep style consistent.
Apply:
- @find . -maxdepth 1 -name ".env.*" -type f | grep -v ".env.example" | sort + @find . -maxdepth 1 -name ".env.*" -type f ! -name ".env.example" | sorte2e/fixtures/logger.fixture.ts (2)
23-29: Be cautious logging rawtestInfo.errorsErrors can include sensitive data. Prefer summarized/serialized errors after redaction (see redaction format in logger.ts).
Apply this diff once redaction is added:
- if (testInfo.errors.length > 0) { - logger.error('Test errors:', { errors: testInfo.errors }); - } + if (testInfo.errors.length > 0) { + logger.error('Test errors', { errors: testInfo.errors.map(e => ({ message: e.message })) }); + }
31-33: Optional: guard console interception behind an env toggleLet CI opt-out if needed.
Apply this diff:
- restoreConsole(); + restoreConsole(); logger.detachFromTest();And wrap interception above:
- const restoreConsole = interceptConsole(logger); + const restoreConsole = process.env.CAPTURE_CONSOLE === 'false' ? () => {} : interceptConsole(logger);e2e/utils/logger.ts (1)
1-7: Aliastimestampformatter and prep for redactionAvoid name shadowing and set up a redaction transform.
Apply this diff:
-import winston from 'winston'; +import winston from 'winston'; import * as path from 'path'; import * as fs from 'fs'; import { TestInfo } from '@playwright/test'; +import { inspect } from 'util'; -const { combine, timestamp, printf, colorize } = winston.format; +const { combine, timestamp: fmtTimestamp, printf, colorize } = winston.format;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
e2e/.gitignore(1 hunks)e2e/README.md(1 hunks)e2e/fixtures/auth.fixture.ts(1 hunks)e2e/fixtures/logger.fixture.ts(1 hunks)e2e/justfile(1 hunks)e2e/package.json(1 hunks)e2e/playwright.config.ts(1 hunks)e2e/utils/logger.ts(1 hunks)e2e/utils/reporting.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/utils/reporting.ts
- e2e/.gitignore
- e2e/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use the any type; prefer proper typing
Avoid using type assertions/casting; prefer proper typing from the start
Files:
e2e/utils/logger.tse2e/fixtures/logger.fixture.tse2e/playwright.config.tse2e/fixtures/auth.fixture.ts
🧠 Learnings (5)
📚 Learning: 2025-02-03T18:57:28.442Z
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Applied to files:
e2e/utils/logger.tse2e/fixtures/logger.fixture.tse2e/fixtures/auth.fixture.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to **/*.{ts,tsx} : Avoid using type assertions/casting; prefer proper typing from the start
Applied to files:
e2e/playwright.config.ts
📚 Learning: 2025-09-02T14:41:56.235Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:41:56.235Z
Learning: Applies to web/**/{store,stores}/**/*.{spec,test}.@(ts|js) : Use factory functions for module mocks to avoid hoisting issues
Applied to files:
e2e/fixtures/auth.fixture.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : Thoroughly test async store actions: assert intermediate loading state, await completion, and check final state
Applied to files:
e2e/fixtures/auth.fixture.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
e2e/README.md
🧬 Code graph analysis (1)
e2e/utils/logger.ts (1)
api/src/core/log.ts (1)
logger(53-83)
🪛 GitHub Check: CodeQL
e2e/fixtures/logger.fixture.ts
[notice] 1-1: Unused variable, import, function or class
Unused import TestInfo.
🪛 LanguageTool
e2e/README.md
[grammar] ~231-~231: There might be a mistake here.
Context: ...### Log Format Each log entry includes: - Timestamp (millisecond precision) - Log ...
(QB_NEW_EN)
[grammar] ~232-~232: There might be a mistake here.
Context: ...des: - Timestamp (millisecond precision) - Log level (INFO, WARN, ERROR) - Browser ...
(QB_NEW_EN)
[grammar] ~233-~233: There might be a mistake here.
Context: ...ecision) - Log level (INFO, WARN, ERROR) - Browser name - Full test path - Message ...
(QB_NEW_EN)
[grammar] ~234-~234: There might be a mistake here.
Context: ...level (INFO, WARN, ERROR) - Browser name - Full test path - Message and metadata E...
(QB_NEW_EN)
[grammar] ~235-~235: There might be a mistake here.
Context: ..., ERROR) - Browser name - Full test path - Message and metadata Example log entry:...
(QB_NEW_EN)
[grammar] ~259-~259: There might be a mistake here.
Context: ...- Videos:test-results/(on failure) - Traces:test-results/` (on retry) - **L...
(QB_NEW_EN)
[grammar] ~260-~260: There might be a mistake here.
Context: ...re) - Traces: test-results/ (on retry) - **Logs: test-results/logs/ (organized by...
(QB_NEW_EN)
[grammar] ~261-~261: There might be a mistake here.
Context: ...sults/logs/(organized by browser/test)** - HTML Report:pnpm --filter @unraid/e2e ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
239-239: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
e2e/README.md (1)
73-75: e2e docs correct —pnpm test:e2eis defined in root package.json (filters@unraid/e2e), so the example is valid.e2e/justfile (1)
56-76: No changes required: scripts exist Verified thattest,test:headed, andplaywright:installare defined in e2e/package.json, matching the justfile commands.e2e/fixtures/auth.fixture.ts (1)
2-2: No changes required
Imports in E2E fixtures must use.jsextensions for ESM compatibility and match existing imports ine2e/tests/*.ts.Likely an incorrect or invalid review comment.
| logger: [async ({ browserName }, use, testInfo) => { | ||
| const logger = new TestLogger(); | ||
| logger.attachToTest(testInfo); | ||
|
|
||
| const restoreConsole = interceptConsole(logger); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make attachToTest async and await it
Switch to testInfo.attach(...) in TestLogger (see logger.ts comment) requires attachToTest to be async; await it here to avoid lost attachment writes.
Apply this diff (paired with logger.ts changes):
- logger: [async ({ browserName }, use, testInfo) => {
+ logger: [async ({ browserName }, use, testInfo) => {
const logger = new TestLogger();
- logger.attachToTest(testInfo);
+ await logger.attachToTest(testInfo);📝 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.
| logger: [async ({ browserName }, use, testInfo) => { | |
| const logger = new TestLogger(); | |
| logger.attachToTest(testInfo); | |
| const restoreConsole = interceptConsole(logger); | |
| logger: [async ({ browserName }, use, testInfo) => { | |
| const logger = new TestLogger(); | |
| await logger.attachToTest(testInfo); | |
| const restoreConsole = interceptConsole(logger); |
🤖 Prompt for AI Agents
In e2e/fixtures/logger.fixture.ts around lines 9-13, attachToTest was changed to
be asynchronous; update this fixture to await the call so attachment writes
aren't lost: mark the lambda as async (already async), call await
logger.attachToTest(testInfo) instead of the current synchronous call, and
ensure any teardown uses the restored console only after the awaited attach
completes; this change should be applied together with the corresponding async
change in logger.ts.
| constructor() { | ||
| this.logger = winston.createLogger({ | ||
| level: process.env.LOG_LEVEL || 'info', | ||
| format: combine( | ||
| timestamp({ format: 'YYYY-MM-DD HH:mm:ss.SSS' }), | ||
| printf(({ level, message, timestamp, testPath, browser, ...meta }) => { | ||
| const testContext = testPath ? `[${testPath}]` : ''; | ||
| const browserContext = browser ? `[${browser}]` : ''; | ||
| const metaStr = Object.keys(meta).length ? ` ${JSON.stringify(meta)}` : ''; | ||
| return `[${timestamp}] [${level.toUpperCase()}] ${browserContext} ${testContext} ${message}${metaStr}`; | ||
| }) | ||
| ), | ||
| transports: [ | ||
| new winston.transports.Console({ | ||
| level: 'error', | ||
| format: combine( | ||
| colorize(), | ||
| printf(({ message, browser }) => { | ||
| const browserPrefix = browser ? `[${browser}]` : ''; | ||
| return `[TEST] ${browserPrefix} ${message}`; | ||
| }) | ||
| ) | ||
| }) | ||
| ] | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add secret redaction, stronger console format, and types
Replace any with typed metadata, add redaction to prevent secrets leakage in CI artifacts, and improve console formatting.
Apply this diff:
constructor() {
this.logger = winston.createLogger({
level: process.env.LOG_LEVEL || 'info',
- format: combine(
- timestamp({ format: 'YYYY-MM-DD HH:mm:ss.SSS' }),
- printf(({ level, message, timestamp, testPath, browser, ...meta }) => {
- const testContext = testPath ? `[${testPath}]` : '';
- const browserContext = browser ? `[${browser}]` : '';
- const metaStr = Object.keys(meta).length ? ` ${JSON.stringify(meta)}` : '';
- return `[${timestamp}] [${level.toUpperCase()}] ${browserContext} ${testContext} ${message}${metaStr}`;
- })
- ),
+ format: combine(
+ fmtTimestamp({ format: 'YYYY-MM-DD HH:mm:ss.SSS' }),
+ // redact sensitive keys in all metadata
+ winston.format((info) => {
+ const redact = (obj: unknown): unknown => {
+ if (!obj || typeof obj !== 'object') return obj;
+ const censor = '***REDACTED***';
+ const keys = ['password','pass','secret','token','key','apikey','accesstoken','idtoken','refreshtoken','localApiKey'];
+ const out: Record<string, unknown> = {};
+ for (const [k, v] of Object.entries(obj as Record<string, unknown>)) {
+ out[k] = keys.some(s => k.toLowerCase().includes(s)) ? censor : (typeof v === 'object' ? redact(v) : v);
+ }
+ return out;
+ };
+ const { testPath, browser, ...meta } = info as Record<string, unknown>;
+ const safeMeta = redact(meta);
+ Object.assign(info, { testPath, browser, meta: safeMeta });
+ return info;
+ })(),
+ printf(({ level, message, timestamp, testPath, browser, meta }) => {
+ const testContext = testPath ? `[${testPath}]` : '';
+ const browserContext = browser ? `[${browser}]` : '';
+ const metaStr = meta && Object.keys(meta).length ? ` ${JSON.stringify(meta)}` : '';
+ return `[${timestamp}] [${String(level).toUpperCase()}] ${browserContext} ${testContext} ${message}${metaStr}`;
+ })
+ ),
transports: [
new winston.transports.Console({
level: 'error',
format: combine(
colorize(),
- printf(({ message, browser }) => {
+ printf(({ message, browser }) => {
const browserPrefix = browser ? `[${browser}]` : '';
return `[TEST] ${browserPrefix} ${message}`;
})
)
})
]
});
}Committable suggestion skipped: line range outside the PR's diff.
| private ensureLogDirectory(testInfo: TestInfo): string { | ||
| const outputDir = process.env.TEST_RESULTS_DIR || 'test-results'; | ||
| const browserName = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase(); | ||
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-').slice(0, -5); | ||
|
|
||
| // Build simple hierarchy: logs/browser/test-file/suite/test-name/ | ||
| const pathParts = [outputDir, 'logs', browserName]; | ||
|
|
||
| // Add test file name | ||
| const testFileName = path.basename(testInfo.file, '.spec.ts') | ||
| .replace(/[^a-z0-9]/gi, '-').toLowerCase(); | ||
| pathParts.push(testFileName); | ||
|
|
||
| // Add test suite hierarchy and test name | ||
| const titleParts = testInfo.titlePath.map(part => | ||
| part.replace(/[^a-z0-9]/gi, '-').toLowerCase() | ||
| ); | ||
| pathParts.push(...titleParts); | ||
|
|
||
| const logDir = path.join(...pathParts); | ||
|
|
||
| if (!fs.existsSync(logDir)) { | ||
| fs.mkdirSync(logDir, { recursive: true }); | ||
| } | ||
|
|
||
| this.browserName = testInfo.project.name; | ||
|
|
||
| return path.join(logDir, `${timestamp}.log`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use testInfo.outputPath and avoid shadowing; create per-test file under Playwright’s output
This ensures artifacts are colocated with the test run and avoids race conditions across workers.
Apply this diff:
- private ensureLogDirectory(testInfo: TestInfo): string {
- const outputDir = process.env.TEST_RESULTS_DIR || 'test-results';
- const browserName = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase();
- const timestamp = new Date().toISOString().replace(/[:.]/g, '-').slice(0, -5);
+ private ensureLogFile(testInfo: TestInfo): string {
+ const browserName = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase();
+ const timeStr = new Date().toISOString().replace(/[:.]/g, '-');
- // Build simple hierarchy: logs/browser/test-file/suite/test-name/
- const pathParts = [outputDir, 'logs', browserName];
+ // logs/<browser>/<test-file>/<suite>/<test-name>/<timestamp>.log
+ const pathParts = ['logs', browserName];
// Add test file name
- const testFileName = path.basename(testInfo.file, '.spec.ts')
+ const testFileName = path.parse(testInfo.file).name
.replace(/[^a-z0-9]/gi, '-').toLowerCase();
pathParts.push(testFileName);
// Add test suite hierarchy and test name
const titleParts = testInfo.titlePath.map(part =>
part.replace(/[^a-z0-9]/gi, '-').toLowerCase()
);
pathParts.push(...titleParts);
- const logDir = path.join(...pathParts);
-
- if (!fs.existsSync(logDir)) {
- fs.mkdirSync(logDir, { recursive: true });
- }
-
this.browserName = testInfo.project.name;
-
- return path.join(logDir, `${timestamp}.log`);
+ const rel = path.join(...pathParts, `${timeStr}.log`);
+ const full = testInfo.outputPath(rel);
+ const dir = path.dirname(full);
+ if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true });
+ return full;
}📝 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.
| private ensureLogDirectory(testInfo: TestInfo): string { | |
| const outputDir = process.env.TEST_RESULTS_DIR || 'test-results'; | |
| const browserName = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase(); | |
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-').slice(0, -5); | |
| // Build simple hierarchy: logs/browser/test-file/suite/test-name/ | |
| const pathParts = [outputDir, 'logs', browserName]; | |
| // Add test file name | |
| const testFileName = path.basename(testInfo.file, '.spec.ts') | |
| .replace(/[^a-z0-9]/gi, '-').toLowerCase(); | |
| pathParts.push(testFileName); | |
| // Add test suite hierarchy and test name | |
| const titleParts = testInfo.titlePath.map(part => | |
| part.replace(/[^a-z0-9]/gi, '-').toLowerCase() | |
| ); | |
| pathParts.push(...titleParts); | |
| const logDir = path.join(...pathParts); | |
| if (!fs.existsSync(logDir)) { | |
| fs.mkdirSync(logDir, { recursive: true }); | |
| } | |
| this.browserName = testInfo.project.name; | |
| return path.join(logDir, `${timestamp}.log`); | |
| } | |
| private ensureLogFile(testInfo: TestInfo): string { | |
| const browserName = testInfo.project.name.replace(/[^a-z0-9]/gi, '-').toLowerCase(); | |
| const timeStr = new Date().toISOString().replace(/[:.]/g, '-'); | |
| // logs/<browser>/<test-file>/<suite>/<test-name>/<timestamp>.log | |
| const pathParts = ['logs', browserName]; | |
| // Add test file name | |
| const testFileName = path.parse(testInfo.file).name | |
| .replace(/[^a-z0-9]/gi, '-').toLowerCase(); | |
| pathParts.push(testFileName); | |
| // Add test suite hierarchy and test name | |
| const titleParts = testInfo.titlePath.map(part => | |
| part.replace(/[^a-z0-9]/gi, '-').toLowerCase() | |
| ); | |
| pathParts.push(...titleParts); | |
| const rel = path.join(...pathParts, `${timeStr}.log`); | |
| const full = testInfo.outputPath(rel); | |
| const dir = path.dirname(full); | |
| if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true }); | |
| this.browserName = testInfo.project.name; | |
| return full; | |
| } |
🤖 Prompt for AI Agents
In e2e/utils/logger.ts around lines 41-69, the current ensureLogDirectory builds
its own TEST_RESULTS_DIR, shadows Playwright output handling and risks races;
change it to use testInfo.outputPath as the base output directory (do not read
process.env.TEST_RESULTS_DIR), keep building the
logs/browser/test-file/suite/name hierarchy under that path, avoid reusing
variable names that shadow external state (e.g., do not overwrite testInfo or
other globals), ensure the directory is created with fs.mkdirSync(..., {
recursive: true }) and set this.browserName without clobbering testInfo, then
return the full per-test log file path inside testInfo.outputPath (e.g.,
path.join(testInfo.outputPath, 'logs', browserName, ..., `${timestamp}.log`)).
| attachToTest(testInfo: TestInfo): void { | ||
| this.testInfo = testInfo; | ||
| this.logFilePath = this.ensureLogDirectory(testInfo); | ||
|
|
||
| const existingFileTransport = this.logger.transports.find( | ||
| t => t instanceof winston.transports.File | ||
| ); | ||
| if (existingFileTransport) { | ||
| this.logger.remove(existingFileTransport); | ||
| } | ||
|
|
||
| this.logger.add( | ||
| new winston.transports.File({ | ||
| filename: this.logFilePath, | ||
| format: this.logger.format | ||
| }) | ||
| ); | ||
|
|
||
| testInfo.attachments.push({ | ||
| name: 'test-logs', | ||
| path: this.logFilePath, | ||
| contentType: 'text/plain' | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use testInfo.attach and make attachToTest async
Directly mutating attachments is brittle; use the API method.
Apply this diff:
- attachToTest(testInfo: TestInfo): void {
+ async attachToTest(testInfo: TestInfo): Promise<void> {
this.testInfo = testInfo;
- this.logFilePath = this.ensureLogDirectory(testInfo);
+ this.logFilePath = this.ensureLogFile(testInfo);
@@
- testInfo.attachments.push({
- name: 'test-logs',
- path: this.logFilePath,
- contentType: 'text/plain'
- });
+ await testInfo.attach('test-logs', {
+ path: this.logFilePath!,
+ contentType: 'text/plain',
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/utils/logger.ts around lines 71 to 94, attachToTest currently mutates
testInfo.attachments directly and is synchronous; change it to async and use the
TestInfo.attach API instead. Keep the existing logic to remove any existing
winston File transport and add a new File transport pointing to the created
logFilePath, then call await testInfo.attach('test-logs', { path:
this.logFilePath, contentType: 'text/plain' }) (or use a stream/buffer as
supported) rather than pushing into testInfo.attachments; ensure the function
signature becomes async and any callers are updated/await the call.
| log(message: string, meta?: any): void { | ||
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| info(message: string, meta?: any): void { | ||
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| warn(message: string, meta?: any): void { | ||
| this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| error(message: string | Error, meta?: any): void { | ||
| const msg = message instanceof Error ? message.message : message; | ||
| const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta; | ||
| this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta }); | ||
| } | ||
|
|
||
| debug(message: string, meta?: any): void { | ||
| this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| verbose(message: string, meta?: any): void { | ||
| this.logger.verbose(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } | ||
|
|
||
| http(message: string, meta?: any): void { | ||
| this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with typed metadata
Comply with our TS guideline to avoid any.
Apply this diff:
- log(message: string, meta?: any): void {
+ log(message: string, meta?: Record<string, unknown>): void {
this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta });
}
- info(message: string, meta?: any): void {
+ info(message: string, meta?: Record<string, unknown>): void {
this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta });
}
- warn(message: string, meta?: any): void {
+ warn(message: string, meta?: Record<string, unknown>): void {
this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta });
}
- error(message: string | Error, meta?: any): void {
+ error(message: string | Error, meta?: Record<string, unknown>): void {
const msg = message instanceof Error ? message.message : message;
const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta;
this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta });
}
- debug(message: string, meta?: any): void {
+ debug(message: string, meta?: Record<string, unknown>): void {
this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta });
}
- verbose(message: string, meta?: any): void {
+ verbose(message: string, meta?: Record<string, unknown>): void {
this.logger.verbose(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta });
}
- http(message: string, meta?: any): void {
+ http(message: string, meta?: Record<string, unknown>): void {
this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta });
}📝 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.
| log(message: string, meta?: any): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| info(message: string, meta?: any): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| warn(message: string, meta?: any): void { | |
| this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| error(message: string | Error, meta?: any): void { | |
| const msg = message instanceof Error ? message.message : message; | |
| const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta; | |
| this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta }); | |
| } | |
| debug(message: string, meta?: any): void { | |
| this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| verbose(message: string, meta?: any): void { | |
| this.logger.verbose(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| http(message: string, meta?: any): void { | |
| this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| log(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| info(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.info(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| warn(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.warn(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| error(message: string | Error, meta?: Record<string, unknown>): void { | |
| const msg = message instanceof Error ? message.message : message; | |
| const errorMeta = message instanceof Error ? { stack: message.stack, ...meta } : meta; | |
| this.logger.error(msg, { testPath: this.getTestPath(), browser: this.browserName, ...errorMeta }); | |
| } | |
| debug(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.debug(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| verbose(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.verbose(message, { testPath: this.getTestTestPath(), browser: this.browserName, ...meta }); | |
| } | |
| http(message: string, meta?: Record<string, unknown>): void { | |
| this.logger.http(message, { testPath: this.getTestPath(), browser: this.browserName, ...meta }); | |
| } |
| export function interceptConsole(logger: TestLogger): () => void { | ||
| const originalConsole = { | ||
| log: console.log, | ||
| info: console.info, | ||
| warn: console.warn, | ||
| error: console.error, | ||
| debug: console.debug, | ||
| }; | ||
|
|
||
| console.log = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.log(message); | ||
| }; | ||
|
|
||
| console.info = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.info(message); | ||
| }; | ||
|
|
||
| console.warn = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.warn(message); | ||
| }; | ||
|
|
||
| console.error = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.error(message); | ||
| }; | ||
|
|
||
| console.debug = (...args: any[]) => { | ||
| const message = args.map(arg => | ||
| typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg) | ||
| ).join(' '); | ||
| logger.debug(message); | ||
| }; | ||
|
|
||
| return () => { | ||
| console.log = originalConsole.log; | ||
| console.info = originalConsole.info; | ||
| console.warn = originalConsole.warn; | ||
| console.error = originalConsole.error; | ||
| console.debug = originalConsole.debug; | ||
| }; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type-safe console interception and robust serialization
Avoid any, handle circular structures with inspect.
Apply this diff:
-export function interceptConsole(logger: TestLogger): () => void {
+export function interceptConsole(logger: TestLogger): () => void {
const originalConsole = {
@@
- console.log = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ const fmt = (args: unknown[]) =>
+ args.map(arg => typeof arg === 'string' ? arg : inspect(arg, { depth: 5, colors: false, compact: false })).join(' ');
+
+ console.log = (...args: unknown[]) => {
+ const message = fmt(args);
logger.log(message);
};
- console.info = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.info = (...args: unknown[]) => {
+ const message = fmt(args);
logger.info(message);
};
- console.warn = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.warn = (...args: unknown[]) => {
+ const message = fmt(args);
logger.warn(message);
};
- console.error = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.error = (...args: unknown[]) => {
+ const message = fmt(args);
logger.error(message);
};
- console.debug = (...args: any[]) => {
- const message = args.map(arg =>
- typeof arg === 'object' ? JSON.stringify(arg, null, 2) : String(arg)
- ).join(' ');
+ console.debug = (...args: unknown[]) => {
+ const message = fmt(args);
logger.debug(message);
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/utils/logger.ts around lines 154 to 205, the console interception uses
`any` and JSON.stringify which can throw on circular structures; replace `any`
with appropriate tuple typing (e.g., (...args: unknown[]) => void) and use
Node's util.inspect to serialize arguments (inspect(arg, { depth: null, colors:
false })) for each arg, joining them with spaces, so circular references are
handled safely; keep the originalConsole capture and restore logic, and ensure
the logger methods are called with the inspected string.
|
Moving to another repo |
2 participants
Summary by CodeRabbit