Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Playwright Electron E2E smoke suite: docs, Playwright config, fixtures, helpers, five smoke specs, an npm script, and widespread Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Builder as Builder (pnpm run build)
participant Runner as Playwright Runner
participant Electron as Electron App
participant Page as Renderer Page
participant Provider as External Provider
participant FS as Filesystem
Dev->>Builder: pnpm run build
Builder-->>Dev: built output (out/main/index.js)
Dev->>Runner: pnpm run e2e:smoke
Runner->>Electron: _electron.launch() (repo root)
Electron->>Page: open main chat window
Runner->>Page: attach listeners, run helpers (waitForAppReady, selectAgent, sendMessage...)
Page->>Provider: network requests for model/provider
Provider-->>Page: generation responses
Page-->>Runner: UI state updates, console/page errors
Runner->>Electron: close app (teardown)
Runner->>FS: write artifacts (screenshots, traces, logs) to test-results/e2e
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/shadcn/components/ui/dialog/DialogContent.vue (1)
16-18: Downstream heads-up:DialogScrollContentdoes not forward$attrs.The
inheritAttrs: false+v-bind="{ ...forwarded, ...$attrs }"pattern here is correct and consistent withSheetContent.vue. However,src/shadcn/components/ui/dialog/DialogScrollContent.vuestill usesv-bind="forwarded"only — any attributes (e.g.,data-testid, ARIA attrs) callers place onDialogScrollContentwill no longer reach the innerDialogContent. If any consumer relies on that, it'll silently stop working. Consider aligningDialogScrollContentwith the same pattern in a follow-up.Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shadcn/components/ui/dialog/DialogContent.vue` around lines 16 - 18, DialogScrollContent currently binds only "forwarded" props to the inner DialogContent, so component attributes (e.g., data-testid or ARIA attrs) are not forwarded; update DialogScrollContent.vue to match DialogContent/SheetContent pattern by adding defineOptions({ inheritAttrs: false }) (if not present) and change the template binding from v-bind="forwarded" to v-bind="{ ...forwarded, ...$attrs }" so $attrs are merged and passed through to DialogContent.test/e2e/fixtures/electronApp.ts (1)
18-31: Main-window detection is brittle.The URL substring check (
/renderer/index.html) relies on the packaged-app file path layout and will not match a dev server URL if the fixture is ever repointed toELECTRON_RENDERER_URL(e.g.,http://localhost:5173/#/chat). Thetitle === 'DeepChat'fallback is also fragile — any future BrowserWindow (splash, floating, modal chat) that adopts the same title could be mistakenly selected, and if the app title is localized it would stop matching.Prefer a positive signal tied to the main page's DOM (e.g.,
data-testid="app-root"/window-sidebar) once a candidate URL matches, or expose a stabledata-*identity ondocument.documentElementthat tests can key off of. At minimum, restrict the URL check to "ends with/renderer/index.html" and drop the title fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/fixtures/electronApp.ts` around lines 18 - 31, The isMainAppWindow function is brittle: replace the fragile URL substring and title fallback with a deterministic DOM-based check; inside isMainAppWindow, first check the URL more strictly (use url.endsWith('/renderer/index.html') instead of includes), and then verify a stable DOM signal on the page (e.g., await page.evaluate(() => !!document.documentElement.getAttribute('data-main-app') || !!document.querySelector('[data-testid="app-root"]')) ) to confirm the main window; remove the title === 'DeepChat' fallback and ensure the test harness or app sets a data-* attribute (like data-main-app) on document.documentElement so the function can reliably detect the main app window.test/e2e/playwright.config.ts (1)
10-12: Consider enablingforbidOnlyin CI to prevent accidentaltest.onlymerges.Optional hardening: add
forbidOnly: !!process.env.CIso a straytest.onlyin a smoke spec doesn't silently skip the rest of the suite on CI. Givenretries: 0andworkers: 1, flakes will be visible, but an accidentally committed.onlywould not be.retries: 0, + forbidOnly: !!process.env.CI, timeout: 300_000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/playwright.config.ts` around lines 10 - 12, Add Playwright's forbidOnly flag and set it to a CI-aware boolean to fail CI if a test is accidentally marked with test.only: update the config object that currently contains workers, retries, timeout (the same object where workers: 1, retries: 0, timeout: 300_000 is defined) to include forbidOnly: !!process.env.CI so forbidOnly is true on CI and false locally.src/renderer/settings/App.vue (1)
495-504: Consider deriving the mapping fromSETTINGS_NAVIGATION_ITEMSto avoid drift.The explicit
SETTINGS_TAB_TEST_IDSmap hand-renames a few route names (e.g.,settings-common→settings-tab-general,settings-provider→settings-tab-model-providers). If a new settings route is added in@shared/settingsNavigationwithout also updating this map, the fallbacksettings-tab-${name.replace(/^settings-/, '')}will silently produce a different id than tests may expect, and there's no single source of truth. Consider moving the test-id alongside each entry inSETTINGS_NAVIGATION_ITEMS(e.g., atestIdfield) so the mapping lives next to the route definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/App.vue` around lines 495 - 504, The current SETTINGS_TAB_TEST_IDS map and getSettingsTabTestId can drift from SETTINGS_NAVIGATION_ITEMS; instead add a testId property to each entry in SETTINGS_NAVIGATION_ITEMS and derive the mapping from that single source (or build a lookup from SETTINGS_NAVIGATION_ITEMS at module init), then update getSettingsTabTestId to return the entry.testId for the matching name with the same fallback logic (e.g., use entry.testId if present, otherwise fallback to settings-tab-${name.replace(/^settings-/, '')}); update usages of SETTINGS_TAB_TEST_IDS/getSettingsTabTestId to use the new derived lookup so test ids are co-located with the route definitions.test/e2e/specs/02-chat-basic.smoke.spec.ts (1)
31-36: Poll assertion has no explicit timeout and default message.
expect.poll(...).toBe(true)relies on the defaultexpect.timeout: 30_000from the config, which is fine, but consider passing a descriptive message to speed up triage when this fails in CI (the current failure will just sayexpected truewith no context about which model was expected vs. observed):- await expect - .poll(async () => { - const text = (await app.page.getByTestId('app-model-switcher').textContent())?.trim() ?? '' - return text.includes(E2E_TARGET_MODEL_ID) - }) - .toBe(true) + await expect + .poll( + async () => (await app.page.getByTestId('app-model-switcher').textContent())?.trim() ?? '', + { message: `model switcher should display ${E2E_TARGET_MODEL_ID}` } + ) + .toContain(E2E_TARGET_MODEL_ID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/specs/02-chat-basic.smoke.spec.ts` around lines 31 - 36, The poll assertion using expect.poll currently relies on defaults and lacks context on failure; update the call to pass an explicit timeout option to expect.poll (e.g., { timeout: 60_000 }) and add a descriptive failure message that includes E2E_TARGET_MODEL_ID and the selector (app.page.getByTestId('app-model-switcher').textContent()) so the error clearly states which model was expected vs. the observed text; target the expect.poll invocation and its .toBe assertion to add these options/message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/e2e-smoke-regression/spec.md`:
- Around line 49-50: The spec contains conflicting scope definitions for the v1
smoke slice: one place limits v1 to "launch + basic chat" (the list item stating
"Deliver the suite in phases, with the first implementation slice limited to
launch + basic chat") while another section requires additional checks ("restart
persistence and settings navigation"); decide which scope is authoritative for
v1 and make both places consistent: either expand the first-slice bullet to
include "restart persistence" and "settings navigation" or remove those checks
from the v1 requirements so they appear in a later phase; update the two
referenced statements (the "Deliver the suite in phases..." bullet and the
section that lists restart persistence/settings navigation) to use the same
phrasing and explicitly label which checks are v1 vs later phases.
- Line 1: The spec package is missing the required tasks artifact; add a new
file named docs/specs/e2e-smoke-regression/tasks.md that lists the
implementation tasks referenced by spec.md and plan.md, includes
owner/estimate/status for each task, and explicitly addresses or resolves any
"[NEEDS CLARIFICATION]" items found in spec.md or plan.md so work is traceable;
ensure the new tasks.md follows the same structure/format used by other
docs/specs/<feature>/tasks.md files and cross-references spec.md and plan.md
sections where appropriate.
In `@scripts/fetch-acp-registry.mjs`:
- Around line 12-20: The registry is being written before icons are updated
which can leave registry.json paired with stale/missing icons; update the flow
so icon fetching succeeds before committing the registry (or make
hasLocalSnapshot validate coherence by checking ICON_OUTPUT_DIR contents against
OUTPUT_PATH), e.g., ensure the icon download step runs and completes without
error prior to writing OUTPUT_PATH/registry.json, or change hasLocalSnapshot to
also verify ICON_OUTPUT_DIR exists and contains the expected entries referenced
by registry.json so you never treat a snapshot as present unless both registry
and icons are coherent.
In `@test/e2e/fixtures/electronApp.ts`:
- Around line 112-147: launchApp currently launches electronApp then proceeds to
operations that can throw (waitForMainAppWindow, page.waitForLoadState) before
the new instance is added to launchedApps, causing orphaned Electron processes
on failure; fix by capturing launchCount into a local currentLaunch, immediately
create and register a minimal ElectronAppInstance (with electronApp, placeholder
page/flags and the close logic) into launchedApps right after await
electron.launch(), then proceed to attachPageListeners/waitForLoadState and
update the instance's page/other fields when ready, and ensure any exception in
the setup path closes electronApp (or remove the registered instance) so
launchedApps and the cleanup finally block always sees the launched app;
reference functions/variables: launchApp, electron.launch, waitForMainAppWindow,
attachPageListeners, launchedApps, launchCount, ElectronAppInstance.
In `@test/e2e/helpers/chat.ts`:
- Around line 32-61: Update selectModel to verify and select by exact selected
state and provider: read the model switcher's data-selected-model-id (and
data-selected-provider-id when a provider check is needed) instead of using
existingText.includes(modelId); return early only if data-selected-model-id ===
modelId (and data-selected-provider-id === providerId when provided). When
locating the option, keep using
`[data-testid="model-option"][data-model-id="${modelId}"]` but ensure the final
wait/poll checks the switcher's data-selected-model-id (and provider attribute)
for exact equality rather than checking text/includes; also ensure the
searchInput fill and option.click behavior is unchanged so selection still
occurs when needed.
In `@test/e2e/helpers/wait.ts`:
- Around line 44-52: The poll in waitForGenerationDone() can pass immediately if
data-generating is still 'false' when called right after sendMessage(), causing
a race; update waitForGenerationDone() to first poll until the chat-page's
data-generating attribute becomes 'true' (with a short timeout) to ensure
generation actually started, then poll until it becomes 'false' (as currently
implemented) so assertions on assistant messages don't race the generation
lifecycle.
In `@test/e2e/README.md`:
- Line 24: Replace the contributor-specific absolute Windows link in
test/e2e/README.md that points to C:/... with a repo-relative link to the
testData.ts file (e.g., ./helpers/testData.ts or ../helpers/testData.ts
depending on README location) so the "testData.ts" reference resolves for all
contributors; update the existing link text that currently contains the Windows
path to use the relative path instead.
In `@test/e2e/specs/03-session-persistence.smoke.spec.ts`:
- Around line 14-50: The test misses verifying session B's message after
relaunch and doesn't restore/select agent filter state on the relaunched app;
update the test to (1) after relaunch call selectAgent(relaunched.page) or
explicitly set the "all agents" filter to ensure sidebar items are visible, and
(2) openSessionById(relaunched.page, sessionBId) and assert
getAssistantMessages(relaunched.page).last() contains sessionBToken (mirroring
the existing session A check) so persistence for both sessions is validated;
reference openSessionById, getAssistantMessages, selectAgent, sessionBId,
sessionBToken and the relaunched variable when making the changes.
In `@test/e2e/specs/05-settings-provider.smoke.spec.ts`:
- Around line 11-20: The smoke test unconditionally runs a live provider
connectivity check (selectProvider and verifyProviderConnection using
E2E_TARGET_PROVIDER_ID/E2E_TARGET_MODEL_ID) which causes failures when
credentials or network are absent; make this opt-in by guarding the test with an
environment flag (e.g., only run the body if
process.env.RUN_PROVIDER_INTEGRATION === 'true') or move the test to a separate
provider-integration suite so default pnpm run e2e:smoke does not execute
selectProvider/verifyProviderConnection; keep the test name and helper calls but
wrap/relocate them so the connectivity check only runs when explicitly enabled.
---
Nitpick comments:
In `@src/renderer/settings/App.vue`:
- Around line 495-504: The current SETTINGS_TAB_TEST_IDS map and
getSettingsTabTestId can drift from SETTINGS_NAVIGATION_ITEMS; instead add a
testId property to each entry in SETTINGS_NAVIGATION_ITEMS and derive the
mapping from that single source (or build a lookup from
SETTINGS_NAVIGATION_ITEMS at module init), then update getSettingsTabTestId to
return the entry.testId for the matching name with the same fallback logic
(e.g., use entry.testId if present, otherwise fallback to
settings-tab-${name.replace(/^settings-/, '')}); update usages of
SETTINGS_TAB_TEST_IDS/getSettingsTabTestId to use the new derived lookup so test
ids are co-located with the route definitions.
In `@src/shadcn/components/ui/dialog/DialogContent.vue`:
- Around line 16-18: DialogScrollContent currently binds only "forwarded" props
to the inner DialogContent, so component attributes (e.g., data-testid or ARIA
attrs) are not forwarded; update DialogScrollContent.vue to match
DialogContent/SheetContent pattern by adding defineOptions({ inheritAttrs: false
}) (if not present) and change the template binding from v-bind="forwarded" to
v-bind="{ ...forwarded, ...$attrs }" so $attrs are merged and passed through to
DialogContent.
In `@test/e2e/fixtures/electronApp.ts`:
- Around line 18-31: The isMainAppWindow function is brittle: replace the
fragile URL substring and title fallback with a deterministic DOM-based check;
inside isMainAppWindow, first check the URL more strictly (use
url.endsWith('/renderer/index.html') instead of includes), and then verify a
stable DOM signal on the page (e.g., await page.evaluate(() =>
!!document.documentElement.getAttribute('data-main-app') ||
!!document.querySelector('[data-testid="app-root"]')) ) to confirm the main
window; remove the title === 'DeepChat' fallback and ensure the test harness or
app sets a data-* attribute (like data-main-app) on document.documentElement so
the function can reliably detect the main app window.
In `@test/e2e/playwright.config.ts`:
- Around line 10-12: Add Playwright's forbidOnly flag and set it to a CI-aware
boolean to fail CI if a test is accidentally marked with test.only: update the
config object that currently contains workers, retries, timeout (the same object
where workers: 1, retries: 0, timeout: 300_000 is defined) to include
forbidOnly: !!process.env.CI so forbidOnly is true on CI and false locally.
In `@test/e2e/specs/02-chat-basic.smoke.spec.ts`:
- Around line 31-36: The poll assertion using expect.poll currently relies on
defaults and lacks context on failure; update the call to pass an explicit
timeout option to expect.poll (e.g., { timeout: 60_000 }) and add a descriptive
failure message that includes E2E_TARGET_MODEL_ID and the selector
(app.page.getByTestId('app-model-switcher').textContent()) so the error clearly
states which model was expected vs. the observed text; target the expect.poll
invocation and its .toBe assertion to add these options/message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f20b232-5d71-4977-9529-300748f77056
📒 Files selected for processing (39)
docs/specs/e2e-smoke-regression/plan.mddocs/specs/e2e-smoke-regression/spec.mdpackage.jsonscripts/fetch-acp-registry.mjssrc/renderer/settings/App.vuesrc/renderer/settings/components/AcpSettings.vuesrc/renderer/settings/components/CommonSettings.vuesrc/renderer/settings/components/DisplaySettings.vuesrc/renderer/settings/components/McpSettings.vuesrc/renderer/settings/components/ModelProviderSettings.vuesrc/renderer/settings/components/ProviderApiConfig.vuesrc/renderer/src/App.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/WindowSideBarSessionItem.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/ChatInputToolbar.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/components/message/MessageItemUser.vuesrc/renderer/src/components/settings/ModelCheckDialog.vuesrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/views/SettingsTabView.vuesrc/shadcn/components/ui/dialog/DialogContent.vuesrc/shadcn/components/ui/select/SelectItem.vuesrc/shadcn/components/ui/select/SelectTrigger.vuetest/e2e/README.mdtest/e2e/fixtures/electronApp.tstest/e2e/helpers/chat.tstest/e2e/helpers/settings.tstest/e2e/helpers/testData.tstest/e2e/helpers/wait.tstest/e2e/playwright.config.tstest/e2e/specs/01-launch.smoke.spec.tstest/e2e/specs/02-chat-basic.smoke.spec.tstest/e2e/specs/03-session-persistence.smoke.spec.tstest/e2e/specs/04-settings-navigation.smoke.spec.tstest/e2e/specs/05-settings-provider.smoke.spec.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/helpers/chat.ts (1)
50-54:⚠️ Potential issue | 🟠 MajorConstrain the model option by provider when
providerIdis supplied.The final poll checks
data-selected-provider-id, but the clicked option still matches onlydata-model-id. If two providers expose the same model ID, this can click the wrong option and then time out.🐛 Proposed fix
- const option = page.locator(`[data-testid="model-option"][data-model-id="${modelId}"]`).first() + const optionSelector = providerId + ? `[data-testid="model-option"][data-provider-id="${providerId}"]` + + `[data-model-id="${modelId}"]` + : `[data-testid="model-option"][data-model-id="${modelId}"]` + const option = page.locator(optionSelector).first() + const modelLabel = providerId ? `${providerId}/${modelId}` : modelId await expect( option, - `Model "${modelId}" was not found. Configure it before running "pnpm run e2e:smoke".` + `Model "${modelLabel}" was not found. Configure it before running "pnpm run e2e:smoke".` ).toBeVisible({ timeout: 30_000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/helpers/chat.ts` around lines 50 - 54, The locator for option (const option = page.locator(`[data-testid="model-option"][data-model-id="${modelId}"]`).first()) only filters by modelId and can pick the wrong provider; when providerId is provided, include an additional attribute filter for data-selected-provider-id (or equivalent) so the locator targets `[data-testid="model-option"][data-model-id="${modelId}"][data-selected-provider-id="${providerId}"]` before clicking and before the final expect; update uses of option and the final expect to use the provider-constrained locator so the click and visibility check target the correct provider-specific model option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/helpers/chat.ts`:
- Around line 50-54: The locator for option (const option =
page.locator(`[data-testid="model-option"][data-model-id="${modelId}"]`).first())
only filters by modelId and can pick the wrong provider; when providerId is
provided, include an additional attribute filter for data-selected-provider-id
(or equivalent) so the locator targets
`[data-testid="model-option"][data-model-id="${modelId}"][data-selected-provider-id="${providerId}"]`
before clicking and before the final expect; update uses of option and the final
expect to use the provider-constrained locator so the click and visibility check
target the correct provider-specific model option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6698f99-206d-47b8-af8f-d7509453ecf6
📒 Files selected for processing (12)
docs/specs/e2e-smoke-regression/plan.mddocs/specs/e2e-smoke-regression/spec.mddocs/specs/e2e-smoke-regression/tasks.mdscripts/fetch-acp-registry.mjssrc/renderer/src/components/chat/ChatStatusBar.vuetest/e2e/README.mdtest/e2e/fixtures/electronApp.tstest/e2e/helpers/chat.tstest/e2e/helpers/wait.tstest/e2e/specs/02-chat-basic.smoke.spec.tstest/e2e/specs/03-session-persistence.smoke.spec.tstest/e2e/specs/05-settings-provider.smoke.spec.ts
✅ Files skipped from review due to trivial changes (5)
- src/renderer/src/components/chat/ChatStatusBar.vue
- test/e2e/specs/03-session-persistence.smoke.spec.ts
- test/e2e/README.md
- docs/specs/e2e-smoke-regression/plan.md
- docs/specs/e2e-smoke-regression/tasks.md
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/specs/05-settings-provider.smoke.spec.ts
- scripts/fetch-acp-registry.mjs
- test/e2e/specs/02-chat-basic.smoke.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/helpers/chat.ts`:
- Around line 78-82: The test helper sendMessage is querying the wrong test-id;
replace usage of 'chat-input-contenteditable' with the renderer-exposed test id
'chat-input-editor' in the sendMessage function so the Page locator matches the
actual editor element (update the const editor = page.getByTestId(...) inside
sendMessage to use 'chat-input-editor'), leaving the rest of the
visibility/click/fill flow unchanged.
- Around line 59-62: The long template string passed to expect (the message
using providerId and modelId) exceeds 100 characters; break or wrap the message
so no line exceeds 100 chars and follow project style (single quotes, no
semicolons). Update the assertion in test/e2e/helpers/chat.ts where option is
checked (the expect(...) .toBeAttached call) to construct the message across
multiple concatenated/template pieces or use a short helper variable for the
message (referencing providerId and modelId) so the final lines are within 100
chars and use single quotes.
- Around line 111-117: The openSessionById function currently clicks the sidebar
item but doesn't wait for the app to complete the session switch; after clicking
the locator found in openSessionById, wait for that same element to have
data-active="true" (the WindowSideBarSessionItem.vue exposes data-active) using
a Playwright attribute-wait (e.g.,
expect(...).toHaveAttribute('data-active','true',{timeout:...})), so callers
won't race against the previous session's messages.
- Around line 25-29: The helper currently clicks the agent button but returns
immediately because page.getByTestId('chat-input-editor') may already be
visible; instead after performing the click (the block that checks
button.getAttribute('data-selected') and calls button.click()), wait until the
clicked button's data-selected attribute becomes 'true' to ensure the sidebar
switched agents (use the same button reference and its 'data-selected'
attribute); replace or augment the final visibility wait with an assertion that
button (from button.getAttribute('data-selected')) has attribute 'data-selected'
=== 'true' before proceeding to
expect(page.getByTestId('chat-input-editor')).toBeVisible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c8bbb27-5117-43b4-bbab-d43e9bc00a1d
📒 Files selected for processing (3)
.gitignoresrc/renderer/src/stores/modelStore.tstest/e2e/helpers/chat.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+59
to
+62
| await expect( | ||
| option, | ||
| `Model "${providerId ? `${providerId}/` : ''}${modelId}" was not found. Configure it before running "pnpm run e2e:smoke".` | ||
| ).toBeAttached({ timeout: 30_000 }) |
Contributor
There was a problem hiding this comment.
Wrap the long model-not-found message.
Line 61 exceeds the configured 100 character width.
🧹 Proposed formatting fix
await expect(
option,
- `Model "${providerId ? `${providerId}/` : ''}${modelId}" was not found. Configure it before running "pnpm run e2e:smoke".`
+ `Model "${providerId ? `${providerId}/` : ''}${modelId}" was not found. ` +
+ 'Configure it before running "pnpm run e2e:smoke".'
).toBeAttached({ timeout: 30_000 })As per coding guidelines, **/*.{ts,tsx,js,jsx}: Use single quotes, no semicolons, and maintain 100 character line width per Oxfmt configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect( | |
| option, | |
| `Model "${providerId ? `${providerId}/` : ''}${modelId}" was not found. Configure it before running "pnpm run e2e:smoke".` | |
| ).toBeAttached({ timeout: 30_000 }) | |
| await expect( | |
| option, | |
| `Model "${providerId ? `${providerId}/` : ''}${modelId}" was not found. ` + | |
| 'Configure it before running "pnpm run e2e:smoke".' | |
| ).toBeAttached({ timeout: 30_000 }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/helpers/chat.ts` around lines 59 - 62, The long template string
passed to expect (the message using providerId and modelId) exceeds 100
characters; break or wrap the message so no line exceeds 100 chars and follow
project style (single quotes, no semicolons). Update the assertion in
test/e2e/helpers/chat.ts where option is checked (the expect(...) .toBeAttached
call) to construct the message across multiple concatenated/template pieces or
use a short helper variable for the message (referencing providerId and modelId)
so the final lines are within 100 chars and use single quotes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/helpers/chat.ts (1)
33-44:createNewChatmay click an invisible fallback button.When
sidebarButtonis not visible, the helper immediately clickscollapsedButtonwithout waiting for it to be visible/attached. If neither button is rendered yet (e.g., during app initialization), this will fail with a less actionable error than an explicit wait. Consider awaiting visibility on the chosen target before clicking, mirroring the pattern used inselectAgent.♻️ Proposed refactor
- if (await sidebarButton.isVisible().catch(() => false)) { - await sidebarButton.click() - } else { - await collapsedButton.click() - } + const button = (await sidebarButton.isVisible().catch(() => false)) + ? sidebarButton + : collapsedButton + await expect(button).toBeVisible({ timeout: 30_000 }) + await button.click()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/helpers/chat.ts` around lines 33 - 44, The createNewChat helper currently may click collapsedButton without waiting for it to be attached/visible; update createNewChat so after determining which control to use (sidebarButton or collapsedButton) you explicitly wait for that button to be visible/attached before clicking (e.g., use await expect(chosenButton).toBeVisible({ timeout }) or await chosenButton.waitFor({ state: 'visible' })), and keep the existing fallback logic and final wait for 'chat-input-editor' — reference the createNewChat function and the sidebarButton / collapsedButton variables and mirror the explicit visibility-wait pattern used in selectAgent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/helpers/chat.ts`:
- Around line 33-44: The createNewChat helper currently may click
collapsedButton without waiting for it to be attached/visible; update
createNewChat so after determining which control to use (sidebarButton or
collapsedButton) you explicitly wait for that button to be visible/attached
before clicking (e.g., use await expect(chosenButton).toBeVisible({ timeout })
or await chosenButton.waitFor({ state: 'visible' })), and keep the existing
fallback logic and final wait for 'chat-input-editor' — reference the
createNewChat function and the sidebarButton / collapsedButton variables and
mirror the explicit visibility-wait pattern used in selectAgent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc4edbc1-f710-46ed-a473-70eed642bc30
📒 Files selected for processing (4)
src/shadcn/components/ui/dialog/DialogScrollContent.vuetest/e2e/helpers/chat.tstest/e2e/playwright.config.tstest/e2e/specs/02-chat-basic.smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/specs/02-chat-basic.smoke.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
Chores
Documentation