Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 63 additions & 22 deletions .github/workflows/playwright.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,28 @@ jobs:
test:
timeout-minutes: 60
runs-on: ubuntu-latest


services:
postgres:
image: postgres:15
env:
POSTGRES_PASSWORD: postgres
POSTGRES_DB: postgres
ports:
- 5432:5432
# Wait for postgres to be ready
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5

strategy:
fail-fast: false
matrix:
shardIndex: [1, 2, 3, 4]
shardTotal: [4]

steps:
- name: Checkout code
uses: actions/checkout@v4
Expand All @@ -26,36 +47,56 @@ jobs:
- name: Install Playwright Browsers
run: bunx playwright install --with-deps

- name: Build application
run: bun run build
- name: Run migrations
run: bun run db:migrate
env:
# Add any required environment variables for build
NODE_ENV: production
DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres

Comment on lines +50 to 54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "migrate.ts" -type f

Repository: QueueLab/QCX

Length of output: 75


🏁 Script executed:

cat -n ./lib/db/migrate.ts

Repository: QueueLab/QCX

Length of output: 1732


🏁 Script executed:

cat -n .github/workflows/playwright.yml

Repository: QueueLab/QCX

Length of output: 3213


🏁 Script executed:

# Check if there's an EXECUTE_MIGRATIONS variable set in the workflow
grep -n "EXECUTE_MIGRATIONS" .github/workflows/playwright.yml

Repository: QueueLab/QCX

Length of output: 38


🏁 Script executed:

cat -n package.json

Repository: QueueLab/QCX

Length of output: 4904


Fix SSL configuration for localhost Postgres in CI migrations.

lib/db/migrate.ts forces SSL for all connections, but the GitHub Actions postgres:15 service doesn't support SSL by default. This causes bun run db:migrate to fail in CI with a protocol mismatch error. Make SSL conditional on non-localhost connections.

🛠️ Suggested fix for lib/db/migrate.ts
-  const pool = new Pool({
-    connectionString: process.env.DATABASE_URL,
-    ssl: {
-      rejectUnauthorized: false, // Ensure this is appropriate for your Supabase connection
-    },
-  });
+  const connectionString = process.env.DATABASE_URL;
+  const useSsl =
+    !!connectionString && !/localhost|127\.0\.0\.1/.test(connectionString);
+  const pool = new Pool({
+    connectionString,
+    ssl: useSsl ? { rejectUnauthorized: false } : false,
+  });
🧰 Tools
🪛 Checkov (3.2.334)

[medium] 53-54: Basic Auth Credentials

(CKV_SECRET_4)

🤖 Prompt for AI Agents
In @.github/workflows/playwright.yml around lines 50 - 54, lib/db/migrate.ts
currently forces SSL for all DB connections causing CI failures; update the DB
client creation logic (the code that instantiates the Pool/Client used by your
migration runner) to parse the DATABASE_URL (new URL(process.env.DATABASE_URL))
and conditionally set ssl only when the host is not localhost/127.0.0.1 (and not
the Docker/GitHub Actions local service); i.e., if url.hostname is 'localhost'
or '127.0.0.1' then disable ssl, otherwise set ssl: { rejectUnauthorized: true }
(or your existing ssl config) so migrations run in CI against the non-SSL
postgres service.

- name: Start application
run: |
bun run start &
sleep 10
# Wait for the server to be ready
npx wait-on http://localhost:3000 --timeout 60000

- name: Run Playwright tests
run: bun run test:e2e
- name: Run Playwright tests (Shard ${{ matrix.shardIndex }}/${{ matrix.shardTotal }})
run: bunx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }}
env:
CI: true
NODE_ENV: production
AUTH_DISABLED_FOR_DEV: true
Comment on lines 59 to 60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n -C3 'AUTH_DISABLED_FOR_DEV' -t yaml -t js -t ts -t json

Repository: QueueLab/QCX

Length of output: 2661


Remove or clarify AUTH_DISABLED_FOR_DEV: true in the E2E test workflow.

The flag is guarded in the code to only work when NODE_ENV !== 'production'. Since this workflow sets NODE_ENV: production, the AUTH_DISABLED_FOR_DEV variable has no effect and should be removed to avoid confusion. If the intent is to disable auth for E2E tests, either set NODE_ENV: development or remove the flag entirely.

🤖 Prompt for AI Agents
In @.github/workflows/playwright.yml around lines 38 - 42, The workflow
currently sets NODE_ENV: production while also defining AUTH_DISABLED_FOR_DEV:
true which is ineffective and confusing; update the Playwright workflow by
either removing AUTH_DISABLED_FOR_DEV entirely or change NODE_ENV to development
if the intention is to disable auth for E2E tests, and ensure the variable
AUTH_DISABLED_FOR_DEV is only present when NODE_ENV !== 'production'
(referencing the NODE_ENV and AUTH_DISABLED_FOR_DEV entries in the workflow).

DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres

- name: Upload test results
uses: actions/upload-artifact@v4
if: always()
with:
name: playwright-report
path: playwright-report/
retention-days: 30
name: blob-report-${{ matrix.shardIndex }}
path: blob-report/
retention-days: 1

merge-reports:
if: always()
needs: [test]
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v4

- name: Upload test screenshots
- name: Setup Bun
uses: oven-sh/setup-bun@v2
with:
bun-version: 1.3.5

- name: Install dependencies
run: bun install --frozen-lockfile

- name: Download blobs
uses: actions/download-artifact@v4
with:
path: all-blobs
pattern: blob-report-*
merge-multiple: true

- name: Merge reports
run: bunx playwright merge-reports --reporter html ./all-blobs

- name: Upload merged report
uses: actions/upload-artifact@v4
if: failure()
with:
name: playwright-screenshots
path: test-results/
retention-days: 7
name: playwright-report
path: playwright-report/
retention-days: 30
2 changes: 1 addition & 1 deletion lib/agents/resolution-search.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,4 @@ Analyze the user's prompt and the image to provide a holistic understanding of t
messages: filteredMessages,
schema: resolutionSearchSchema,
})
}
}
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
"lint": "next lint",
"db:migrate": "cross-env EXECUTE_MIGRATIONS=true bun lib/db/migrate.ts",
"test:e2e": "playwright test",
"test:e2e:smoke": "playwright test --grep @smoke",
"test:e2e:parallel": "playwright test --workers=4",
"test:e2e:chromium": "playwright test --project=chromium",
"test:e2e:shard": "playwright test --shard=$SHARD_INDEX/$SHARD_TOTAL",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Cross-platform compatibility concern with environment variable syntax.

The $SHARD_INDEX/$SHARD_TOTAL syntax works on Unix-like systems but will fail on Windows. Since the script is primarily used in CI (Linux), this may be acceptable, but consider using cross-env (already a devDependency) for local developer usage if needed.

Proposed fix for cross-platform support (if needed)
-    "test:e2e:shard": "playwright test --shard=$SHARD_INDEX/$SHARD_TOTAL",
+    "test:e2e:shard": "cross-env playwright test --shard=${SHARD_INDEX}/${SHARD_TOTAL}",

Note: This still won't work on native Windows CMD. If Windows support is required, consider a small Node script wrapper.

📝 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
"test:e2e:shard": "playwright test --shard=$SHARD_INDEX/$SHARD_TOTAL",
"test:e2e:shard": "cross-env playwright test --shard=${SHARD_INDEX}/${SHARD_TOTAL}",
🤖 Prompt for AI Agents
In `@package.json` at line 17, The "test:e2e:shard" npm script uses Unix-style env
syntax ($SHARD_INDEX/$SHARD_TOTAL) which breaks on Windows; update the script to
use cross-env to set/consume these variables cross-platform (reference the
"test:e2e:shard" script and the cross-env devDependency) or replace it with a
small Node wrapper that reads process.env.SHARD_INDEX and
process.env.SHARD_TOTAL and invokes Playwright, ensuring the variables are
provided via cross-env in package.json when running locally.

"test:e2e:ui": "playwright test --ui",
"test:e2e:headed": "playwright test --headed",
"test:e2e:debug": "playwright test --debug"
Expand Down
18 changes: 13 additions & 5 deletions playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { defineConfig, devices } from '@playwright/test';

