feat: Add E2E test suite for Assistant feature

[stepandel]

Summary

Comprehensive E2E test coverage for the Pinecone Assistant feature (PINE-51).

Test Suites (8 spec files)

1. assistant-mode.spec.ts - Mode switching (switcher, keyboard shortcuts, persistence)
2. assistant-crud.spec.ts - Assistant CRUD operations (create, edit, delete, validation)
3. assistant-files.spec.ts - Files panel (list, status, selection, context menu)
4. assistant-upload.spec.ts - File upload flow (dialog, file picker, metadata, progress)
5. assistant-file-detail.spec.ts - File detail panel (metadata, download, delete)
6. assistant-chat.spec.ts - Chat interface (messages, streaming, model selector)
7. assistant-citations.spec.ts - Citation functionality (popover, file navigation)
8. assistant-integration.spec.ts - Full flow (create → upload → process → chat → citations)

Supporting Files

• e2e/helpers/assistant-helpers.ts - Helper functions for all assistant operations
• e2e/fixtures/test-document.txt - Test fixture for upload tests

Components Modified

Added data-testid attributes to:

• ModeSwitcher, AssistantsPanel, AssistantConfigView
• FilesPanel, FileDetailPanel, UploadFileDialog
• ChatView, ChatMessage, CitationPopover
• NewButton (accepts data-testid prop)

Testing

• Build passes: pnpm test:build ✅
• E2E tests require Electron system deps + valid PINECONE_API_KEY

Closes PINE-51

Summary by CodeRabbit

• Tests
  • Added comprehensive end-to-end test coverage for assistant features including chat interactions, file uploads, citations, and mode switching.
  • Added testing infrastructure and helper utilities to ensure reliable assistant functionality across different scenarios and user workflows.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pine-51-assistant-e2e-tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 13

🤖 Fix all issues with AI agents

In `@e2e/assistant-citations.spec.ts`:
- Around line 99-134: The test "citation superscript should be visible when
response has citations" can pass without citations; update it to use a
deterministic setup that guarantees citations: ensure getFileCount or test setup
uploads/uses a known fixture knowledge base (or mock the assistant response) so
the prompt will always produce citations, replace the generic prompt in
sendChatMessage with a citation-triggering prompt/fixture, and add an explicit
assertion on citationCount (e.g., expect(citationCount).toBeGreaterThan(0))
and/or expect(citations.first()).toBeVisible() so the test fails when no
citations are rendered; locate changes around the test block, getFileCount,
sendChatMessage, waitForAssistantResponse, and the
'[data-testid="citation-superscript"]' locator.
- Around line 277-307: The test "multiple citations should have incremental
numbers" currently silently passes when citationCount is 0 or 1; change the
logic to explicitly skip the test in that case by invoking test.skip() when
citationCount <= 1 (i.e., after computing citationCount from the locator named
citations), otherwise proceed with the existing loop that collects
data-citation-index into indices and asserts uniqueIndices.size equals
indices.length; this ensures the test is skipped rather than passing when there
are insufficient citations.

In `@e2e/assistant-crud.spec.ts`:
- Around line 191-204: The test defines an errorMessage locator but never
asserts it; update the spec to assert the validation error is shown after
filling the invalid name by adding an expectation such as
expect(errorMessage).toBeVisible() or expect(await
errorMessage.count()).toBeGreaterThan(0) immediately after reading inputValue
(before clicking the assistant-cancel-button). Locate and modify the existing
variables nameInput and errorMessage in the test and place the visibility/count
assertion prior to the cancel click to ensure the validation is actually tested.
- Around line 293-313: The test "should edit assistant via context menu"
currently dispatches a CustomEvent('assistant-edit') which the UI doesn't listen
for; instead invoke the IPC context menu handler used by AssistantsPanel: inside
the test replace the page.evaluate block that dispatches 'assistant-edit' with a
call to the renderer IPC wrapper (e.g. call
window.electronAPI.contextMenu.onAssistantAction or the equivalent method
exposed on window.electronAPI) and pass the 'edit' action and { assistantName:
testAssistantName } so the same code path as a native context-menu edit is
exercised; if you cannot reliably call the IPC shim in the test environment,
remove the test and document the native context-menu limitation.

