new frontend e2e tests #86
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralizes Playwright e2e fixtures and adds many frontend E2E suites; replaces contextvar-backed metrics with DI-provided metric objects threaded through providers/services and tests; parallelizes startup and consumer initialization; consolidates CI into a stack-tests workflow; updates docker-compose/deploy/helm and adds frontend lint/type-check tooling. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator as Lifecycle Orchestrator
participant DI as DI Container / Providers
participant Metrics as MetricsProvider
participant Infra as Infra Init (DB/Schema/RateLimits)
participant Services as Consumers & Producers
Orchestrator->>DI: resolve providers & request services
DI->>Metrics: construct metric instances (Event/Execution/DLQ/...)
Metrics-->>DI: return metric instances
DI->>Infra: initialize DB, schema, rate-limits (parallel)
par parallel infra init
Infra-->>Orchestrator: DB/schema ready
Infra-->>Orchestrator: rate-limits ready
end
Orchestrator->>Services: start consumers/producers with injected metrics (async gather)
Services-->>Orchestrator: services started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@frontend/e2e/admin-events.spec.ts`:
- Around line 300-306: The "can manually refresh events" test currently clicks
refreshButton but only calls page.waitForTimeout(500) with no assertion; replace
the arbitrary timeout with a deterministic assertion that the refresh actually
triggered: after clicking refreshButton, await either page.waitForResponse or
page.waitForRequest for the events API endpoint (e.g., the request URL that
loads events) or assert a loading state appears and then disappears (e.g., check
a "loading" role/text via page.getByRole/getByText) and then assert updated
event content is present; remove the waitForTimeout(500) and use refreshButton
and the appropriate network or UI selectors to prove data reloaded.
In `@frontend/e2e/admin-users.spec.ts`:
- Around line 222-231: The test "can open rate limits modal" currently clicks
the rate limits button but makes no assertions; update it to verify the modal
actually opened by waiting for and asserting on a modal/dialog locator (e.g.,
the dialog role or a heading/text like "Rate Limits") and at least one expected
element inside (close button, form field, or save button). Use the same test and
rateLimitsButton locator, then after the click replace the passive wait with a
Playwright expect against a modal locator (e.g., page.locator('role=dialog', {
hasText: 'Rate Limits' }) or a heading locator) to assert visible and optionally
check a specific control inside the modal is present/enabled.
In `@frontend/e2e/editor.spec.ts`:
- Around line 37-52: The test "can select different language" is too permissive
because the final assertion accepts any language; update the verification to
assert that Python was actually selected: after interacting with languageButton,
pythonButton, versionMenu and versionButton, explicitly check that the selected
menuitem/radio reflects Python (e.g., the versionButton or its menuitemradio has
aria-checked="true" or the main languageButton text contains "Python" or the
specific Python version label) and also assert the versionMenu is closed/not
visible; use the existing locators languageButton, pythonButton, versionMenu,
and versionButton to perform these concrete assertions instead of the broad
/python|go|javascript/i pattern.
In `@frontend/e2e/home.spec.ts`:
- Around line 185-197: The test currently swallows missing elements via nested
conditionals and .catch(() => false) which allows a silent pass; update the
'shows footer with privacy link' test to assert the footer and privacy link
exist instead of conditionally checking: use the 'footer' locator and call
expect(footer).toBeVisible({ timeout: 3000 }) unconditionally, then define
'privacyLink' with footer.getByRole('link', { name: /Privacy/i }) and call
expect(privacyLink).toBeVisible({ timeout: 2000 }) so failures surface properly
(remove the .catch fallback and the nested if blocks).
- Around line 153-160: The test "shows mobile menu button on small screens"
clears cookies but not browser storage, which can cause cross-test pollution;
update the test (located by the test name and the calls to page.setViewportSize
and page.context().clearCookies) to also clear localStorage and sessionStorage
before navigating—invoke a storage-clear step (e.g., evaluate a script on the
page context that clears localStorage and sessionStorage) prior to
page.goto('/') so the menuButton locator check runs with a clean storage state.
In `@frontend/e2e/notifications.spec.ts`:
- Around line 68-77: The test "shows mark all as read button when unread
notifications exist" currently silently passes if markAllButton isn't visible;
update the test to assert visibility or fail clearly: ensure test seeds or mocks
unread notifications before running, or replace the conditional branch around
the markAllButton check in the test with a hard assertion (e.g., assert
isVisible or await expect(markAllButton).toBeVisible()) so the test fails when
the button is missing; alternatively, if data-dependence is unavoidable, convert
the test to test.skip() under a clear condition or at minimum log a failing
message when isVisible is false so CI surfaces the skipped state. Include the
same fix pattern for the other interaction tests that use the
isVisible.catch(...) pattern.
- Around line 169-176: The test "can access notifications from header dropdown"
currently only conditionally clicks notificationButton and has no assertions;
update the test to assert the expected result after clicking: locate the
notification dropdown (or expected notification list element) and assert it is
visible and/or contains expected content (e.g., a specific notification text or
a non-empty item count) after clicking notificationButton, and if the button is
not visible fail the test or assert that the header shows the notifications
trigger; reference the existing identifiers headerButtons and notificationButton
to find and click the trigger, then add assertions against the dropdown locator
you choose.
🧹 Nitpick comments (19)
frontend/e2e/editor.spec.ts (3)
3-15: Consider extracting the login helper to a shared fixture.The
loginhelper is duplicated across multiple e2e spec files (admin-sagas.spec.ts,admin-users.spec.ts,admin-settings.spec.ts) with slight variations (admin vs user credentials). Extracting this to a shared test fixture or utility module would improve maintainability and reduce duplication.Example shared fixture approach
Create a shared file like
frontend/e2e/fixtures/auth.ts:import { type Page, expect } from '@playwright/test'; export async function login(page: Page, username = 'user', password = 'user123') { await page.context().clearCookies(); await page.goto('/login'); await page.evaluate(() => { localStorage.clear(); sessionStorage.clear(); }); await page.waitForSelector('#username'); await page.fill('#username', username); await page.fill('#password', password); await page.click('button[type="submit"]'); await expect(page.getByRole('heading', { name: 'Code Editor' })).toBeVisible({ timeout: 10000 }); } export const loginAsAdmin = (page: Page) => login(page, 'admin', 'admin123');
72-76: Replace hardcodedwaitForTimeoutwith condition-based waits.Using
waitForTimeoutis an anti-pattern that leads to flaky tests. The timing may vary across different environments and CI runners. Instead, wait for a specific condition that indicates the action completed.Suggested approach
- await page.waitForTimeout(500); - const content = await page.locator('.cm-content').textContent(); - expect(content).toBeTruthy(); + // Wait for editor content to be populated + await expect(page.locator('.cm-content')).not.toBeEmpty(); + const content = await page.locator('.cm-content').textContent(); + expect(content).toBeTruthy();Similar pattern applies to other
waitForTimeoutcalls throughout this file (lines 84, 118, 131, 147, 164, 184, 201, 218).
78-97: Conditional assertions weaken test coverage.Several tests use the pattern
if (await element.isVisible()) { await expect(element).toBeVisible() }which is tautological - if the condition is true, the assertion always passes; if false, the test silently succeeds without verifying anything. This pattern masks missing UI elements.Consider either:
- Making the assertion unconditional if the element is required
- Using
test.skip()with a descriptive reason when the feature is optional- Using
test.fixme()if the feature should exist but is currently brokenfrontend/e2e/admin-sagas.spec.ts (3)
3-20: Duplicated login helper - consider shared fixture.The
loginAsAdminfunction is identical to the one inadmin-users.spec.tsandadmin-settings.spec.ts. This violates DRY principle.See the earlier comment on
editor.spec.tsfor a suggested shared fixture approach.
67-77: Multiple tests with silent pass on missing elements.The filtering and stats tests use patterns like:
if (isVisible) { await expect(element).toBeVisible(); }If the UI element doesn't exist, these tests pass without verifying anything meaningful. This could mask regressions where features are accidentally removed.
Consider using unconditional assertions or explicitly skipping tests when optional features are absent:
test('shows search input', async ({ page }) => { const searchInput = page.locator('input[placeholder*="Search"], input[type="search"]').first(); // If search is a required feature, assert unconditionally: await expect(searchInput).toBeVisible({ timeout: 3000 }); // Or if optional, skip explicitly: // test.skip(!await searchInput.isVisible(), 'Search feature not available'); });Also applies to: 85-92, 94-101, 103-110
150-162: Excessive hardcoded waits reduce test reliability.The
waitForTimeout(2000)on line 151 (and similar patterns throughout this file) introduces unnecessary delays and potential flakiness. Playwright's auto-waiting should handle most cases.Suggested fix
test('shows sagas table or empty state', async ({ page }) => { - await page.waitForTimeout(2000); - const table = page.locator('table').first(); const emptyState = page.getByText(/No sagas found/i); - const loadingState = page.getByText(/Loading/i); - const hasTable = await table.isVisible({ timeout: 3000 }).catch(() => false); - const hasEmpty = await emptyState.isVisible({ timeout: 3000 }).catch(() => false); - const isLoading = await loadingState.isVisible({ timeout: 1000 }).catch(() => false); - - expect(hasTable || hasEmpty || isLoading).toBe(true); + // Wait for either table or empty state to appear (loading should resolve) + await expect(table.or(emptyState)).toBeVisible({ timeout: 10000 }); });frontend/e2e/admin-users.spec.ts (1)
3-20: Duplicated login helper.Same pattern as other admin spec files. Should be extracted to a shared module.
frontend/e2e/admin-settings.spec.ts (2)
3-20: Duplicated login helper.Same duplication issue as other admin spec files.
219-230: Toast verification could be more robust.The test conditionally checks for a toast, but if toast display is expected behavior after saving, the assertion should be unconditional. If toast implementation is optional/varies, consider documenting this with
test.skip().Suggested improvement
test('can save settings', async ({ page }) => { const saveButton = page.getByRole('button', { name: 'Save Settings' }); await saveButton.click(); - const toast = page.locator('[class*="toast"]').first(); - const hasToast = await toast.isVisible({ timeout: 5000 }).catch(() => false); - - if (hasToast) { - await expect(toast).toBeVisible(); - } + // If toast is expected, assert unconditionally + await expect(page.locator('[class*="toast"]').first()).toBeVisible({ timeout: 5000 }); + // Or verify via URL/page state if toast is unreliable });frontend/e2e/register.spec.ts (1)
103-114: Add cleanup hooks for test user data.The test creates new users during registration (line 107-110) without any teardown mechanism. Multiple test runs will accumulate test users in the database. Consider:
- Adding an
afterEachhook to clean up test users via API or database query- Using a dedicated test database that resets between test suites
- Implementing a test-specific cleanup endpoint
frontend/e2e/notifications.spec.ts (3)
3-15: Consider extracting login helper to a shared utility.This
loginfunction is duplicated across multiple test files (settings.spec.ts,admin-events.spec.ts). Extract it to a shared test utility or Playwright fixture to improve maintainability and reduce code duplication.Example location:
frontend/e2e/utils/auth.tsor use Playwright's global setup for authentication state reuse.
55-56: AvoidwaitForTimeoutin favor of explicit waits.Using
waitForTimeoutis a Playwright anti-pattern that leads to flaky tests. Instead, wait for a specific condition such as network idle or element visibility.♻️ Suggested approach
- await page.waitForTimeout(500); + // Wait for network to be idle after filter action + await page.waitForLoadState('networkidle');
98-107: Redundant assertion pattern.The pattern
if (isVisible) { await expect(severityBadge).toBeVisible(); }is tautological—ifisVisibleistrue, the assertion will always pass. This provides no additional test value.This same pattern repeats at lines 109-118, 120-129, and 131-140.
frontend/e2e/home.spec.ts (1)
95-100: Consider replacingwaitForTimeoutwith explicit condition.For theme toggle verification, wait for the theme class change or visual indicator rather than a fixed timeout.
♻️ Suggested improvement
await themeButton.click(); - await page.waitForTimeout(300); + // Wait for theme transition to complete + await expect(page.locator('html')).toHaveClass(/dark|light/, { timeout: 1000 });frontend/e2e/settings.spec.ts (2)
3-15: Duplicate login helper.Same helper exists in
notifications.spec.tsandadmin-events.spec.ts. Extract to shared utility.
169-183: Consider using role-based locator for toast.The class-based selector
[class*="toast"]is fragile. If the application uses a toast library, consider using its accessible name or role instead.♻️ Suggested improvement
- const toast = page.locator('[class*="toast"]').first(); - await expect(toast).toBeVisible({ timeout: 5000 }); + // Use role if toast has alert role, or status role + await expect(page.getByRole('status').or(page.getByRole('alert'))).toBeVisible({ timeout: 5000 });frontend/e2e/admin-events.spec.ts (3)
3-15: Duplicate admin login helper pattern.Consider creating a shared auth utility that accepts role/credentials as parameters to avoid duplication across test files.
♻️ Example shared utility
// e2e/utils/auth.ts export async function login(page: Page, role: 'user' | 'admin' = 'user') { const credentials = { user: { username: 'user', password: 'user123' }, admin: { username: 'admin', password: 'admin123' }, }; const { username, password } = credentials[role]; // ... rest of login logic }
79-134: ExcessivewaitForTimeoutusage in filtering tests.This section has 5 instances of
waitForTimeout(lines 83, 90, 96, 108, 120). This makes tests slow and potentially flaky. Replace with explicit waits:
- After clicking Filters button: wait for filter panel animation/visibility
- After selecting options: wait for UI to update
♻️ Example improvement for filter panel
test('can toggle filter panel', async ({ page }) => { const filterButton = page.getByRole('button', { name: /Filters/i }); await filterButton.click(); - await page.waitForTimeout(500); - const filterPanel = page.locator('[class*="filter"], [class*="panel"]').filter({ hasText: /Event Type|From|To/i }); - const isExpanded = await filterPanel.first().isVisible({ timeout: 2000 }).catch(() => false); + await expect(filterPanel.first()).toBeVisible({ timeout: 2000 }); + + await filterButton.click(); + await expect(filterPanel.first()).not.toBeVisible({ timeout: 2000 });
171-183: Long fixed waits slow down test suite.Each table test starts with
waitForTimeout(2000). With multiple tests, this adds significant time. Consider:
- Using
waitForLoadState('networkidle')once after navigation- Moving the wait to
beforeEachso it's done once per test group- Waiting for the specific element (table or empty state) directly
♻️ Suggested improvement
test('shows events table or empty state', async ({ page }) => { - await page.waitForTimeout(2000); - const table = page.locator('table').first(); const emptyState = page.getByText(/No events found/i); - const loadingState = page.getByText(/Loading/i); - const hasTable = await table.isVisible({ timeout: 3000 }).catch(() => false); - const hasEmpty = await emptyState.isVisible({ timeout: 3000 }).catch(() => false); - const isLoading = await loadingState.isVisible({ timeout: 1000 }).catch(() => false); - - expect(hasTable || hasEmpty || isLoading).toBe(true); + // Wait for either table or empty state, with longer timeout to account for data loading + await expect(table.or(emptyState)).toBeVisible({ timeout: 10000 }); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/e2e/admin-events.spec.tsfrontend/e2e/admin-sagas.spec.tsfrontend/e2e/admin-settings.spec.tsfrontend/e2e/admin-users.spec.tsfrontend/e2e/editor.spec.tsfrontend/e2e/home.spec.tsfrontend/e2e/notifications.spec.tsfrontend/e2e/register.spec.tsfrontend/e2e/settings.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Scan Backend
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (22)
frontend/e2e/editor.spec.ts (2)
110-174: LGTM!The execution tests properly handle the asynchronous nature of script execution with appropriate timeouts. The assertions for execution state, button disabling, and output visibility are well-structured.
237-250: LGTM!The unauthenticated access control test properly verifies redirect behavior and follows good practices for clearing auth state before testing.
frontend/e2e/register.spec.ts (3)
3-11: LGTM!The
beforeEachhook properly resets browser state (cookies, localStorage, sessionStorage) before each test, ensuring test isolation.
39-51: LGTM!Good use of browser validation API (
validity.valid) to verify HTML5 form validation is working correctly for required fields.
53-75: LGTM!Password validation tests cover important edge cases (mismatch and minimum length) with appropriate selector patterns for error messages.
frontend/e2e/admin-sagas.spec.ts (1)
313-344: LGTM!Access control tests properly verify that non-admin and unauthenticated users are redirected appropriately. The redirect URL pattern matching is correct.
frontend/e2e/admin-users.spec.ts (2)
142-153: LGTM!The create user test properly generates unique usernames and verifies the modal closes after successful creation, indicating the operation completed.
250-281: LGTM!Access control tests properly verify authorization behavior for both non-admin authenticated users and unauthenticated visitors.
frontend/e2e/admin-settings.spec.ts (3)
80-102: Good practice: Tests restore modified values.These tests properly save the original value before modification and restore it afterward. This prevents test pollution and maintains consistent state for subsequent tests.
291-311: LGTM!Navigation tests are well-structured, verifying that sidebar links correctly navigate to other admin pages with appropriate heading assertions.
258-289: LGTM!Access control tests follow the same solid pattern as other admin spec files, properly verifying authorization redirects.
frontend/e2e/notifications.spec.ts (1)
143-156: LGTM!Access control test correctly verifies unauthenticated users are redirected to login.
frontend/e2e/home.spec.ts (3)
3-60: LGTM!The Home Page test suite is well-structured with clear, focused test cases for hero section, welcome message, CTA, and features.
103-137: LGTM!Navigation tests comprehensively cover CTA behavior, header links, and logo navigation with proper URL assertions.
220-257: LGTM!Privacy Page tests have direct, unconditional assertions that will properly fail if content is missing.
frontend/e2e/settings.spec.ts (4)
17-65: LGTM!Settings Page tests properly verify tab structure, theme dropdown options, and theme application with concrete assertions on the
darkclass.
67-124: LGTM!Editor Tab tests comprehensively cover settings visibility, dropdown options, input modifications, and toggle state verification.
126-156: LGTM!Notifications Tab tests properly verify setting toggles with state comparison assertions.
210-241: LGTM!Access control tests thoroughly verify both redirect behavior and redirect target preservation after login—excellent coverage.
frontend/e2e/admin-events.spec.ts (3)
22-55: LGTM!Admin Events Page tests properly verify header, sidebar navigation, active link styling, and action buttons.
137-163: LGTM!Export dropdown tests are concise and properly verify CSV/JSON options.
327-358: LGTM!Access control tests properly verify that both non-admin authenticated users and unauthenticated users are redirected appropriately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
22 issues found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/admin-events.spec.ts">
<violation number="1" location="frontend/e2e/admin-events.spec.ts:67">
P1: This conditional assertion pattern means the test will always pass, even if the element never appears. If the element is optional, consider skipping the test or marking it appropriately. If the element should exist, use `await expect(element).toBeVisible()` directly without the conditional wrapper.</violation>
<violation number="2" location="frontend/e2e/admin-events.spec.ts:83">
P2: Using `waitForTimeout` is discouraged in Playwright tests as it causes flakiness and slowness. Replace with proper wait conditions like `waitForSelector`, `waitForLoadState`, or auto-waiting locators. For example, after clicking a filter button, wait for the panel to appear using `await filterPanel.waitFor()` instead of arbitrary timeouts.</violation>
<violation number="3" location="frontend/e2e/admin-events.spec.ts:150">
P3: The 'can open export dropdown' test already verifies that CSV and JSON options are visible. The separate tests for CSV and JSON options are redundant and increase test execution time without adding value. Consider removing the duplicate tests or consolidating them.</violation>
</file>
<file name="frontend/e2e/notifications.spec.ts">
<violation number="1" location="frontend/e2e/notifications.spec.ts:55">
P2: Using `waitForTimeout` is a Playwright anti-pattern that causes flaky tests. Additionally, this test has no meaningful assertion after clicking the filter button. Consider waiting for a specific UI change (e.g., network idle, element appearance) and adding an assertion for the expected filter result.</violation>
<violation number="2" location="frontend/e2e/notifications.spec.ts:69">
P2: Using `waitForTimeout` causes flaky tests. Instead, wait for a specific condition like an element appearing, network request completing, or using `waitForLoadState`. This anti-pattern appears 7 times in this file.</violation>
<violation number="3" location="frontend/e2e/notifications.spec.ts:105">
P3: Redundant assertion: if `isVisible` is true, then `toBeVisible()` will always pass. Consider either removing the check-and-assert pattern or asserting something more meaningful about the element's content/state.</violation>
<violation number="4" location="frontend/e2e/notifications.spec.ts:174">
P2: Test has no assertions. After clicking the notification button, add an assertion to verify expected behavior (e.g., dropdown appears, navigation occurs). Tests without assertions don't verify any behavior.</violation>
</file>
<file name="frontend/e2e/admin-sagas.spec.ts">
<violation number="1" location="frontend/e2e/admin-sagas.spec.ts:12">
P2: Hardcoded credentials should be moved to environment variables or a test configuration file. This improves security and allows different credentials per environment.</violation>
<violation number="2" location="frontend/e2e/admin-sagas.spec.ts:68">
P2: `waitForTimeout` is an anti-pattern in Playwright that causes flaky tests. Use auto-waiting assertions or explicit wait conditions like `waitForSelector` or `waitForLoadState('networkidle')` instead.</violation>
<violation number="3" location="frontend/e2e/admin-sagas.spec.ts:73">
P1: This test will always pass even if the search input doesn't exist. The conditional `if (isVisible)` causes the assertion to be skipped when the element is missing, making the test useless for catching regressions. Either remove the conditional and let the test fail when the element is missing, or use `test.skip()` with a clear reason if the feature is optional.</violation>
</file>
<file name="frontend/e2e/editor.spec.ts">
<violation number="1" location="frontend/e2e/editor.spec.ts:72">
P2: `waitForTimeout` is a Playwright anti-pattern that leads to flaky tests. Instead of fixed time waits, wait for specific conditions. For example, after clicking the Example button, wait for the editor content to be populated:
```typescript
const content = page.locator('.cm-content');
await expect(content).not.toBeEmpty();
Or use locator.waitFor() to wait for the element's state to change.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@frontend/e2e/admin-users.spec.ts`:
- Around line 23-32: The test 'shows users table with correct columns' currently
skips assertions if desktopTable (page.locator('.table').first()) isn't visible,
making the test ineffective on mobile; update the test to cover both layouts by
removing the conditional and instead detect which table element is present
(desktopTable or mobile/card selector '[class*="card"]') and assert that
'Username', 'Email', 'Role', and 'Status' are visible in the discovered
container; use the existing desktopTable locator and a mobile/card locator,
check visibility for each and run the same expect assertions against the visible
container (or fail if neither is visible) so the assertions always run
regardless of viewport.
In `@frontend/e2e/editor.spec.ts`:
- Around line 87-97: The test 'shows resource usage after execution' can
silently pass because the CPU/Memory assertions are guarded by a conditional;
change it to assert that the "Resource Usage:" element is visible (use
expect(resourceUsage).toBeVisible({ timeout: 5000 })) and then unconditionally
assert the CPU and Memory labels (expect(page.getByText(/CPU:/)).toBeVisible()
and expect(page.getByText(/Memory:/)).toBeVisible()), or explicitly fail if
resourceUsage is not visible; update the block around the resourceUsage lookup
in the test function so the visibility check is an assertion rather than a
conditional guard.
- Around line 115-123: The test 'can save script when authenticated' calls
expectToastVisible(page) but does not assert its boolean result; update the test
to assert the return value of expectToastVisible (e.g., using expect(await
expectToastVisible(page)).toBeTruthy() or similar) after clicking the "Save
current script" button so failures are caught, referencing the test name and the
expectToastVisible helper to locate the change.
- Around line 76-85: The test "shows execution output on successful run"
currently guards the assertion with a conditional which allows silent passes;
remove the conditional and assert unconditionally that the output section and
the output pre element are visible (replace the if(...) block with awaits:
expect(outputSection).toBeVisible(...) and
expect(page.locator('.output-pre').first()).toBeVisible(...)) so the test fails
when no output appears, or alternatively mark the whole test with
test.skip('reason') if output is not reliably available; update references to
outputSection and '.output-pre' accordingly.
In `@frontend/e2e/fixtures.ts`:
- Around line 108-112: The URL assertion in testNonAdminAccessControl uses a
confusing/malformed regex (/^\/$|\/login/) where the anchors only apply to the
first alternative and the slashes are unclear; update the assertion to reliably
check either exactly "/" or any path containing "/login" by parsing the current
page URL (use page.url() → new URL(...)) and assert that url.pathname === '/' ||
url.pathname.includes('/login'), replacing the existing
expect(page).toHaveURL(...) call in testNonAdminAccessControl.
In `@frontend/e2e/home.spec.ts`:
- Line 118: The locator uses an invalid Playwright selector
`.locator('visible=true')`; update the call on the Locator returned by
page.getByRole('link', { name: 'Login' }) to filter by visibility instead —
e.g., replace the `.locator('visible=true')` usage with `.filter({ visible: true
})` (or use the selector modifier syntax `'selector >> visible=true'`) so the
expression becomes page.getByRole(...).filter({ visible: true }).first() before
asserting toBeVisible().
In `@frontend/e2e/settings.spec.ts`:
- Around line 134-141: The test "can save settings" currently calls
expectToastVisible(page) and ignores its boolean return; capture and assert it
instead so the test fails when no toast appears—e.g., await expect(await
expectToastVisible(page)).toBeTruthy() or const visible = await
expectToastVisible(page); expect(visible).toBe(true); update the assertion in
the test function that uses expectToastVisible to verify its boolean result.
♻️ Duplicate comments (10)
frontend/e2e/editor.spec.ts (2)
22-34: Test assertion is too permissive.This issue was previously flagged. The final assertion accepts any language (
/python|go|javascript/i) even though the test attempts to select Python specifically. If Python selection fails, the test passes regardless.
46-52: ReplacewaitForTimeoutwith condition-based waiting.
waitForTimeoutis a Playwright anti-pattern that leads to flaky tests. This was previously flagged.frontend/e2e/register.spec.ts (1)
60-70: Loading state assertion always passes.This issue was previously flagged. The regex
/Registering|Create Account/matches the default button text, so the test passes even if the loading state never appears.frontend/e2e/admin-users.spec.ts (4)
34-37: ReplacewaitForTimeoutwith explicit waiting.This was previously flagged. Use
waitForSelectoror assertion-based waiting instead.
39-44: AnotherwaitForTimeoutusage.Previously flagged. Replace with condition-based waiting.
90-97: Conditional assertion makes test ineffective.Previously flagged. The test silently passes if the edit button doesn't exist.
99-108: Same conditional assertion issue in edit modal test.This test also silently passes if the edit button isn't visible.
frontend/e2e/admin-settings.spec.ts (1)
118-121: Assert the toast visibility result to ensure save succeeded.
expectToastVisiblereturns a boolean but the result is not asserted. If the toast doesn't appear (save failed), this test passes silently.Suggested fix
test('can save settings', async ({ page }) => { await page.getByRole('button', { name: 'Save Settings' }).click(); - await expectToastVisible(page); + const toastShown = await expectToastVisible(page); + expect(toastShown).toBe(true); });frontend/e2e/home.spec.ts (2)
116-116: AvoidwaitForTimeout- use web-first assertions instead.This is a known anti-pattern in Playwright that leads to flaky tests. The subsequent
toBeVisible()assertion already auto-waits. Remove the explicit timeout.♻️ Suggested fix
await menuButton.click(); - await page.waitForTimeout(300); // Mobile menu Login has different class than desktop - look for the visible one
102-108: Missing storage clearing after viewport change.This test clears cookies but the storage clearing happens inside
clearSessiononly after navigating. However, sinceclearSessionnavigates to/if onabout:blank, and then clears storage, the current flow should work. Consider verifying the test doesn't inherit state from previous tests.
🧹 Nitpick comments (2)
frontend/e2e/fixtures.ts (1)
88-95: Naming suggests assertion but function only returns boolean.The
expectprefix conventionally indicates an assertion that throws on failure. This function returnstrue/falsesilently. Consider renaming towaitForToastorisToastVisible, or change the implementation to throw on timeout.Option 1: Rename to clarify behavior
-export async function expectToastVisible(page: Page, timeout = 5000): Promise<boolean> { +export async function isToastVisible(page: Page, timeout = 5000): Promise<boolean> { try { await page.locator('[class*="toast"]').first().waitFor({ state: 'visible', timeout }); return true; } catch { return false; } }Option 2: Make it a true assertion
-export async function expectToastVisible(page: Page, timeout = 5000): Promise<boolean> { - try { - await page.locator('[class*="toast"]').first().waitFor({ state: 'visible', timeout }); - return true; - } catch { - return false; - } +export async function expectToastVisible(page: Page, timeout = 5000): Promise<void> { + await expect(page.locator('[class*="toast"]').first()).toBeVisible({ timeout }); }frontend/e2e/settings.spec.ts (1)
162-170: Use fixture constants instead of hardcoded credentials.The credentials are hardcoded here but
TEST_USERSconstants exist in fixtures. Using the constants improves maintainability and consistency.♻️ Suggested fix
-import { test, expect, loginAsUser, clearSession, expectToastVisible } from './fixtures'; +import { test, expect, loginAsUser, clearSession, expectToastVisible, TEST_USERS } from './fixtures'; - await page.fill('#username', 'user'); - await page.fill('#password', 'user123'); + await page.fill('#username', TEST_USERS.user.username); + await page.fill('#password', TEST_USERS.user.password);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/e2e/admin-events.spec.tsfrontend/e2e/admin-sagas.spec.tsfrontend/e2e/admin-settings.spec.tsfrontend/e2e/admin-users.spec.tsfrontend/e2e/editor.spec.tsfrontend/e2e/fixtures.tsfrontend/e2e/home.spec.tsfrontend/e2e/notifications.spec.tsfrontend/e2e/register.spec.tsfrontend/e2e/settings.spec.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/e2e/admin-sagas.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/e2e/notifications.spec.ts
- frontend/e2e/admin-events.spec.ts
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/e2e/home.spec.ts (1)
frontend/e2e/fixtures.ts (1)
clearSession(8-18)
frontend/e2e/settings.spec.ts (1)
frontend/e2e/fixtures.ts (4)
expect(114-114)loginAsUser(34-36)expectToastVisible(88-95)clearSession(8-18)
frontend/e2e/admin-users.spec.ts (1)
frontend/e2e/fixtures.ts (6)
navigateToAdminPage(47-54)loginAsAdmin(30-32)expect(114-114)expectAdminSidebar(80-86)loginAsUser(34-36)clearSession(8-18)
frontend/e2e/register.spec.ts (1)
frontend/e2e/fixtures.ts (1)
clearSession(8-18)
frontend/e2e/admin-settings.spec.ts (1)
frontend/e2e/fixtures.ts (6)
navigateToAdminPage(47-54)loginAsAdmin(30-32)expect(114-114)expectAdminSidebar(80-86)expectToastVisible(88-95)clearSession(8-18)
frontend/e2e/editor.spec.ts (1)
frontend/e2e/fixtures.ts (4)
loginAsUser(34-36)expect(114-114)expectToastVisible(88-95)clearSession(8-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Tests
- GitHub Check: Scan Backend
- GitHub Check: Build Frontend
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (20)
frontend/e2e/admin-settings.spec.ts (2)
1-11: Clean test setup using shared fixtures.Good use of the centralized fixtures module for authentication and navigation helpers. The
navigateToSettingswrapper provides a clean abstraction.
146-159: Access control tests are well-structured.Good coverage of both non-admin and unauthenticated user redirection scenarios with appropriate URL pattern matching.
frontend/e2e/editor.spec.ts (1)
142-148: Unauthenticated redirect test is well-implemented.Clean test for access control with proper session clearing and URL assertion.
frontend/e2e/fixtures.ts (2)
97-106:testAccessControlnavigates to/loginbeforetargetPathbut doesn't verify redirect.The function navigates to
/login(line 103) then immediately totargetPath(line 104). The intermediate/loginnavigation seems unnecessary and may mask issues where the app doesn't redirect unauthenticated users properly.Was the intent to test that visiting
/loginfirst (without completing login) and then navigating totargetPathtriggers a redirect? If so, the/loginnavigation should probably be removed to test the actual unauthenticated redirect behavior:export async function testAccessControl( page: Page, targetPath: string, expectedRedirectPattern: RegExp ): Promise<void> { await clearSession(page); - await page.goto('/login'); await page.goto(targetPath); await expect(page).toHaveURL(expectedRedirectPattern); }
1-6: Well-structured shared fixtures module.Good centralization of test utilities with clear separation of concerns. The
TEST_USERSconstant and login helpers provide a clean API for test files.Also applies to: 20-36
frontend/e2e/register.spec.ts (3)
1-7: Clean test setup with proper session isolation.Good use of
clearSessioninbeforeEachto ensure tests start from a clean state.
30-38: Good use of validity API for form validation testing.Using the native HTML5 validity API to verify required field validation is a robust approach.
82-91: Successful registration test creates unique users.Good use of
Date.now()to generate unique usernames, preventing test pollution across runs.frontend/e2e/admin-users.spec.ts (2)
73-81: Create user form test is well-implemented.Good use of unique username generation and proper modal close verification after submission.
111-124: Access control tests are comprehensive.Good coverage of both non-admin and unauthenticated user scenarios with appropriate redirects.
frontend/e2e/home.spec.ts (5)
1-7: LGTM - Clean setup pattern.The import and beforeEach setup correctly uses
clearSessionfrom fixtures before navigation, ensuring a clean test state.
9-36: LGTM - Well-structured content assertions.The Home Page content tests use appropriate Playwright selectors and web-first assertions. Good coverage of hero, welcome message, CTA, and feature cards.
38-62: LGTM - Header tests are concise and focused.Clean assertions for header visibility, logo link, auth buttons, and theme toggle presence.
64-91: LGTM - Navigation flow tests.Good coverage of CTA navigation, header links, and logo return-to-home behavior.
122-143: LGTM - Privacy Page tests.Clean assertions for privacy policy content sections.
frontend/e2e/settings.spec.ts (5)
1-6: LGTM - Clean imports and helper setup.The local
navigateToSettingshelper provides good reusability within this test file.
8-49: LGTM - Settings Page general tab tests.Good coverage of tab visibility, theme selection UI, and theme class verification.
51-93: LGTM - Editor tab tests are well-structured.Clean input manipulation and toggle state verification.
95-122: LGTM - Notifications tab tests.Good coverage of notification type toggles and state changes.
155-160: LGTM - Access control redirect test.Clean verification that unauthenticated users are redirected to login.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
13 issues found across 10 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/admin-settings.spec.ts">
<violation number="1" location="frontend/e2e/admin-settings.spec.ts:120">
P2: Test does not assert on `expectToastVisible` return value. The function returns `true`/`false` instead of throwing, so this test will pass even if no toast appears. Add an assertion: `expect(await expectToastVisible(page)).toBe(true)`.</violation>
</file>
<file name="frontend/e2e/editor.spec.ts">
<violation number="1" location="frontend/e2e/editor.spec.ts:122">
P2: The `expectToastVisible(page)` function returns a boolean but the return value isn't checked here. If the toast never appears, the function will catch the error and return `false`, but the test will still pass. Add an assertion on the return value to ensure the toast actually appeared.</violation>
</file>
<file name="frontend/e2e/admin-sagas.spec.ts">
<violation number="1" location="frontend/e2e/admin-sagas.spec.ts:39">
P1: Test will pass silently if the search input doesn't exist. This defeats the purpose of the test - if the UI element is removed, the test should fail, not pass. Either assert unconditionally or use `test.skip()` with a condition.</violation>
<violation number="2" location="frontend/e2e/admin-sagas.spec.ts:46">
P1: Test will pass silently if the state filter dropdown doesn't exist. A test asserting UI presence should fail when the element is missing, not pass silently.</violation>
<violation number="3" location="frontend/e2e/admin-sagas.spec.ts:96">
P1: Test will pass without verifying toggle functionality if the checkbox doesn't exist. Interaction tests should fail when the interactive element is missing.</violation>
</file>
<file name="frontend/e2e/fixtures.ts">
<violation number="1" location="frontend/e2e/fixtures.ts:22">
P2: Unnecessary navigation to `/login` before navigating to `targetPath`. For testing unauthenticated access control, you should clear the session and go directly to the protected route. The intermediate `/login` navigation doesn't add value and makes the test's intent unclear.</violation>
<violation number="2" location="frontend/e2e/fixtures.ts:111">
P2: Regex has inconsistent anchoring. The first alternative `^/$` is anchored, but `\/login` is not, which could match `/login` appearing anywhere in the URL (e.g., `/api/login-status`). Consider anchoring both alternatives for consistent behavior.</violation>
</file>
<file name="frontend/e2e/notifications.spec.ts">
<violation number="1" location="frontend/e2e/notifications.spec.ts:58">
P2: This test is tautological and will always pass. It checks if a badge is visible, and only if it is, asserts it's visible. If no badge exists, no assertion runs and the test still passes. Consider either removing this test or making it meaningful by asserting a specific expected state (e.g., when notifications exist, badges must be present).</violation>
<violation number="2" location="frontend/e2e/notifications.spec.ts:66">
P2: This test is tautological and will always pass. The conditional assertion checks if an element is visible only to assert it's visible. If no timestamp exists, the test silently passes. This provides false test coverage.</violation>
</file>
<file name="frontend/e2e/admin-events.spec.ts">
<violation number="1" location="frontend/e2e/admin-events.spec.ts:47">
P2: Tautological test pattern: if the visibility check passes, asserting visibility is redundant; if it fails, no assertion runs. Either assert unconditionally or use a different approach to handle optional elements.</violation>
<violation number="2" location="frontend/e2e/admin-events.spec.ts:84">
P2: Tautological test pattern: the conditional check makes the assertion redundant. If the Time column is required, assert it directly; if optional, consider skipping the test or documenting why it's optional.</violation>
<violation number="3" location="frontend/e2e/admin-events.spec.ts:97">
P2: Test has no assertions and will always pass. Consider verifying refresh behavior (e.g., checking for a loading indicator, or that data updated).</violation>
</file>
<file name="frontend/e2e/home.spec.ts">
<violation number="1" location="frontend/e2e/home.spec.ts:118">
P2: Invalid Playwright selector syntax. `.locator('visible=true')` is not a valid selector - it attempts to find child elements matching the literal selector string. Use `.filter({ visible: true })` to filter for visible elements, or simply remove the filter since `.first()` combined with `toBeVisible()` assertion already handles visibility checking.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
4 issues found across 8 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/editor.spec.ts">
<violation number="1" location="frontend/e2e/editor.spec.ts:65">
P2: `test.setTimeout()` at describe block level won't work as expected. Use `test.describe.configure({ timeout: 60000 })` to set timeout for all tests in this describe block.</violation>
</file>
<file name="frontend/src/routes/admin/AdminLayout.svelte">
<violation number="1" location="frontend/src/routes/admin/AdminLayout.svelte:80">
P1: Naming collision: the loop variable `route` shadows the imported `route` action from `@mateothegreat/svelte5-router`. The `use:route` directive on line 84 will receive the loop object instead of the router action, breaking navigation. Rename the loop variable to avoid shadowing (e.g., `adminRoute` or `item`).</violation>
</file>
<file name="frontend/e2e/fixtures.ts">
<violation number="1" location="frontend/e2e/fixtures.ts:61">
P2: Silently swallowing the `networkidle` timeout error can hide real issues and cause flaky tests. If the network doesn't settle, the subsequent heading check may fail intermittently. Consider either removing the `.catch()` to let the timeout propagate, or logging a warning so test failures can be diagnosed.</violation>
<violation number="2" location="frontend/e2e/fixtures.ts:134">
P2: Missing explicit timeout and assertion. The `testUnauthenticated` block uses `expect(page).toHaveURL()` for assertion, but `testNonAdmin` only uses `waitForURL()` which provides less informative failure messages. Consider adding an explicit timeout and a proper assertion for consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/notifications.spec.ts">
<violation number="1" location="frontend/e2e/notifications.spec.ts:38">
P2: Inconsistent heading locator - missing `level: 1`. Other assertions in this file use `{ name: 'Notifications', level: 1 }` to match the specific h1 element. Without `level: 1`, this could match an unintended heading.</violation>
</file>
<file name="frontend/e2e/admin-events.spec.ts">
<violation number="1" location="frontend/e2e/admin-events.spec.ts:44">
P2: Avoid using `page.waitForTimeout()` as it makes tests slow and flaky. Consider waiting for a specific condition like the table or empty state to be visible, or use `waitForLoadState('networkidle')` if waiting for data to load.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/notifications.spec.ts">
<violation number="1" location="frontend/e2e/notifications.spec.ts:56">
P2: Scope the severity badge locator to the notification card so the test verifies badges are actually rendered inside the card, not elsewhere on the page.</violation>
<violation number="2" location="frontend/e2e/notifications.spec.ts:67">
P2: Limit the timestamp lookup to the notification card so the test validates that the card itself renders a timestamp.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/e2e/admin-settings.spec.ts`:
- Around line 48-51: The test "can change log level" mutates the `#log-level`
setting but never restores it; update the test to read and store the original
value from page.locator('#log-level') before selecting 'DEBUG', perform the
assertion, and then restore the original value by selecting it back (mirroring
the pattern used in the "can modify max timeout value" test) so the global state
is not polluted for subsequent tests.
♻️ Duplicate comments (1)
frontend/e2e/admin-settings.spec.ts (1)
58-61:expectToastVisiblereturn value is not asserted.The
expectToastVisiblefunction returns a boolean rather than throwing on failure. Without asserting on the return value, this test will pass even if no toast appears.Suggested fix
test('can save settings', async ({ page }) => { await page.getByRole('button', { name: 'Save Settings' }).click(); - await expectToastVisible(page); + expect(await expectToastVisible(page)).toBe(true); });
🧹 Nitpick comments (1)
frontend/e2e/theme.spec.ts (1)
3-5: Good refactoring to eliminate duplication.Extracting the dark theme check into a reusable helper improves readability and maintainability.
Consider importing
Pageat the top for consistency with common TypeScript conventions:-import { test, expect } from '@playwright/test'; +import { test, expect, Page } from '@playwright/test'; -async function hasDarkTheme(page: import('@playwright/test').Page): Promise<boolean> { +async function hasDarkTheme(page: Page): Promise<boolean> {
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/editor.spec.ts">
<violation number="1" location="frontend/e2e/editor.spec.ts:33">
P2: Avoid forcing the click here; it bypasses actionability checks and can hide real UI regressions. Prefer a normal click after visibility assertions.</violation>
</file>
<file name="frontend/e2e/fixtures.ts">
<violation number="1" location="frontend/e2e/fixtures.ts:95">
P2: The new 15s timeout for execution completion is much shorter than the suite’s 120s allowance and the prior 60s wait, which can cause flaky failures when execution takes longer. Consider restoring the longer timeout for this wait.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="deploy.sh">
<violation number="1" location="deploy.sh:121">
P3: The new --timeout option doesn’t validate that a value follows it. With set -e, a missing value triggers a shift error and aborts the script. Add an explicit missing-value check (in both cmd_dev and cmd_infra) before consuming the timeout argument.</violation>
</file>
<file name="docker-compose.yaml">
<violation number="1" location="docker-compose.yaml:423">
P3: `ENABLE_TRACING=${ENABLE_TRACING:-false}` overrides the `ENABLE_TRACING=true` value in backend/.env (env_file has lower precedence), so tracing is now disabled by default even when the env file enables it. This is easy to miss and makes backend/.env ineffective for tracing. Consider removing this override or updating backend/.env/docs to make the new default explicit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose.yaml (1)
100-106: Guard kubeconfig mounts when the file is missing.If
backend/kubeconfig.yamldoesn’t exist, Docker may create a directory at the mount path and k8s clients will fail. Consider allowing an override path (or adding a preflight check indeploy.sh) and make sure it’s gitignored.🔧 Suggested tweak (allow override path)
- - ./backend/kubeconfig.yaml:/app/kubeconfig.yaml:ro + - ${KUBECONFIG_PATH:-./backend/kubeconfig.yaml}:/app/kubeconfig.yaml:ro(Apply the same change to the worker mounts.)
Also applies to: 459-463, 493-497, 529-533
🤖 Fix all issues with AI agents
In `@deploy.sh`:
- Around line 58-64: Add the missing help text entries for the undocumented
flags by updating the usage output around the existing echo lines for the dev
and infra commands: document the --no-observability flag (used to disable
observability) and the --timeout <secs> flag (used to override health-check wait
timeout) alongside the existing --ci and --wait entries so users see these
options in the dev and infra help blocks; modify the echo statements that print
the "dev [options]" and "infra [options]" sections to include lines describing
"--no-observability" and "--timeout" (and ensure the general usage/help section
shows these flags where appropriate).
🧹 Nitpick comments (1)
.github/workflows/frontend-ci.yml (1)
161-167: Verify runner support fordocker build --build-context; consider buildx.On some runners
--build-contextis only supported via buildx. To avoid CI breakage, consider usingdocker buildx build --loadfor these images.🔧 Buildx-based alternative
- docker build -t integr8scode-coordinator:latest -f backend/workers/Dockerfile.coordinator --build-context base=docker-image://integr8scode-base:latest backend - docker build -t integr8scode-k8s-worker:latest -f backend/workers/Dockerfile.k8s_worker --build-context base=docker-image://integr8scode-base:latest backend - docker build -t integr8scode-pod-monitor:latest -f backend/workers/Dockerfile.pod_monitor --build-context base=docker-image://integr8scode-base:latest backend - docker build -t integr8scode-result-processor:latest -f backend/workers/Dockerfile.result_processor --build-context base=docker-image://integr8scode-base:latest backend - docker build -t integr8scode-saga-orchestrator:latest -f backend/workers/Dockerfile.saga_orchestrator --build-context base=docker-image://integr8scode-base:latest backend + docker buildx build --load -t integr8scode-coordinator:latest -f backend/workers/Dockerfile.coordinator --build-context base=docker-image://integr8scode-base:latest backend + docker buildx build --load -t integr8scode-k8s-worker:latest -f backend/workers/Dockerfile.k8s_worker --build-context base=docker-image://integr8scode-base:latest backend + docker buildx build --load -t integr8scode-pod-monitor:latest -f backend/workers/Dockerfile.pod_monitor --build-context base=docker-image://integr8scode-base:latest backend + docker buildx build --load -t integr8scode-result-processor:latest -f backend/workers/Dockerfile.result_processor --build-context base=docker-image://integr8scode-base:latest backend + docker buildx build --load -t integr8scode-saga-orchestrator:latest -f backend/workers/Dockerfile.saga_orchestrator --build-context base=docker-image://integr8scode-base:latest backend
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/.env.test">
<violation number="1" location="backend/.env.test:2">
P2: Use a dedicated test database name in `.env.test` so test runs don’t share the same Mongo database name as the default environment.</violation>
<violation number="2" location="backend/.env.test:42">
P2: Tracing should stay off in the test environment to avoid exporter retries and external dependencies; set `ENABLE_TRACING` to false (or reintroduce `TESTING=true`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/.env.test`:
- Around line 1-83: Split the current heavy backend/.env.test into a minimal
unit-test file and separate integration/e2e configs: create a lightweight
backend/.env.test that only includes essentials (e.g., PROJECT_NAME,
DATABASE_NAME or an in-memory DB config, SECRET_KEY, ALGORITHM,
ACCESS_TOKEN_EXPIRE_MINUTES, BCRYPT_ROUNDS, LOG_LEVEL, SERVER_HOST) and removes
Kafka/Schema Registry/Jaeger/OTEL and other infra vars (KAFKA_BOOTSTRAP_SERVERS,
SCHEMA_REGISTRY_URL, ENABLE_EVENT_STREAMING, JAEGER_AGENT_HOST,
OTEL_EXPORTER_OTLP_ENDPOINT, MONGODB_URL) into new backend/.env.integration and
backend/.env.e2e files that contain the full-stack values (including KAFKA_*,
SCHEMA_REGISTRY_URL, ENABLE_TRACING, OTEL_*, DLQ_*, etc.); update project
docs/README to note which .env to use for unit vs integration/e2e tests and
adjust CI/test scripts to load the appropriate file for each job.
- Line 68: The OTEL_RESOURCE_ATTRIBUTES in .env.test incorrectly sets
environment=production; update the OTEL_RESOURCE_ATTRIBUTES value to use an
appropriate test label (e.g., environment=test or environment=ci) so test
telemetry is tagged as test/CI instead of production, ensuring the
OTEL_RESOURCE_ATTRIBUTES entry is changed from environment=production to
environment=test.
- Around line 2-8: The DATABASE_NAME in the test env is set to the production
value `integr8scode_db`; change it to a test-specific name such as
`integr8scode_test` (or `integr8scode_test_db`) and update the MONGODB_URL to
reference that test DB (adjust the connection string built from
MONGO_ROOT_USER/MONGO_ROOT_PASSWORD to point to `integr8scode_test`), ensuring
all references in this file (DATABASE_NAME and MONGODB_URL) consistently use the
test database name.
- Around line 19-31: Update the test environment to avoid requiring Kafka for
unit tests by changing ENABLE_EVENT_STREAMING=false in the backend/.env.test and
set EVENT_RETENTION_DAYS=1 (or remove it) so unit tests won't attempt to spin up
Kafka; keep KAFKA_BOOTSTRAP_SERVERS and other KAFKA_* keys but ensure
conftest.py continues to gracefully fallback, and for the single integration
test (test_producer_roundtrip.py) either add an override in
tests/integration/conftest.py to set ENABLE_EVENT_STREAMING=true and
EVENT_RETENTION_DAYS back to 30, or create a separate .env.integration file that
the integration test runner loads.
- Line 3: The .env test file uses shell-style parameter expansion (e.g.,
SECRET_KEY=${SECRET_KEY:-...} and
MONGODB_URL="mongodb://${MONGO_ROOT_USER}:${MONGO_ROOT_PASSWORD}@...") which
pydantic-settings/python-dotenv will not expand; update those entries so Python
loads correct values by either replacing the `${VAR:-default}` forms with
concrete static defaults in the file (e.g., set SECRET_KEY to the actual default
string and expand MONGO_ROOT_USER/MONGO_ROOT_PASSWORD into the MONGODB_URL), or
remove defaults from the .env and define defaults in your pydantic Settings
model using Field(default=...) (change in the Settings class where SECRET_KEY,
MONGODB_URL, MONGO_ROOT_USER, MONGO_ROOT_PASSWORD are declared); alternatively,
add a brief note that this .env.test is for docker-compose only so maintainers
know not to rely on it for local Python runs.
In `@deploy.sh`:
- Around line 124-127: The --timeout handling in both cmd_dev and cmd_infra
currently does an unconditional shift and assigns WAIT_TIMEOUT="$1" which allows
missing values; modify both argument-parsing blocks to first verify that a next
positional argument exists and is not another flag (e.g., empty or starts with
'-') before assigning to WAIT_TIMEOUT, and if the check fails print a clear
error and exit (or set a sane default) so passing `--timeout` without a value
does not leave WAIT_TIMEOUT empty and cause docker-compose errors.
🧹 Nitpick comments (1)
backend/.env.test (1)
41-48: Consider reducing tracing overhead in test environment.The test environment has tracing enabled with 100% sampling rate (
TRACING_SAMPLING_RATE=1.0). While useful for debugging, this adds overhead to every test operation and may slow test execution significantly.Consider one of these approaches:
- Disable tracing by default for tests (enable only when debugging):
-ENABLE_TRACING=true +ENABLE_TRACING=false
- Use minimal sampling (e.g., 1%):
-TRACING_SAMPLING_RATE=1.0 +TRACING_SAMPLING_RATE=0.01
- Keep enabled but document that developers can override with
ENABLE_TRACING=falsefor faster test runs.
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="deploy.sh">
<violation number="1" location="deploy.sh:276">
P2: `docker compose run` executes pytest inside the backend container, but the image/compose mounts don’t include `backend/tests` (or `.env.test`). That means the `tests/integration` and `tests/unit` paths won’t exist in the container, so the test run fails immediately. Mount the tests (and env file) or run pytest on the host as before.</violation>
</file>
<file name=".github/workflows/backend-ci.yml">
<violation number="1" location=".github/workflows/backend-ci.yml:101">
P2: Running pytest inside `docker compose run --rm` writes coverage.xml inside the container’s /app directory, but that file isn’t bind-mounted to the host. The container is removed after the run, so Codecov won’t find backend/coverage.xml. Bind-mount the coverage output (and consider applying the same change to the E2E step).</violation>
<violation number="2" location=".github/workflows/backend-ci.yml:180">
P2: The E2E tests now run inside a container with the kubeconfig mounted at /app/kubeconfig.yaml, but KUBECONFIG is no longer set. Python’s Kubernetes client will fall back to ~/.kube/config, so the tests won’t see the mounted config and will fail to connect to the cluster. Add KUBECONFIG in the container invocation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/operations/cicd.md (1)
265-303: Update or remove local registry section—actual workflows useload: trueinstead.The documentation describes using a
registry:2service withlocalhost:5000for dependent builds, but the current workflows (frontend-ci.yml, docker.yml, backend-ci.yml) do not implement this pattern. They instead useload: trueto load images into the Docker daemon and reference them directly viadocker-image://base:latest. Update this section to document the actual current approach or remove it if no longer applicable.
🤖 Fix all issues with AI agents
In @.github/workflows/backend-ci.yml:
- Around line 165-172: The shell redirect in the command `sudo k3s kubectl
config view --raw > /home/runner/.kube/config` is not executed as root (SC2024);
replace the redirect with a root-capable write using tee: run `sudo k3s kubectl
config view --raw | sudo tee /home/runner/.kube/config >/dev/null` (and also
adjust the subsequent `sed`/chmod if needed to run under sudo), locating the
command by the `k3s kubectl config view --raw > /home/runner/.kube/config`
snippet and the related `sed -i "s/127.0.0.1/${HOST_IP}/g"
/home/runner/.kube/config` and `sudo chmod 644 /home/runner/.kube/config` lines.
♻️ Duplicate comments (2)
deploy.sh (2)
124-127:--timeoutlacks validation for missing value.If
--timeoutis passed without a value (e.g.,./deploy.sh dev --timeout), theshiftconsumes nothing meaningful andWAIT_TIMEOUTbecomes empty or the next flag, causing a confusing docker compose error downstream.🔧 Suggested fix
--timeout) + if [[ -z "${2:-}" || "$2" == -* ]]; then + print_error "--timeout requires a numeric value" + exit 1 + fi shift WAIT_TIMEOUT="$1" ;;
184-187: Same--timeoutvalidation issue incmd_infra.Apply the same validation fix here to prevent empty
WAIT_TIMEOUTwhen the flag is passed without a value.🔧 Suggested fix
--timeout) + if [[ -z "${2:-}" || "$2" == -* ]]; then + print_error "--timeout requires a numeric value" + exit 1 + fi shift WAIT_TIMEOUT="$1" ;;
🧹 Nitpick comments (4)
deploy.sh (2)
105-130: Unknown arguments are silently ignored.The argument parsing loop doesn't handle unknown flags—they're consumed by
shiftwithout any warning. Consider adding a catch-all case to alert users of typos.💡 Suggested enhancement
--timeout) shift WAIT_TIMEOUT="$1" ;; + -*) + print_error "Unknown option: $1" + exit 1 + ;; esac shift done
179-190: Same silent ignore of unknown arguments incmd_infra.For consistency and user feedback, consider adding unknown argument handling here as well.
.github/workflows/backend-ci.yml (1)
92-93: Backend image build lacks GHA caching.The base image uses
docker/build-push-actionwith GHA cache, but the backend image uses plaindocker build. Consider usingbuild-push-actionfor the backend as well to benefit from layer caching.💡 Suggested enhancement
- name: Build backend image uses: docker/build-push-action@v6 with: context: ./backend file: ./backend/Dockerfile load: true tags: integr8scode-backend:latest build-contexts: base=docker-image://base:latest cache-from: type=gha,scope=backend cache-to: type=gha,mode=max,scope=backend.github/workflows/frontend-ci.yml (1)
139-145: Worker images lack GHA caching.These five worker image builds use plain
docker buildwithout GHA layer caching. While they depend on the base image, adding caching could still speed up CI when only non-worker code changes.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="deploy.sh">
<violation number="1" location="deploy.sh:269">
P2: cmd_test starts the stack without waiting for services to be healthy, so pytest can run against a backend that hasn’t finished starting. This can cause flaky failures; pass --wait (or --ci) to ensure the stack is ready before running tests.</violation>
</file>
<file name=".github/actions/k3s-setup/action.yml">
<violation number="1" location=".github/actions/k3s-setup/action.yml:25">
P1: Avoid executing a remote install script via `curl | sh` without pinning and integrity verification; download a specific version and verify its checksum before running to prevent remote code execution risks in CI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/stack-tests.yml:
- Around line 351-361: The workflow caches Playwright browser binaries but
currently skips running the system dependency install when the cache hits
because the "Install Playwright browsers" step (running "npx playwright install
--with-deps chromium") is gated by "if: steps.playwright-cache.outputs.cache-hit
!= 'true'"; change the workflow so system packages are always installed—either
remove the "if" on the existing "Install Playwright browsers" step so it always
runs, or split into two steps: an unconditional step that runs "npx playwright
install-deps" (or "npx playwright install --with-deps" but without skipping) to
ensure libatk/libnss3/etc. are installed, and a separate conditional step that
restores/installs only the browser binaries when cache is missed (keeping "Cache
Playwright browsers" and the cache-hit conditional for the browser-download
work).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/stack-tests.yml:
- Around line 37-43: Update the GitHub Action usage for
actions/download-artifact from `@v6` to `@v7`; locate the step that references
"actions/download-artifact" in the workflow and change its version tag to
"actions/download-artifact@v7" while leaving other actions (e.g.,
actions/checkout@v6, actions/setup-node@v6, actions/upload-artifact@v6,
docker/build-push-action@v6, astral-sh/setup-uv@v7, codecov/codecov-action@v5)
unchanged.
In `@docs/testing/frontend-testing.md`:
- Around line 102-112: The fenced directory tree block in
docs/testing/frontend-testing.md lacks a language tag which triggers MD040;
update the triple-backtick fence to include a language (e.g., text or bash) so
the block becomes ```text (or ```bash) to satisfy markdownlint—locate the
directory-tree fenced block in that file and add the chosen language token
immediately after the opening backticks.
In `@frontend/e2e/auth.setup.ts`:
- Around line 4-13: The storageState calls for USER_AUTH_FILE and
ADMIN_AUTH_FILE fail when the e2e/.auth directory is missing; before calling
page.context().storageState({ path: USER_AUTH_FILE }) and the equivalent for
ADMIN_AUTH_FILE, ensure the directory exists (e.g., create the directory once
using fs.mkdir or fs.promises.mkdir with recursive: true for the dirname of
USER_AUTH_FILE/ADMIN_AUTH_FILE) so storageState can write the file; add this
check/create at the start of the setup blocks (referencing USER_AUTH_FILE,
ADMIN_AUTH_FILE, and the setup('authenticate as user' / admin setup functions)
so the files are saved reliably in fresh checkouts/CI.
♻️ Duplicate comments (1)
frontend/e2e/fixtures.ts (1)
84-94: Execution wait timeout may be too short for CI.10s can be tight for backend execution; consider restoring a longer timeout or making it environment-configurable to avoid flakes.
🔧 Proposed fix
- await expect(success.or(failure).first()).toBeVisible({ timeout: 10000 }); + await expect(success.or(failure).first()).toBeVisible({ timeout: 60000 });
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/e2e/fixtures.ts">
<violation number="1" location="frontend/e2e/fixtures.ts:80">
P2: clearSession can throw on a fresh page (about:blank) because it accesses localStorage/sessionStorage without first navigating to a real origin. This makes expectAuthRequired and similar flows fail before navigation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…mit fixes (turn on/off flag added)
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/core/middlewares/rate_limit.py">
<violation number="1" location="backend/app/core/middlewares/rate_limit.py:62">
P2: This change makes it possible to disable rate limiting via configuration, which weakens the security posture and conflicts with the stated requirement that rate limiting is always enforced. Consider removing the bypass so rate limiting remains mandatory.</violation>
</file>
<file name="backend/.env.test">
<violation number="1" location="backend/.env.test:18">
P2: Setting `RATE_LIMIT_ENABLED=false` in the test env disables the rate-limit middleware, so tests won’t exercise rate limiting and can drift from production behavior. Keep rate limiting enabled in tests to match real behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/core/providers.py`:
- Around line 167-176: There are two providers registering DLQManager in the
same container—MessagingProvider.get_dlq_manager and
DLQProcessorProvider.get_dlq_manager—causing a DI conflict; fix by either
removing the redundant DLQProcessorProvider.get_dlq_manager (if it merely
duplicates the defaults of create_dlq_manager) or differentiate them with
explicit qualifiers/aliases on the `@provide` decorator (e.g.,
`@provide`(scope=Scope.APP, qualifier="dlq_for_processor") on
DLQProcessorProvider.get_dlq_manager and update any consumers to request that
qualifier), making sure all calls that expect the specific instance reference
the chosen qualifier and that create_dlq_processor_container wiring is updated
accordingly.
🧹 Nitpick comments (1)
backend/app/core/providers.py (1)
203-209: Consider removingasyncif not needed.The method is marked
asyncbut doesn'tawaitanything—create_event_storereturnsEventStoredirectly (not a coroutine). If this is intentional for future async initialization, disregard; otherwise, consider making it a regular sync method for clarity.♻️ Suggested simplification
`@provide` - async def get_event_store( - self, schema_registry: SchemaRegistryManager, logger: logging.Logger, event_metrics: EventMetrics - ) -> EventStore: + def get_event_store( + self, schema_registry: SchemaRegistryManager, logger: logging.Logger, event_metrics: EventMetrics + ) -> EventStore: return create_event_store( schema_registry=schema_registry, logger=logger, event_metrics=event_metrics, ttl_days=90 )
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cert-generator/setup-k8s.sh">
<violation number="1" location="cert-generator/setup-k8s.sh:19">
P2: The node-ip parsing assumes the value is quoted. On unquoted k3s configs, this produces a malformed host string (e.g., `node-ip: 10.0.0.5`) and results in an invalid kubeconfig URL. Parse the value without requiring quotes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/.env (1)
11-16: Move K8S_NAMESPACE to satisfy dotenv‑linter ordering.Line 16 is out of order per the linter; moving it above
K8S_POD_CPU_LIMITkeeps lint clean.As per static analysis, please keep key ordering consistent.♻️ Proposed reorder
- K8S_POD_CPU_LIMIT=1000m - K8S_POD_MEMORY_LIMIT=128Mi - K8S_POD_CPU_REQUEST=200m - K8S_POD_MEMORY_REQUEST=128Mi - K8S_POD_EXECUTION_TIMEOUT=5 - K8S_NAMESPACE=integr8scode + K8S_NAMESPACE=integr8scode + K8S_POD_CPU_LIMIT=1000m + K8S_POD_MEMORY_LIMIT=128Mi + K8S_POD_CPU_REQUEST=200m + K8S_POD_MEMORY_REQUEST=128Mi + K8S_POD_EXECUTION_TIMEOUT=5
🤖 Fix all issues with AI agents
In `@cert-generator/setup-k8s.sh`:
- Around line 18-25: The node-ip extraction currently can leave the YAML key in
K3S_HOST_IP and produce an invalid kubeconfig; update the parsing where
K3S_HOST_IP is set (the grep/sed pipeline) to robustly extract only the
IP/address value (strip the "node-ip:" key and any surrounding quotes) — e.g.,
replace the current grep|sed pipeline with a regex or helper that returns just
the value, and apply the same fix to the duplicate parsing later in the script
so the sed that rewrites /etc/rancher/k3s/k3s.yaml always substitutes the host
with a bare IP.
In `@docs/operations/deployment.md`:
- Around line 357-371: Add a prominent data-loss warning immediately before the
steps that delete k3s state (the lines removing /var/lib/rancher/k3s/server/db/
and /var/lib/rancher/k3s/server/tls/), stating that these removals will delete
cluster state, workloads, and configuration and instructing operators to back up
cluster state (etcd snapshots, kubeconfigs, manifests, and any important PV
data) before proceeding; place this warning before the step that deletes the DB
(the "sudo rm -rf /var/lib/rancher/k3s/server/db/" line) and include a brief
suggestion to confirm backups have been taken prior to continuing.
In `@frontend/e2e/home.spec.ts`:
- Around line 4-8: The beforeEach hooks (and some tests) call clearSession(page)
before navigation, but clearSession early-returns when page.url() ===
'about:blank' so storage isn't cleared; update those calls to navigate first
(await page.goto('/'); await page.waitForLoadState('domcontentloaded')) and then
call await clearSession(page) (or modify clearSession to handle about:blank by
forcing a navigation internally), ensuring clearSession actually clears
localStorage/sessionStorage; target the test.beforeEach blocks and individual
tests that call clearSession (referencing clearSession and the test.beforeEach
usages shown).
♻️ Duplicate comments (2)
frontend/e2e/notifications.spec.ts (1)
58-66: Scope badge/timestamp locators to the notification card to avoid false positives.Line 61 and Line 73 search the entire page, so a badge or timestamp elsewhere can satisfy the assertion even if the card doesn’t render it. Scope these to
notificationCardso the test actually verifies the card content.🔧 Suggested fix
- const severityBadge = userPage.locator('[class*="badge"]').filter({ hasText: /low|medium|high|urgent/i }).first(); + const severityBadge = notificationCard.locator('[class*="badge"]').filter({ hasText: /low|medium|high|urgent/i }).first(); @@ - const timeIndicator = userPage.locator('text=/ago|Just now|\\d{1,2}:\\d{2}|\\d{4}-\\d{2}-\\d{2}/').first(); + const timeIndicator = notificationCard.locator('text=/ago|Just now|\\d{1,2}:\\d{2}|\\d{4}-\\d{2}-\\d{2}/').first();Also applies to: 69-76
frontend/e2e/admin-settings.spec.ts (1)
65-69: Assert the toast result fromexpectToastVisible.
expectToastVisiblereturns a boolean; ignoring the return value means this test passes even if the toast never appears.🔧 Suggested fix
- await expectToastVisible(adminPage); + await expect(await expectToastVisible(adminPage)).toBe(true);
🧹 Nitpick comments (1)
docker-compose.yaml (1)
21-35: Consider making/etc/rancher/k3sbind mount optional.Line 25 assumes the host path exists; on non‑k3s machines this can create a root‑owned directory or fail with permissions. Consider gating it behind a profile/override compose file or making it explicitly optional.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cert-generator/setup-k8s.sh (1)
274-274: Consider restricting kubeconfig file permissions.The kubeconfig contains a ServiceAccount token granting cluster access. Using
644makes it world-readable;600would limit access to the owner only.🔒 Suggested fix
-chmod 644 /backend/kubeconfig.yaml +chmod 600 /backend/kubeconfig.yaml
🧹 Nitpick comments (1)
cert-generator/setup-k8s.sh (1)
224-242: Consider extracting duplicatenode-ipparsing into a helper function.Lines 19 and 229 use identical sed patterns to extract the k3s node-ip. Extracting this into a reusable helper improves maintainability.
♻️ Suggested refactor
+# Extract node-ip from k3s config (handles quoted and unquoted values) +extract_k3s_node_ip() { + grep -E '^node-ip:' /etc/rancher/k3s/config.yaml 2>/dev/null \ + | sed -E 's/^node-ip:[[:space:]]*"?([^"[:space:]]+)"?.*/\1/' \ + | head -1 +} + configure_kubectl() { # ... if [ -r /etc/rancher/k3s/config.yaml ]; then - K3S_HOST_IP=$(grep -E '^node-ip:' /etc/rancher/k3s/config.yaml 2>/dev/null | sed -E 's/^node-ip:[[:space:]]*"?([^"[:space:]]+)"?.*/\1/' | head -1) + K3S_HOST_IP=$(extract_k3s_node_ip) fiApply the same change to
get_container_host_ip()on line 229.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/actions/k3s-setup/action.yml:
- Around line 51-54: Ensure the kubeconfig directory exists and tighten file
permissions: before writing to KUBECONFIG_PATH create its parent directory if
missing (mkdir -p on dirname(KUBECONFIG_PATH)), write the modified config to
that path, and set restrictive permissions (e.g., 600) instead of world-readable
644; reference the sed output step that writes to "$KUBECONFIG_PATH" and the
subsequent chmod call and replace them with directory creation plus a chmod 600
on KUBECONFIG_PATH.
♻️ Duplicate comments (1)
.github/actions/k3s-setup/action.yml (1)
22-27: Pin and verify the k3s installer instead ofcurl | sh.Running an unpinned remote script (Line 27) is a supply‑chain RCE risk in CI. Please pin a specific installer version/commit and verify its checksum or signature before execution.
✅ Example hardening
- curl -sfL https://get.k3s.io | INSTALL_K3S_EXEC="--disable=traefik --bind-address 0.0.0.0 --tls-san host.docker.internal" sh - + K3S_INSTALLER_SHA256="<pin_me>" + curl -sfL -o /tmp/install_k3s.sh https://get.k3s.io + echo "${K3S_INSTALLER_SHA256} /tmp/install_k3s.sh" | sha256sum -c - + INSTALL_K3S_EXEC="--disable=traefik --bind-address 0.0.0.0 --tls-san host.docker.internal" sh /tmp/install_k3s.sh
🧹 Nitpick comments (2)
cert-generator/setup-k8s.sh (1)
224-242: Consider extracting node-ip parsing to a shared helper.The node-ip extraction logic (lines 228-234) duplicates the pattern from
configure_kubectl(lines 18-20). Consider extracting to a shared function for maintainability.♻️ Suggested refactor
+# Extract k3s node-ip from config (handles both quoted and unquoted values) +get_k3s_node_ip() { + if [ -f /etc/rancher/k3s/config.yaml ]; then + grep -E '^node-ip:' /etc/rancher/k3s/config.yaml 2>/dev/null | sed -E 's/^node-ip:[[:space:]]*"?([^"[:space:]]+)"?.*/\1/' | head -1 + fi +} + configure_kubectl() { # ... if [ -r /etc/rancher/k3s/config.yaml ]; then - K3S_HOST_IP=$(grep -E '^node-ip:' /etc/rancher/k3s/config.yaml 2>/dev/null | sed -E 's/^node-ip:[[:space:]]*"?([^"[:space:]]+)"?.*/\1/' | head -1) + K3S_HOST_IP=$(get_k3s_node_ip) fi # ... } get_container_host_ip() { if [ -f /etc/rancher/k3s/config.yaml ]; then - K3S_NODE_IP=$(grep -E '^node-ip:' /etc/rancher/k3s/config.yaml 2>/dev/null | sed -E 's/^node-ip:[[:space:]]*"?([^"[:space:]]+)"?.*/\1/' | head -1) + K3S_NODE_IP=$(get_k3s_node_ip) # ... fi }docker-compose.yaml (1)
651-668: Pin kafka-exporter to a specific version.The
kafka-exporterimage uses thelatesttag, which can introduce unexpected changes. For consistency with other version-pinned images (Grafana, Kafka, etc.), pin to a specific version such as v1.9.0, the current latest stable release.♻️ Suggested fix
- image: danielqsj/kafka-exporter:latest + image: danielqsj/kafka-exporter:v1.9.0
|
Summary by cubic
Added comprehensive Playwright end-to-end tests for the frontend to cover core user flows, admin pages, and registration. Adds worker-scoped auth fixtures and centralized admin routes; also fixes SSE connection cleanup to prevent Redis leaks on client disconnect.
New Features
Migration
Written for commit edda992. Summary will update on new commits.
Summary by CodeRabbit
Testing Infrastructure
Code Quality
Operational Changes
UI/UX
✏️ Tip: You can customize this high-level summary in your review settings.