export default defineConfig({
testDir: './tests',
globalSetup: require.resolve('./tests/global-setup'),
fullyParallel: true,
forbidOnly: !!process.env.CI,
retries: process.env.CI ? 2 : 0,
workers: process.env.CI ? 1 : undefined,
reporter: 'html',
workers: process.env.CI ? 2 : undefined,
reporter: process.env.CI ? [['list'], ['github'], ['blob']] : 'html',
use: {
baseURL: 'http://localhost:3000',
trace: 'on-first-retry',
Expand All @@ -15,28 +16,35 @@ export default defineConfig({
{
name: 'chromium',
use: { ...devices['Desktop Chrome'] },
testIgnore: /mobile\.spec\.ts/,
},
{
name: 'firefox',
use: { ...devices['Desktop Firefox'] },
testIgnore: /mobile\.spec\.ts/,
},
{
name: 'webkit',
use: { ...devices['Desktop Safari'] },
testIgnore: /mobile\.spec\.ts/,
},
{
name: 'Mobile Chrome',
use: { ...devices['Pixel 5'] },
testMatch: /mobile\.spec\.ts/,
},
{
name: 'Mobile Safari',
use: { ...devices['iPhone 12'] },
testMatch: /mobile\.spec\.ts/,
},
],
/* webServer: {
command: process.env.CI ? 'npm run build && npm run start' : 'npm run dev',
webServer: {
command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev',
url: 'http://localhost:3000',
reuseExistingServer: !process.env.CI,
timeout: 600000,
}, */
stdout: 'pipe',
stderr: 'pipe',
},
Comment on lines 3 to 49

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set workers: 4 in CI and also shard into 4 jobs. That yields up to 16 concurrent workers (4 jobs × 4 workers) against the same webServer config per job.

This can overload CI resources and make tests less reliable (especially with WebGL/map tests and any shared external dependencies). If the goal is 4-way parallelization total, you typically want either sharding or increased workers, not both.

Suggestion

Decide on one primary scaling mechanism:

  • Option A (recommended for CI predictability): shard in GitHub Actions and keep workers: 1 per shard.
  • Option B: no sharding, keep workers: 4 in a single job.

If you keep both, cap workers when sharding is enabled:

const isSharded = !!process.env.SHARD_INDEX && !!process.env.SHARD_TOTAL;
workers: process.env.CI ? (isSharded ? 1 : 4) : undefined,

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.

Comment on lines 42 to 49

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webServer.command runs bun run build && bun run start. next start typically blocks and does not exit; Playwright’s webServer runner will manage the lifecycle, but only if it can reliably detect readiness via url.

Right now url is hard-coded to http://localhost:3000. If the app is configured to bind a different port (common in CI) or requires env vars, the server may start but never become "ready" to Playwright, causing shard timeouts.

Also, the workflow sets AUTH_DISABLED_FOR_DEV: true and other env, but webServer is what actually starts the app; unless Playwright passes those env vars through (it uses the parent process env), this will work only if the workflow env is present for the Playwright process (it is) — but it’s fragile if someone runs tests locally with different env expectations.

Suggestion

Parameterize baseURL/webServer.url (and optionally port) via process.env.PLAYWRIGHT_BASE_URL / PORT (e.g. const port = process.env.PORT ?? '3000') and set webServer.env explicitly for required flags like AUTH_DISABLED_FOR_DEV. This reduces CI/local drift and avoids hangs when port differs. Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this.

Comment on lines +42 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider reducing the webServer timeout or documenting the rationale.

The 10-minute (600000ms) timeout is quite generous. While build times can vary, this may mask issues where the server fails to start within a reasonable window. If typical build + start times are known, consider:

  • Reducing to a tighter bound (e.g., 180000ms = 3 minutes) with buffer
  • Adding a comment explaining why 10 minutes is necessary

The stdout: 'pipe' and stderr: 'pipe' configuration is excellent for capturing server logs during CI failures.

♻️ Suggested adjustment with documentation
   webServer: {
     command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev',
     url: 'http://localhost:3000',
     reuseExistingServer: !process.env.CI,
-    timeout: 600000,
+    timeout: 180000, // 3 minutes - adjust based on typical build times
     stdout: 'pipe',
     stderr: 'pipe',
   },
📝 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
webServer: {
command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev',
url: 'http://localhost:3000',
reuseExistingServer: !process.env.CI,
timeout: 600000,
}, */
stdout: 'pipe',
stderr: 'pipe',
},
webServer: {
command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev',
url: 'http://localhost:3000',
reuseExistingServer: !process.env.CI,
timeout: 180000, // 3 minutes - adjust based on typical build times
stdout: 'pipe',
stderr: 'pipe',
},
🤖 Prompt for AI Agents
In `@playwright.config.ts` around lines 42 - 49, Reduce the excessive webServer
timeout in playwright.config.ts by setting webServer.timeout to a tighter value
(e.g., 180000 ms) and add a short inline comment explaining the chosen bound
(expected build+start time plus buffer) so reviewers/CI maintainers know why it
isn’t the default; retain stdout: 'pipe' and stderr: 'pipe' as-is to keep log
capture. Ensure you update the webServer block (the timeout field) and add the
comment next to the webServer configuration entry.

});
60 changes: 30 additions & 30 deletions tests/calendar.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import { test, expect } from '@playwright/test';
import { test, expect } from './fixtures';

