test(jef-122): add E2E coverage for application-detail, calendar, analytics, offers, notifications, and share links - #309
Conversation
…lytics, offers, notifications, and share links
Adds 6 new Playwright specs covering previously-untested apps/web flows:
application-detail (notes/contacts/interviews/documents CRUD), offers
(add + compare), calendar (interview round on today's view), analytics
(totals against created applications), notifications (a real
cross-device "New sign-in detected" alert), and share-links
(authenticated create -> anonymous view via a second browser context).
Introduces e2e/helpers/{auth,applications}.ts shared across specs, and
fixes the 4 pre-existing specs (auth/applications/account/a11y), which
were silently broken: registration no longer auto-authenticates into
the app (it shows a "Check your email" screen), so every spec now goes
through a real register-then-sign-in flow; several locators had never
actually been exercised (getByLabel against unlabeled inputs, getByRole
'button' against what are actually links) since the specs always failed
before reaching them; and a11y.spec.ts's `waitForLoadState('networkidle')`
never resolved (something keeps a connection open indefinitely), so its
scans had never actually executed — swapped for waiting on rendered UI.
Along the way, found and fixed four real, reproducible app bugs
surfaced by driving the app end-to-end for the first time:
- EditApplicationPage imported the wrong route's `Route` object,
so `Route.useParams()` always threw — the edit page was completely
broken for every user.
- VercelBlobStorageProvider read a `BLOB_PUBLIC_READ_WRITE_TOKEN` env
var that is never documented or set anywhere (only the unused
`BLOB_READ_WRITE_TOKEN` is) — document upload was broken everywhere.
- CreateSessionUseCase queried known user-agents *after* inserting the
new session, so the just-created session always self-matched as
"known" — the new-device security alert could never fire, for any
login, ever.
- calendar.tsx and dashboard.tsx's calendarEvents queries shared the
same React Query cache key despite different query shapes; combined
with the 60s default staleTime, the calendar page could silently
serve the dashboard widget's stale (pre-event) cached result instead
of fetching fresh data.
Also playwright.config.ts now pins workers to 1 — the whole suite
shares one dev API server + one local SQLite file, and running spec
files in parallel overloaded that single server (tests pass in
isolation but fail/timeout en masse under concurrent load).
Two findings are filed as follow-ups rather than fixed here, since
they're open-ended beyond this issue's E2E-coverage scope:
- JEF-124: a11y.spec.ts's axe-core scans, now that they actually run,
surface ~16 real violation categories across the app.
- JEF-125: the notification inbox modal isn't portaled, so clicking
its "Mark read" button can click through to the sidebar nav
underneath instead.
WalkthroughThe pull request updates session device detection, documents the Vercel Blob token configuration, stabilises Playwright execution, refreshes existing E2E flows, fixes route and cache handling, and adds coverage for analytics, application details, calendar, notifications, offers, and share links. ChangesAPI runtime corrections
Web end-to-end coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/web/e2e/analytics.spec.ts (1)
40-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the chart and funnel data.
Lines 40-42 only confirm that the headings render. The test passes if the weekly chart or stage funnel contains incorrect values. Assert the rendered data for the two created applications and their statuses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/e2e/analytics.spec.ts` around lines 40 - 42, Extend the analytics assertions after the “Applications per week” and “Stage funnel” heading checks to validate the rendered counts and status values for both created applications. Use the existing application fixtures and visible chart/funnel labels, preserving the “No data yet.” absence assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/use-cases/sessions/CreateSessionUseCase.ts`:
- Around line 33-39: The device-history lookup in CreateSessionUseCase.execute
must not prevent session creation when findDistinctUserAgentsByUserId rejects.
Represent lookup failure separately from an empty first-session result, continue
to sessionRepository.create, and skip device-alert evaluation when the snapshot
is unavailable; retain the existing post-creation error handling. Add a
regression test covering the rejected lookup and successful session creation.
In `@apps/web/e2e/helpers/auth.ts`:
- Around line 4-5: Update uniqueEmail to use nanoid() as the generated
identifier segment instead of combining Date.now() with truncated Math.random(),
while preserving the prefix and `@e2e.example.com` email format.
In `@apps/web/e2e/notifications.spec.ts`:
- Around line 17-21: Configure the secondary browser context created in the
secondDeviceContext setup with the test’s existing baseURL before calling
secondPage.goto('/login'), so the relative navigation resolves correctly.
In `@apps/web/e2e/offers.spec.ts`:
- Around line 51-55: Update the offer assertions in the test to scope the `Best`
marker to the `$150,000/yearly` row, and explicitly verify that the lower-offer
row does not contain `Best`. Keep the existing row-count assertion and use
row-specific locators so reversed ranking cannot pass.
---
Nitpick comments:
In `@apps/web/e2e/analytics.spec.ts`:
- Around line 40-42: Extend the analytics assertions after the “Applications per
week” and “Stage funnel” heading checks to validate the rendered counts and
status values for both created applications. Use the existing application
fixtures and visible chart/funnel labels, preserving the “No data yet.” absence
assertion.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15a9131a-3592-4e83-8995-a12a12beee48
📒 Files selected for processing (18)
apps/api/src/infrastructure/storage/VercelBlobStorageProvider.tsapps/api/src/use-cases/sessions/CreateSessionUseCase.tsapps/web/.gitignoreapps/web/e2e/a11y.spec.tsapps/web/e2e/account.spec.tsapps/web/e2e/analytics.spec.tsapps/web/e2e/application-detail.spec.tsapps/web/e2e/applications.spec.tsapps/web/e2e/auth.spec.tsapps/web/e2e/calendar.spec.tsapps/web/e2e/helpers/applications.tsapps/web/e2e/helpers/auth.tsapps/web/e2e/notifications.spec.tsapps/web/e2e/offers.spec.tsapps/web/e2e/share-links.spec.tsapps/web/playwright.config.tsapps/web/src/routes/_authenticated/applications/$applicationId/-components/EditApplicationPage.tsxapps/web/src/routes/_authenticated/calendar.tsx
| // Snapshot known user-agents *before* inserting this session — querying | ||
| // after insertion would always find this session's own userAgent already | ||
| // persisted, making every login look like a "known" device. | ||
| const knownUserAgents = input.userAgent | ||
| ? await this.deps.sessionRepository.findDistinctUserAgentsByUserId(input.userId) | ||
| : []; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not let the device-history lookup block session creation.
The await at Line [37] runs before sessionRepository.create at Line [40]. If the repository read rejects, execute rejects and no session is created. The .catch(() => {}) at Line [59] only handles failures after session creation.
Catch this lookup failure and continue creating the session. Skip only device alerting when the snapshot is unavailable. If [] must remain reserved for “first session”, use a separate unavailable state.
Add a regression test that rejects findDistinctUserAgentsByUserId and verifies that session creation still succeeds.
As per coding guidelines, every new or changed use case must ship with matching tests in the same change.
Proposed fix
- const knownUserAgents = input.userAgent
- ? await this.deps.sessionRepository.findDistinctUserAgentsByUserId(input.userId)
- : [];
+ let knownUserAgents: string[] = [];
+ if (input.userAgent) {
+ try {
+ knownUserAgents =
+ await this.deps.sessionRepository.findDistinctUserAgentsByUserId(input.userId);
+ } catch {
+ // Device detection is best-effort; do not block session creation.
+ }
+ }📝 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.
| // Snapshot known user-agents *before* inserting this session — querying | |
| // after insertion would always find this session's own userAgent already | |
| // persisted, making every login look like a "known" device. | |
| const knownUserAgents = input.userAgent | |
| ? await this.deps.sessionRepository.findDistinctUserAgentsByUserId(input.userId) | |
| : []; | |
| // Snapshot known user-agents *before* inserting this session — querying | |
| // after insertion would always find this session's own userAgent already | |
| // persisted, making every login look like a "known" device. | |
| let knownUserAgents: string[] = []; | |
| if (input.userAgent) { | |
| try { | |
| knownUserAgents = | |
| await this.deps.sessionRepository.findDistinctUserAgentsByUserId(input.userId); | |
| } catch { | |
| // Device detection is best-effort; do not block session creation. | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/use-cases/sessions/CreateSessionUseCase.ts` around lines 33 -
39, The device-history lookup in CreateSessionUseCase.execute must not prevent
session creation when findDistinctUserAgentsByUserId rejects. Represent lookup
failure separately from an empty first-session result, continue to
sessionRepository.create, and skip device-alert evaluation when the snapshot is
unavailable; retain the existing post-creation error handling. Add a regression
test covering the rejected lookup and successful session creation.
Source: Coding guidelines
| export const uniqueEmail = (prefix: string) => | ||
| `${prefix}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}@e2e.example.com`; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that nanoid is installed and inspect existing TypeScript usage.
fd -H -t f '^(package\.json|pnpm-lock\.yaml)$' . -x rg -n '"nanoid"|nanoid@' {}
rg -n --glob '*.ts' --glob '*.tsx' "from 'nanoid'|nanoid\\(" apps/webRepository: mankatcheung/job-finder
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Changed files:"
git diff -- --name-only
echo
echo "apps/web/e2e/helpers/auth.ts context:"
sed -n '1,80p' apps/web/e2e/helpers/auth.ts
echo
echo "Search nanoid usage across repository TypeScript/TSX:"
rg -n --glob '*.ts' --glob '*.tsx' "from ['\"]nanoid['\"]|nanoid\\(" . || true
echo
echo "Package manifests mentioning nanoid:"
git ls-files 'package.json' 'pnpm-lock.yaml' 'apps/web/package.json' 'apps/web/package*.json' 2>/dev/null | while read -r f; do
echo "--- $f"
rg -n '"nanoid"|nanoid@' "$f" || true
done
echo
echo "Inspect nanoid dependency declarations:"
python3 - <<'PY'
import json, pathlib, re
for path in pathlib.Path('.').rglob('package.json'):
if any(part in {'.git','node_modules'} for part in path.parts):
continue
data=json.loads(path.read_text())
deps={}
for key in ('dependencies','devDependencies','peerDependencies'):
if key in data:
deps.update(data[key])
for name in ('nanoid','`@esbuild`','typescript'):
if name in deps:
print(f"{path}: {name}: {deps[name]}")
PYRepository: mankatcheung/job-finder
Length of output: 4124
Use nanoid() for the generated E2E email identifier.
uniqueEmail combines Date.now() with a truncated Math.random() value. Use nanoid() for the identifier segment to follow the identifier guideline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/e2e/helpers/auth.ts` around lines 4 - 5, Update uniqueEmail to use
nanoid() as the generated identifier segment instead of combining Date.now()
with truncated Math.random(), while preserving the prefix and `@e2e.example.com`
email format.
Source: Coding guidelines
| const secondDeviceContext = await browser.newContext({ | ||
| userAgent: 'Mozilla/5.0 (E2E second device; secondary-browser/1.0)', | ||
| }); | ||
| const secondPage = await secondDeviceContext.newPage(); | ||
| await secondPage.goto('/login'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 '\bbaseURL\b' apps/web/playwright.config.ts
rg -n -C 4 'browser\.newContext|secondPage\.goto' apps/web/e2e/notifications.spec.tsRepository: mankatcheung/job-finder
Length of output: 1205
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Package playwright version:\n'
if [ -f package.json ]; then jq -r '.dependencies["`@playwright/test`"], .devDependencies["`@playwright/test`"]' package.json; fi
if [ -f apps/web/package.json ]; then jq -r '.dependencies["`@playwright/test`"], .devDependencies["`@playwright/test`"]' apps/web/package.json; fi
printf '\nCandidate fixture/baseURL code:\n'
rg -n -C 3 'test\.extend|\.extend|fixtures|baseURL|BrowserContext|Browser' -g '*.ts' -g '*.cts' -g '*.mts' | head -n 200
printf '\n notifications spec header:\n'
sed -n '1,35p' apps/web/e2e/notifications.spec.tsRepository: mankatcheung/job-finder
Length of output: 245
🌐 Web query:
Playwright 1.61 BrowserContext newPage relative URLs baseURL fixtures browser fixture baseURL
💡 Result:
In Playwright, baseURL facilitates relative URL navigation (e.g., page.goto('/path')) by resolving it against a configured base URL [1][2]. Its behavior when creating new pages or contexts depends on how those entities are initialized. When you create a BrowserContext or Page manually (e.g., via browser.newContext or browser.newPage), Playwright does not automatically inherit the baseURL defined in your Playwright Test configuration [3]. If you want relative URLs to work in manually created contexts or pages, you must explicitly pass the baseURL option to the creation method [3][4]. For example, when creating a new page in a context: const context = await browser.newContext({ baseURL: 'http://localhost:3000' }); const page = await context.newPage; await page.goto('/relative-path'); // Navigates to http://localhost:3000/relative-path When using Playwright Test fixtures, the built-in page fixture is already pre-configured to use the baseURL defined in your configuration [3]. If you create custom fixtures that involve creating your own pages or contexts, you must ensure the baseURL is passed to the context or page options to maintain this functionality [3][4]. Regarding Playwright 1.61: The 1.61 release primarily introduced new features such as virtual WebAuthn authenticator support via browserContext.credentials [5]. While baseURL remains a fundamental configuration option, if you are working with custom fixtures, you can override or access the baseURL fixture like any other test fixture to ensure your custom logic (like starting a dev server) aligns with your navigation requirements [2][6][7]. If you encounter issues with overriding baseURL, note that Playwright Test enforces strict rules on overriding built-in options; using the standard fixture extension pattern is the supported approach [6][8].
Citations:
- 1: https://playwright.dev/docs/api/class-browser
- 2: https://playwright.dev/docs/test-use-options
- 3: [BUG] Documentation: need specify baseURL in fixture microsoft/playwright#19264
- 4: https://stackoverflow.com/questions/76568934/playwright-baseurl-is-getting-ignored-when-using-custom-test-fixture
- 5: https://playwright.dev/docs/release-notes
- 6: https://playwright.dev/docs/test-fixtures
- 7: [Question] Can I set the baseURL from inside a worker fixture? microsoft/playwright#19560
- 8: [Regression]: Unable to override baseURL since 1.60.0 microsoft/playwright#41757
Configure baseURL for the secondary browser context.
browser.newContext() creates a BrowserContext outside the first test page. That context does not inherit the Playwright Test use.baseURL, so secondPage.goto('/login') cannot resolve the relative URL; pass the configured baseURL into the created context.
Proposed fix
test('shows a real "New sign-in detected" alert triggered by a second device', async ({
browser,
+ baseURL,
page,
}) => {
@@
const secondDeviceContext = await browser.newContext({
+ baseURL,
userAgent: 'Mozilla/5.0 (E2E second device; secondary-browser/1.0)',
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const secondDeviceContext = await browser.newContext({ | |
| userAgent: 'Mozilla/5.0 (E2E second device; secondary-browser/1.0)', | |
| }); | |
| const secondPage = await secondDeviceContext.newPage(); | |
| await secondPage.goto('/login'); | |
| test('shows a real "New sign-in detected" alert triggered by a second device', async ({ | |
| browser, | |
| baseURL, | |
| page, | |
| }) => { | |
| const secondDeviceContext = await browser.newContext({ | |
| baseURL, | |
| userAgent: 'Mozilla/5.0 (E2E second device; secondary-browser/1.0)', | |
| }); | |
| const secondPage = await secondDeviceContext.newPage(); | |
| await secondPage.goto('/login'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/e2e/notifications.spec.ts` around lines 17 - 21, Configure the
secondary browser context created in the secondDeviceContext setup with the
test’s existing baseURL before calling secondPage.goto('/login'), so the
relative navigation resolves correctly.
| await expect(page.getByText('Acme Corp').first()).toBeVisible(); | ||
| await expect(page.getByText('Best')).toBeVisible(); | ||
| // Both rows are for the same application — the higher offer wins. | ||
| const rows = page.locator('tbody tr'); | ||
| await expect(rows).toHaveCount(2); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the higher offer has the Best marker.
The current assertions only confirm that one row has Best. A reversed ranking can still pass this test. Scope the marker assertion to the $150,000/yearly row and verify that the lower offer does not have it.
Proposed test change
const rows = page.locator('tbody tr');
await expect(rows).toHaveCount(2);
+ const higherOfferRow = rows.filter({ hasText: '$150,000/yearly' });
+ const lowerOfferRow = rows.filter({ hasText: '$130,000/yearly' });
+ await expect(higherOfferRow.getByText('Best')).toBeVisible();
+ await expect(lowerOfferRow.getByText('Best')).not.toBeVisible();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(page.getByText('Acme Corp').first()).toBeVisible(); | |
| await expect(page.getByText('Best')).toBeVisible(); | |
| // Both rows are for the same application — the higher offer wins. | |
| const rows = page.locator('tbody tr'); | |
| await expect(rows).toHaveCount(2); | |
| await expect(page.getByText('Acme Corp').first()).toBeVisible(); | |
| await expect(page.getByText('Best')).toBeVisible(); | |
| // Both rows are for the same application — the higher offer wins. | |
| const rows = page.locator('tbody tr'); | |
| await expect(rows).toHaveCount(2); | |
| const higherOfferRow = rows.filter({ hasText: '$150,000/yearly' }); | |
| const lowerOfferRow = rows.filter({ hasText: '$130,000/yearly' }); | |
| await expect(higherOfferRow.getByText('Best')).toBeVisible(); | |
| await expect(lowerOfferRow.getByText('Best')).not.toBeVisible(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/e2e/offers.spec.ts` around lines 51 - 55, Update the offer
assertions in the test to scope the `Best` marker to the `$150,000/yearly` row,
and explicitly verify that the lower-offer row does not contain `Best`. Keep the
existing row-count assertion and use row-specific locators so reversed ranking
cannot pass.
… instead BLOB_PUBLIC_READ_WRITE_TOKEN (not BLOB_READ_WRITE_TOKEN) is the correct env var here — it's a public-access Blob store token, by design. My earlier commit on this branch swapped the provider to read BLOB_READ_WRITE_TOKEN instead, based on it being the only one documented in .env.example; that was the wrong fix. Reverting the provider and documenting BLOB_PUBLIC_READ_WRITE_TOKEN in .env.example instead, which was the actual gap.
Summary
Adds 6 new Playwright E2E specs covering previously-untested
apps/webflows:application-detail.spec.ts— notes, contacts, interview rounds (add + mark outcome), and document upload/delete on the application detail pageoffers.spec.ts— add offers, compare view with best-offer highlightingcalendar.spec.ts— an interview round scheduled today shows up on the Day viewanalytics.spec.ts— dashboard totals/charts reflect real created applicationsnotifications.spec.ts— a genuinely-triggered "New sign-in detected" alert (two browser contexts, different User-Agent)share-links.spec.ts— authenticated create → anonymous view via a second, unauthenticated browser contextAdds
e2e/helpers/{auth,applications}.ts, shared across specs.Fixing the existing suite
The 4 pre-existing specs (
auth,applications,account,a11y) were silently broken and needed fixing before any of the above could be verified:registerAndLoginhelper.getByLabelagainst inputs with no label association,getByRole('button')against what are actually<a>links) — always masked by the beforeEach failing first.a11y.spec.ts'swaitForLoadState('networkidle')never resolved, so its axe scans had never actually run — swapped for waiting on rendered UI (a Playwright best practice regardless).Real bugs found and fixed along the way
Driving the app end-to-end for the first time surfaced three real, reproducible bugs:
EditApplicationPageimported the wrong route'sRouteobject —Route.useParams()always threw, so the edit-application page was completely broken for every user.CreateSessionUseCasequeried known user-agents after inserting the new session, so a session always found itself already "known" — the new-device security alert could never fire, for any login, ever.calendar.tsxanddashboard.tsxshared a React Query cache key for differently-shapedcalendarEventsqueries; combined with the 60s defaultstaleTime, the calendar page could silently serve the dashboard widget's stale (pre-event) cached result.Also documented
BLOB_PUBLIC_READ_WRITE_TOKENin.env.example(it was missing there, even though it's the correct/intended varVercelBlobStorageProviderreads for public Blob storage — an early commit on this branch mistakenly "fixed" this by pointing the provider at the wrong var instead; reverted, see the follow-up commit).playwright.config.tsnow pinsworkers: 1— the whole suite shares one dev API server + one local SQLite file, and running spec files in parallel overloaded that single server (each file passes in isolation but the combined run failed/timed out en masse under concurrent load).Filed as follow-ups (out of scope here)
a11y.spec.ts's axe-core scans, now that they actually run, surface ~16 real violation categories across the app. Real remediation project, not an E2E-coverage task.document.body; clicking its "Mark read" button can click through to the sidebar nav underneath instead.notifications.spec.tsverifies the notification renders correctly but doesn't exercise that broken interaction.Test plan
pnpm typecheck(apps/web, apps/api) — cleanpnpm lint(apps/web, apps/api) — clean (only pre-existing unrelated warnings)pnpm test(apps/api, Vitest) — 1201/1201 passingpnpm --filter @job-finder/web test:e2eexcludinga11y.spec.ts(tracked separately in JEF-124) — 30/30 passing, run twice for stabilitya11y.spec.ts— infra fix verified (scans now execute); assertions intentionally left red pending JEF-124🤖 Generated with Claude Code
https://claude.ai/code/session_01FioLhY9DMwRYGVW5H9D3Lu
Summary by CodeRabbit
Bug Fixes
Tests