In `@e2e/assistant-file-detail.spec.ts`:
- Around line 275-306: The test 'delete button should delete the file' currently
clicks the delete button but never asserts the outcome; update the test (inside
the same block that uses getFileCount, fileItem, fileId, and deleteButton) to
verify deletion by waiting for the file item to disappear (e.g., wait for the
locator '[data-testid="file-item"][data-file-id="<fileId>"]' to be detached) or
assert the detail panel shows a "Deleting" status (waitForSelector that contains
"Deleting"); use Playwright waitForSelector/waitForTimeout patterns and explicit
assertions so the test fails if the file remains.
- Around line 114-144: The test "selecting a file should show its details" is a
no-op when getFileCount(page) === 0; update the test to ensure a file exists
before asserting by either uploading a fixture or failing fast: if fileCount ===
0, call the existing upload helper (e.g., uploadFile(page,
'fixtures/sample.pdf') or uploadTestFile(page, ...)) and wait for indexing, or
replace the skip with an assertion that creates a test file; then proceed to
locate the first file via page.locator('[data-testid="file-item"]').first(),
click it, and assert the detail panel shows the file name as before so the test
always executes its assertions.

In `@e2e/assistant-integration.spec.ts`:
- Around line 388-405: The test "should show error for network failures"
currently only checks chat-view visibility; update it to simulate a network
failure using Playwright's page.route() to intercept the app's API request(s)
(e.g., the assistant request URL pattern used by your app) and respond with an
error (500 or network failure), then trigger the action that sends the request
and assert that the error display element (e.g., the error-area locator like
'[data-testid="chat-error"]' or the component that renders when `error` state is
truthy) becomes visible and contains an expected error message; keep the
existing hasRealApiKey guard or alternatively skip real-key branch and ensure
you use the test name should show error for network failures and the page and
chat-view locators to locate the UI.

In `@e2e/assistant-mode.spec.ts`:
- Around line 148-149: The test uses hardcoded macOS-only modifiers ('Meta+1'
and 'Meta+2'); make it cross-platform by selecting the modifier at runtime
(e.g., const modifier = process.platform === 'darwin' ? 'Meta' : 'Control') and
replace page.keyboard.press('Meta+1') / page.keyboard.press('Meta+2') with
page.keyboard.press(`${modifier}+1`) and page.keyboard.press(`${modifier}+2`),
updating both occurrences (the calls to page.keyboard.press for 1 and 2) so the
shortcuts work on Windows/Linux and macOS.
- Around line 198-204: The conditional check around the mode assertion allows
the test to skip verifying mode when the mode switcher is not visible; remove
the conditional and assert the mode switcher visibility first (use the existing
modeSwitcher locator) so the test fails if the UI element is missing, then call
getCurrentMode(page) and expect it toBe('assistant'); update the test block that
contains modeSwitcher and getCurrentMode to always perform both the visibility
assertion and the mode equality assertion.