test.describe('Calendar functionality', () => {
test.beforeEach(async ({ page }) => {
await page.goto('/');
await page.waitForSelector('[data-testid="calendar-toggle"]');
});

test('should open and close the calendar', async ({ page }) => {
test.describe('Calendar functionality @smoke @calendar', () => {
test('should open and close the calendar', async ({ authenticatedPage: page }) => {
// Open calendar
await page.click('[data-testid="calendar-toggle"]');
const calendar = page.locator('[data-testid="calendar-notepad"]');
Expand All @@ -17,7 +12,7 @@ test.describe('Calendar functionality', () => {
await expect(calendar).not.toBeVisible();
});

test('should display current month and year', async ({ page }) => {
test('should display current month and year', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

const currentDate = new Date();
Expand All @@ -30,7 +25,7 @@ test.describe('Calendar functionality', () => {
await expect(calendarHeader).toContainText(monthYear);
});

test('should navigate to previous month', async ({ page }) => {
test('should navigate to previous month', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

const initialMonth = await page.locator('[data-testid="calendar-header"]').innerText();
Expand All @@ -41,7 +36,7 @@ test.describe('Calendar functionality', () => {
expect(newMonth).not.toBe(initialMonth);
});

test('should navigate to next month', async ({ page }) => {
test('should navigate to next month', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

const initialMonth = await page.locator('[data-testid="calendar-header"]').innerText();
Expand All @@ -52,7 +47,7 @@ test.describe('Calendar functionality', () => {
expect(newMonth).not.toBe(initialMonth);
});

test('should select a date', async ({ page }) => {
test('should select a date', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

// Click on a specific date (e.g., the 15th of the current month)
Expand All @@ -63,43 +58,46 @@ test.describe('Calendar functionality', () => {
await expect(dateButton).toHaveClass(/selected|active/);
});

test('should add a note to a selected date', async ({ page }) => {
test('should add a note to a selected date', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

// Select a date
await page.click('[data-testid="calendar-day-15"]');

// Add a note
// Add a unique note to avoid conflicts during parallel execution
const uniqueNote = `Important meeting ${Date.now()}`;
const noteInput = page.locator('[data-testid="calendar-note-input"]');
await noteInput.fill('Important meeting at 2 PM');
await noteInput.fill(uniqueNote);

await page.click('[data-testid="calendar-save-note"]');

// Verify the note is saved
const savedNote = page.locator('[data-testid="calendar-note-content"]');
await expect(savedNote).toContainText('Important meeting at 2 PM');
await expect(savedNote).toContainText(uniqueNote);
});

test('should edit an existing note', async ({ page }) => {
test('should edit an existing note', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

// Select a date and add a note
await page.click('[data-testid="calendar-day-15"]');
await page.fill('[data-testid="calendar-note-input"]', 'Original note');
const originalNote = `Original note ${Date.now()}`;
await page.fill('[data-testid="calendar-note-input"]', originalNote);
await page.click('[data-testid="calendar-save-note"]');

// Edit the note
await page.click('[data-testid="calendar-edit-note"]');
await page.fill('[data-testid="calendar-note-input"]', 'Updated note');
const updatedNote = `Updated note ${Date.now()}`;
await page.fill('[data-testid="calendar-note-input"]', updatedNote);
await page.click('[data-testid="calendar-save-note"]');

// Verify the note is updated
const updatedNote = page.locator('[data-testid="calendar-note-content"]');
await expect(updatedNote).toContainText('Updated note');
await expect(updatedNote).not.toContainText('Original note');
const updatedNoteElement = page.locator('[data-testid="calendar-note-content"]');
await expect(updatedNoteElement).toContainText(updatedNote);
await expect(updatedNoteElement).not.toContainText(originalNote);
});

test('should delete a note', async ({ page }) => {
test('should delete a note', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

// Select a date and add a note
Expand All @@ -116,12 +114,13 @@ test.describe('Calendar functionality', () => {
await expect(noteContent).not.toBeVisible();
});

test('should persist notes after closing and reopening calendar', async ({ page }) => {
test('should persist notes after closing and reopening calendar', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

// Add a note
// Add a unique note
await page.click('[data-testid="calendar-day-15"]');
await page.fill('[data-testid="calendar-note-input"]', 'Persistent note');
const persistentNoteText = `Persistent note ${Date.now()}`;
await page.fill('[data-testid="calendar-note-input"]', persistentNoteText);
await page.click('[data-testid="calendar-save-note"]');

// Close calendar
Expand All @@ -133,22 +132,23 @@ test.describe('Calendar functionality', () => {

// Verify the note is still there
const persistedNote = page.locator('[data-testid="calendar-note-content"]');
await expect(persistedNote).toContainText('Persistent note');
await expect(persistedNote).toContainText(persistentNoteText);
});

test('should highlight dates with notes', async ({ page }) => {
test('should highlight dates with notes', async ({ authenticatedPage: page }) => {
await page.click('[data-testid="calendar-toggle"]');

// Add a note to a date
await page.click('[data-testid="calendar-day-15"]');
await page.fill('[data-testid="calendar-note-input"]', 'Test note');
await page.fill('[data-testid="calendar-note-input"]', 'Highlight test note');
await page.click('[data-testid="calendar-save-note"]');

// Close the note panel
await page.click('[data-testid="calendar-close-note"]');

// Verify the date has a visual indicator (dot, badge, etc.)
// Verify the date has a visual indicator
const dateWithNote = page.locator('[data-testid="calendar-day-15"]');
await expect(dateWithNote).toHaveClass(/has-note|noted/);
});

});
Loading