In `@e2e/assistant-upload.spec.ts`:
- Around line 182-200: The test "upload dialog should show metadata input"
contains no assertions and will always pass; either add assertions that verify
the metadata input or explicitly skip with a TODO. To fix, when hasRealApiKey is
true use the existing electronContext.page to simulate file selection (e.g., use
Playwright's setInputFiles or mock the file picker) to open the upload dialog,
then query for the metadata input by its data-testid (the data-testid used in
the codebase for the upload flow) and assert it exists and is visible;
alternatively, if file dialog mocking isn't implemented yet, call test.skip()
unconditionally with a TODO comment so the test doesn't silently pass. Ensure
you update the test block named upload dialog should show metadata input and use
electronContext.page to locate [data-testid="..."] and add expect assertions
accordingly.
- Around line 231-254: The test 'file should show processing status initially'
currently returns silently when getFileCount(page) === 0; change it to
explicitly skip the test in that case by invoking test.skip() (same pattern as
used in the other test) so the outcome is unambiguous. Locate the block that
reads const fileCount = await getFileCount(page) and add an early conditional
that calls test.skip() and returns when fileCount === 0 (refer to the existing
use of test.skip() earlier in this spec), leaving the rest of the logic that
queries page.locator('[data-testid="file-item"]').first() and the statusText
expectation unchanged. Ensure the skip path runs before any assertions so the
test either asserts the status or is reported as skipped.
- Around line 256-274: The test "upload progress should be shown during upload"
is a placeholder with no assertions and should be marked skipped until a real
upload path exists; modify the test declaration for the upload progress (the
test('upload progress should be shown during upload', async () => { ... })) to
be skipped by using test.skip('upload progress should be shown during upload',
async () => { ... }) or call test.skip() at the top of the test body, add a
concise TODO comment referencing wiring up the real upload flow (the
electronContext upload path), and ensure no empty placeholder logic remains so
the suite doesn't report a false positive.
- Around line 147-180: The test "should upload a file via API" uses a hardcoded
placeholder uploadResult and an unused initialFileCount, so change the test to
be skipped until a real test fixture exists: call test.skip() at the start of
the test (or replace the body with a skipped test and a TODO comment) for the
test named "should upload a file via API"; keep references to initialFileCount
and uploadResult intact for future implementation but remove the ineffective
assertion expect(uploadResult).toBeDefined() so the test cannot falsely pass
until you implement a real file fixture and a post-upload assertion comparing
initialFileCount to the new count.

🧹 Nitpick comments (15)

e2e/helpers/assistant-helpers.ts (4)

11-18: Replace arbitrary timeouts with condition-based waits.

Using waitForTimeout(500) is a test anti-pattern that leads to flaky tests. The wait could be too short on slow CI machines or unnecessarily long locally.

♻️ Suggested improvement

 export async function switchToAssistantMode(page: Page): Promise<void> {
   const modeButton = page.locator('[data-testid="mode-assistant"]')
   await modeButton.click()
-  await page.waitForTimeout(500) // Wait for mode transition
   
   // Verify we're in assistant mode
   await expect(modeButton).toHaveAttribute('aria-checked', 'true')
+  // Wait for the assistants panel to confirm mode transition completed
+  await page.waitForSelector('[data-testid="assistants-panel"]', { timeout: 5000 })
 }

---

77-96: Fragile selectors in deleteTestAssistant may cause test failures.

The selectors for the confirmation input and delete button are imprecise and could match unintended elements or fail if the UI changes slightly.

♻️ Suggested improvements

 export async function deleteTestAssistant(page: Page, name: string): Promise<void> {
   // Find and right-click the assistant item
   const assistantItem = page.locator(`[data-testid="assistant-item"][data-assistant-name="${name}"]`)
   await assistantItem.click({ button: 'right' })
   
-  // Wait for native context menu action to complete (via IPC)
-  // The delete dialog should open
-  await page.waitForTimeout(500)
+  // Wait for delete dialog to appear
+  const dialog = page.locator('[role="dialog"]')
+  await expect(dialog).toBeVisible({ timeout: 5000 })
   
   // Type the assistant name in the confirmation input
-  const confirmationInput = page.locator('input[placeholder]').filter({ hasText: '' })
+  const confirmationInput = dialog.locator(`input[placeholder="${name}"]`)
   await confirmationInput.fill(name)
   
   // Click the delete button
-  const deleteButton = page.locator('button:has-text("Delete")')
+  const deleteButton = dialog.locator('button:has-text("Delete")')
   await deleteButton.click()

---

113-147: uploadTestFile relies on undocumented global state.

The function assumes window.__testProfileId and window.__testAssistantName are set externally, but this contract is not documented and could lead to confusing test failures if the setup is missed.

♻️ Suggested improvement - accept profile/assistant as parameters

 export async function uploadTestFile(
   page: Page, 
   filePath: string,
   options?: {
     metadata?: Record<string, string | number>
     multimodal?: boolean
+    profileId: string
+    assistantName: string
   }
 ): Promise<void> {
-  // Get current profile and assistant from the page context
-  const result = await page.evaluate(async ({ filePath, metadata, multimodal }) => {
-    // Get the current profile and assistant from the window state
-    // This requires access to the React context, so we use a data attribute approach
-    const profileId = (window as any).__testProfileId
-    const assistantName = (window as any).__testAssistantName
-    
-    if (!profileId || !assistantName) {
-      throw new Error('Profile ID or assistant name not set for test')
-    }
+  if (!options?.profileId || !options?.assistantName) {
+    throw new Error('profileId and assistantName are required')
+  }
+  
+  const result = await page.evaluate(async ({ filePath, metadata, multimodal, profileId, assistantName }) => {
     
     await (window as any).electronAPI.assistant.files.upload(profileId, assistantName, {
       filePath,
       metadata,
       multimodal,
     })
     
     return { success: true }
-  }, { filePath, metadata: options?.metadata, multimodal: options?.multimodal })
+  }, { filePath, metadata: options?.metadata, multimodal: options?.multimodal, profileId: options.profileId, assistantName: options.assistantName })

---

311-326: getFileDetails uses brittle class-based selectors.

Selectors like .font-medium and [class*="bg-green"] are fragile and will break if styling changes. Consider adding data-testid attributes to these elements instead.

♻️ Suggested approach

Add data-testid attributes to the FileDetailPanel component for the filename and status elements, then update the helper:

 export async function getFileDetails(page: Page): Promise<{
   name: string | null
   status: string | null
 }> {
   const detailPanel = page.locator('[data-testid="file-detail-panel"]')
   
-  // Get file name
-  const nameElement = detailPanel.locator('.font-medium').first()
-  const name = await nameElement.textContent()
+  // Get file name (requires data-testid="file-detail-name" in component)
+  const nameElement = detailPanel.locator('[data-testid="file-detail-name"]')
+  const name = await nameElement.textContent()
   
-  // Get status badge text
-  const statusBadge = detailPanel.locator('[class*="bg-green"], [class*="bg-yellow"], [class*="bg-red"]').first()
-  const status = await statusBadge.textContent()
+  // Get status badge text (requires data-testid="file-detail-status" in component)
+  const statusBadge = detailPanel.locator('[data-testid="file-detail-status"]')
+  const status = await statusBadge.textContent()
   
   return { name, status }
 }

e2e/assistant-crud.spec.ts (1)

53-59: Consider extracting the API key check to reduce repetition.

The API key check is duplicated across all tests. While this is necessary for serial tests, it could be extracted into a reusable helper or a test.beforeEach hook that sets a skip condition.

♻️ Optional improvement

// At the top of the file or in a shared helper
function skipIfNoApiKey(test: typeof import('@playwright/test').test) {
  const hasRealApiKey = !!process.env.PINECONE_API_KEY &&
                       process.env.PINECONE_API_KEY !== 'dummy-key-for-local-testing'
  if (!hasRealApiKey) {
    test.skip()
  }
}

// In each test:
test('should list existing assistants', async () => {
  skipIfNoApiKey(test)
  // ... rest of test
})

e2e/assistant-chat.spec.ts (1)

92-97: Replace arbitrary timeout with status polling for assistant readiness.

Using waitForTimeout(5000) is unreliable - the assistant might take longer on slow networks or be ready much sooner. Consider polling the assistant status indicator.

♻️ Suggested improvement

     // Wait for assistant to be created and become ready
     await page.waitForSelector(`[data-testid="assistant-item"][data-assistant-name="${testAssistantName}"]`, { timeout: 30000 })
     
-    // Wait for assistant to be ready
-    await page.waitForTimeout(5000)
+    // Wait for assistant to reach Ready status
+    const assistantItem = page.locator(`[data-testid="assistant-item"][data-assistant-name="${testAssistantName}"]`)
+    const statusIndicator = assistantItem.locator('[data-testid="assistant-status"]')
+    await expect(statusIndicator).toHaveAttribute('data-status', 'Ready', { timeout: 60000 })

e2e/assistant-files.spec.ts (2)

188-211: Test has no coverage when fileCount is 0.

For a newly created test assistant without uploaded files, this test's main assertions (lines 205-209) are never executed. Consider either:

1. Uploading a test file in setup, or
2. Marking the test as skipped with a clear message when no files exist

♻️ Suggested improvement

   test('file items should show status indicators', async () => {
     // ... API key check ...
 
     await selectAssistant(page, testAssistantName)
 
     const fileCount = await getFileCount(page)
     
-    if (fileCount > 0) {
+    if (fileCount === 0) {
+      test.skip(true, 'No files available to test status indicators')
+      return
+    }
+    
-      // Check that file items have status indicators
-      const fileItem = page.locator('[data-testid="file-item"]').first()
-      
-      // Should show one of: Ready, Processing, Failed, Deleting
-      const statusIndicator = fileItem.locator('text=/Ready|Processing|Failed|Deleting/')
-      await expect(statusIndicator).toBeVisible({ timeout: 5000 })
-    }
+    // Check that file items have status indicators
+    const fileItem = page.locator('[data-testid="file-item"]').first()
+    
+    // Should show one of: Ready, Processing, Failed, Deleting
+    const statusIndicator = fileItem.locator('text=/Ready|Processing|Failed|Deleting/')
+    await expect(statusIndicator).toBeVisible({ timeout: 5000 })
   })

---

159-162: Inconsistent selector between tests.

Line 134 uses page.locator('[data-testid="upload-file-button"]') while line 160 uses page.locator('button:has-text("Upload File")'). Use the data-testid consistently for reliability.

♻️ Suggested fix

-    const uploadButton = page.locator('button:has-text("Upload File")')
+    const uploadButton = page.locator('[data-testid="upload-file-button"]')
     await expect(uploadButton).toBeVisible({ timeout: 5000 })

e2e/assistant-upload.spec.ts (1)

48-91: Consider DRYing assistant creation and API-key gating.
You already have a createTestAssistant helper; using it here avoids duplicated UI steps. Also, the API-key gate is repeated in every test—centralizing it in a beforeEach would reduce noise and avoid drift.

e2e/assistant-integration.spec.ts (3)

55-90: Centralize the API-key gate to reduce repetition.
The same hasRealApiKey guard appears in every test; a beforeEach gate would be cleaner and reduce drift.

---

283-313: Consider removing fixed sleeps.
waitForTimeout(500) is brittle; the subsequent attribute/panel assertions already provide a deterministic wait.

♻️ Suggested tweak

       const indexButton = page.locator('[data-testid="mode-index"]')
       await indexButton.click()
-      await page.waitForTimeout(500)
 
       // Verify we're in index mode
       await expect(indexButton).toHaveAttribute('aria-checked', 'true')

---

335-361: Prefer waiting on UI state vs fixed timeouts.
You can rely on the visibility assertion timeout instead of a fixed 3s sleep, which can be flaky under load.

♻️ Suggested tweak

       await page.evaluate(async (name) => {
         const profiles = await (window as any).electronAPI.profiles.getAll()
         const profile = profiles.find((p: any) => p.id.startsWith('test-'))
         if (profile) {
           await (window as any).electronAPI.assistant.delete(profile.id, name)
         }
       }, testAssistantName)
 
-      // Wait for deletion
-      await page.waitForTimeout(3000)
-
       // Assistant should no longer be in the list
       const assistantItem = page.locator(`[data-testid="assistant-item"][data-assistant-name="${testAssistantName}"]`)
       await expect(assistantItem).not.toBeVisible({ timeout: 10000 })

e2e/assistant-file-detail.spec.ts (1)

49-92: Use the existing helper to reduce duplication.
createTestAssistant already encapsulates the create flow and waits; reusing it keeps this consistent with other specs.

e2e/assistant-citations.spec.ts (2)

53-97: Consider using the helper for assistant creation.
createTestAssistant would reduce duplicate UI steps and keep the flow consistent with other specs.

---

248-275: Make the “page numbers” assertion meaningful.
pageInfo is computed but never asserted. Consider asserting its visibility when it exists, so the test matches the title.