[STU-25] Staff request fulfillment — pipeline, CV export, work log appeals, payments detail, hub content#8
Merged
Conversation
Implement the full request-to-shortlist data layer for STU-25: - invitation-actions: createInvitationAction, updateInvitationStatusAction - interview-actions: scheduleInterviewAction, updateInterviewAction - application-actions: transitionApplicationAction with optional note - story-actions: createStoryAction, updateStoryStatusAction - candidate-actions: addCandidateNote/Tag/Warning, removeCandidateTag - MatchActions: Suggest form + Invite button on matched candidate cards - StageActions: inline status transitions on all pipeline cards - RequestFulfillmentOS: wired all actions into 6-card pipeline with story creation form and conditional status action buttons All mutations write legacy-compatible rows. All views capability-scoped by staff assignment via requireCapability(). TypeScript: zero errors | ESLint: zero warnings | Build: compiles Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n, import aliases - Add staff ownership verification in interview/invitation/story update actions - Validate interview date before constructing Date object - Remove application_uuid fallback to interviewUuid - Rename scopedCandidateIds to staffCandidateIds for clarity Co-Authored-By: Paperclip <noreply@paperclip.ing>
These are generated test artifacts that should not be tracked. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…work log appeals, payments detail, hub content, smoke tests Includes: - Pipeline server actions (invitation, application, interview transitions) - CV document bundle export with scope-enforced HTML renderer - Work log approval/rejection and appeal resolution server actions - Candidate payment detail page with transfer info - HubContent component extracted for reusability - Playwright smoke test suite for login, search, requests, portals, self-service - Test fixture discovery and validation scripts - Workspace OS updates for request fulfillment and stage actions Co-Authored-By: Paperclip <noreply@paperclip.ing>
WalkthroughThis PR establishes comprehensive end-to-end test infrastructure using Playwright and completes major gaps in candidate/request fulfillment surfaces. It adds test automation, refactors the hub/app page with role-scoped guides and command palettes, implements staff-side candidate and work log management, enables CV export, and expands the request pipeline with interview evaluations and narrative tracking. ChangesE2E testing and feature completion
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Replace dead <Link> to "Export CVs" with the live <ExportCVsForm> component, passing selected candidate IDs as props. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers dashboard rendering, candidate suggestion, invitation, status transitions, story creation/completion, and desk grid error-free rendering — all authenticated as staff. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/workspace/WorkTabs.tsx (1)
89-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSide effect inside state updater can cause issues in concurrent rendering.
Calling
router.push()inside thesetTabsupdater is a side effect in what should be a pure function. React may call state updaters multiple times (StrictMode, concurrent features), potentially causing multiple navigations or unpredictable behavior.Consider moving the navigation outside the updater:
🛠️ Proposed fix
const closeTab = useCallback( (path: string) => { + let nextPath: string | null = null; setTabs((prev) => { const next = prev.filter((t) => t.path !== path); writeTabs(next); - if (pathname === path && next.length > 0) { - router.push(next[0].path as Route); + if (pathname === path && next.length > 0) { + nextPath = next[0].path; } return next; }); + // Navigate after state update completes + if (nextPath) { + router.push(nextPath as Route); + } }, [pathname, router] );Note: The variable assignment approach works because
setTabswith a callback is synchronous in terms of when the callback executes, but if you want to be fully safe with concurrent React, consider usingflushSyncor auseEffectthat watches for tab changes.🤖 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 `@src/modules/workspace/WorkTabs.tsx` around lines 89 - 101, The closeTab updater currently performs a side effect (router.push) inside the setTabs callback which must be a pure function; move navigation out of the state updater in the closeTab function: compute the new tabs array first (using the existing filter logic from closeTab), call writeTabs and setTabs(next) with that array, and then if pathname === path and next.length > 0 call router.push(next[0].path as Route) outside the setTabs callback (or, alternatively, set a ref/flag and perform the navigation in a useEffect or via flushSync to avoid concurrent React re-run issues). Ensure you update the closeTab function and keep writeTabs usage intact.src/modules/requests/RequestFulfillmentOS.tsx (1)
190-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing stage anchors for pipeline navigation.
Line 92 links to
#${stage.id}, but the cards on Line 190, Line 213, and Line 236 have no matchingid. Those stage links can’t scroll to a target.Suggested fix
- <Card className="requestPanel"> + <Card className="requestPanel" id="applications"> ... - <Card className="requestPanel"> + <Card className="requestPanel" id="interviews"> ... - <Card className="requestPanel"> + <Card className="requestPanel" id="stories">🤖 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 `@src/modules/requests/RequestFulfillmentOS.tsx` around lines 190 - 236, The Inbound (Applications), Evaluation (Interviews) and Stories cards lack id attributes that match the stage anchor links (`#${stage.id}`), so add the corresponding id props to those Card elements (the Card wrapping the Applications RequestRows, the Card wrapping the Interviews RequestRows, and the Card for Stories) so the stage anchor links can scroll to them; locate the Cards in RequestFulfillmentOS.tsx (the blocks using RequestRows with ApplicationStatusActions and InterviewStatusActions) and set id={stage.id} (or the appropriate stage identifier from props/data) on each Card to match the anchor target.
🧹 Nitpick comments (6)
src/modules/candidates/CandidateEditForm.tsx (1)
314-318: ⚡ Quick winRedundant hidden
typefield.Line 317 sets a hidden
typefield, but the server actionuploadDocumentnow derives the type from the file input name (file_${type}) and no longer reads a separatetypefield. This hidden input is now dead code.♻️ Remove redundant hidden field
function DocumentUpload({ label, type, current, }: { label: string; type: string; current: string | null; }) { return ( <fieldset className="documentUploadField"> <legend>{label}</legend> - <input type="hidden" name="type" value={type} /> <input type="file" name={`file_${type}`} accept={acceptFor(type)} />🤖 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 `@src/modules/candidates/CandidateEditForm.tsx` around lines 314 - 318, Remove the redundant hidden input element that sets name="type" in CandidateEditForm.tsx: the server action uploadDocument now derives the document type from the file input name (`file_${type}`), so delete the <input type="hidden" name="type" value={type} /> element (and ensure there are no remaining references to this hidden field elsewhere in CandidateEditForm or related handlers like uploadDocument).src/modules/candidates/actions.ts (1)
615-635: 💤 Low valueHard delete for warnings vs. soft delete pattern used elsewhere.
removeStaffCandidateWarningusesprisma.candidate_warning.delete()(hard delete), while tags and skills use soft delete (deleted: 1). If audit trails or undo functionality are needed for warnings, this inconsistency may cause issues.If intentional, no change needed; otherwise consider soft delete for consistency.
🤖 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 `@src/modules/candidates/actions.ts` around lines 615 - 635, The removeStaffCandidateWarning function currently performs a hard delete via prisma.candidate_warning.delete which is inconsistent with the soft-delete pattern used elsewhere (e.g., tags/skills which set deleted: 1); change the deletion to a soft-delete update (use prisma.candidate_warning.update to set deleted: 1 and any audit fields like deleted_at/deleted_by if present) and adjust any subsequent logic that expects existence (revalidatePath calls can remain); ensure you reference prisma.candidate_warning.update and the deleted field so behavior matches other entities.e2e/smoke/login.spec.ts (1)
28-38: ⚡ Quick winAvoid permanently skipped coverage for logged-in redirect.
Line 37 unconditionally skips the test, so this behavior is never validated. Convert this to a real gated test (or
test.fixmewith a tracking reference) to avoid false smoke coverage.🤖 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 `@e2e/smoke/login.spec.ts` around lines 28 - 38, The test named "session redirects logged-in user away from login" currently has an unconditional test.skip() that prevents validation; change it to a gated test by replacing the unconditional test.skip() with either a conditional test.fixme/skip that only runs in CI without DB (e.g., if (process.env.CI && !process.env.PLAYWRIGHT_DB_TESTS) { test.fixme("requires DB fixture"); return; }) or implement the actual test logic that sets a valid session cookie and asserts navigation from /login to /app; update the test block referenced by the string "session redirects logged-in user away from login" accordingly so the behavior is covered or explicitly marked as FIXME with a tracking reference.src/modules/candidates/worklog-actions.ts (1)
136-159: ⚡ Quick winUse proper Prisma types instead of
any[]for the transaction.The explicit
any[]type disables TypeScript's type checking for the transaction operations. Prisma's$transactionmethod properly infers types fromPrismaPromise<T>[].♻️ Suggested fix using Prisma types
- const operations: any[] = [ + const operations: Parameters<typeof prisma.$transaction>[0] = [ prisma.candidate_working_hour_appeal.update({ where: { appeal_uuid: appealUuid }, data: { status: newStatus, updated_at: now }, }), ];Alternatively, import
PrismaPromisefrom@prisma/client:import { PrismaPromise } from "`@prisma/client`"; const operations: PrismaPromise<unknown>[] = [ // ... ];🤖 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 `@src/modules/candidates/worklog-actions.ts` around lines 136 - 159, The operations array is typed as any[], disabling TS checks; change it to use PrismaPromise types by importing PrismaPromise from `@prisma/client` and typing operations as PrismaPromise<unknown>[] (or a more specific PrismaPromise<ReturnType>[] if you want exact results) so the calls to prisma.candidate_working_hour_appeal.update and prisma.candidate_working_hour_appeal_updates.create are statically checked before calling prisma.$transaction(operations).src/modules/candidates/export-actions.ts (1)
54-57: 💤 Low valueConsider graceful handling of individual candidate fetch failures.
Promise.allwill reject entirely if anygetCandidateDetailcall fails. For a bulk export, partial success with a warning may be preferable to complete failure.Use Promise.allSettled for resilience
- const details = await Promise.all( - scopedIds.map((id) => getCandidateDetail(id)), - ); + const results = await Promise.allSettled( + scopedIds.map((id) => getCandidateDetail(id)), + ); + const details = results + .filter((r): r is PromiseFulfilledResult<Awaited<ReturnType<typeof getCandidateDetail>>> => r.status === "fulfilled") + .map((r) => r.value); + + if (!details.length) { + return new Response("Failed to fetch candidate details.", { status: 500 }); + }🤖 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 `@src/modules/candidates/export-actions.ts` around lines 54 - 57, The Promise.all call that fetches candidate details (scopedIds.map(id => getCandidateDetail(id))) will fail-fast; replace it with Promise.allSettled to collect per-id results, keep all fulfilled values for the export (assign to details), log or surface warnings for rejected entries including the candidate id and error, and handle the case where all fetches fail by throwing or returning an appropriate error; update references to details in this module (export-actions.ts, getCandidateDetail, scopedIds) to operate on the filtered fulfilled results.src/app/candidate/payments/page.tsx (1)
18-18: ⚡ Quick winAvoid
as unknown as Route; it bypasses typed-route safety.Use a direct
Routeassertion on the final template string instead of erasing the type first.Suggested refactor
- rowHref={(row) => `/candidate/payments/${row.id}` as unknown as Route} + rowHref={(row) => `/candidate/payments/${String(row.id)}` as Route}🤖 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 `@src/app/candidate/payments/page.tsx` at line 18, The current rowHref uses an unsafe double-cast ("as unknown as Route") which defeats the type system; update the rowHref function to assert the final template string directly as Route (e.g. return (`/candidate/payments/${row.id}`) as Route) and remove the intermediate "as unknown" cast, making sure the string interpolation in rowHref produces the exact Route shape expected by the table component and that row.id is the correct type for the path.
🤖 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 `@docs/migration/test-coverage.md`:
- Line 86: Replace the misspelled word "brower" with "browser" in the Playwright
section text (the offending token "brower" is what to search for) so the
sentence reads "for authenticated browser tests. Fixtures are discovered from
the database via ..." and ensure surrounding punctuation/spacing remains
consistent.
In `@e2e/smoke/candidate-search.spec.ts`:
- Line 2: Replace the relative import of the fixtures with the project alias:
update the import that currently brings in getFixtures, disconnectPrisma, and
signSession from "../fixtures/auth" to use "`@/fixtures/auth`" so the test uses
the repository alias for internal imports.
In `@e2e/smoke/candidate-self-service.spec.ts`:
- Line 2: Replace the relative traversal import in
candidate-self-service.spec.ts with the project path alias: change the import
that currently pulls getFixtures and disconnectPrisma from "../fixtures/auth" to
use the "`@/fixtures/auth`" alias so it follows the project's internal import
convention and resolves correctly; update the import statement referencing
getFixtures and disconnectPrisma accordingly.
In `@e2e/smoke/request-desk.spec.ts`:
- Line 2: Replace the relative import of test fixtures with the project
path-alias: change the import that currently references getFixtures and
disconnectPrisma from "../fixtures/auth" to use the "`@/fixtures/auth`" alias so
the file imports getFixtures and disconnectPrisma via "`@/fixtures/auth`" instead
of a relative path.
In `@e2e/smoke/role-portals.spec.ts`:
- Line 2: Replace the relative import of the auth fixtures with the repo
path-alias form: change the import that currently pulls getFixtures and
disconnectPrisma from "../fixtures/auth" to use the "`@/fixtures/auth`" alias so
the symbols getFixtures and disconnectPrisma are imported via the `@/` path as
required by the project's internal import style.
In `@src/app/app/page.tsx`:
- Line 27: The prop is using a redundant cast "as unknown as HubContentData"
which silences TypeScript instead of fixing the source type; remove the
double-cast from the JSX (the data={...} expression) and instead ensure the
value is correctly typed at its origin—either declare the result of
getUnifiedHub() as returning HubContentData or type the local variable that
holds it as HubContentData—so the JSX passes data (prop name "data") as a
properly typed HubContentData without casting. Reference symbols: data prop in
page.tsx, HubContentData type, and getUnifiedHub()/the local variable that
receives its result.
In `@src/app/candidate/payments/`[id]/page.tsx:
- Around line 15-16: The route currently calls
getCandidateTransferDetail(Number(id), Number(session.id)) where Number(id) can
be NaN and will propagate into prisma.transfer_candidate.findFirst({ where: {
tc_id: tcId } }); validate and guard the route id first: parse and check the id
(e.g., use parseInt or Number and then Number.isFinite/Number.isInteger or
isNaN) and call notFound() (or return a 404) if the id is not a valid integer
before invoking getCandidateTransferDetail; pass the validated numeric id into
getCandidateTransferDetail instead of raw Number(id) so prisma never receives
NaN for tc_id.
In `@src/modules/candidates/ExportCVsForm.tsx`:
- Around line 7-10: The form is using a server action exportCandidateBundle as a
form action which returns a Response that won't be delivered to the browser;
change to either (A) implement a route handler (e.g.
app/api/export-cvs/route.ts) that returns the file Response with proper headers
and call that endpoint from the client, or (B) keep exportCandidateBundle logic
but invoke it via client-side fetch from ExportCVsForm (replace the <form> with
a button that onClick uses fetch to POST candidateIds, reads response.blob(),
creates an object URL and programmatically clicks a download link). Locate
ExportCVsForm and the exportCandidateBundle reference and update the component
to call the new route or fetch and handle blob -> URL.createObjectURL + anchor
click to trigger the download. Ensure the server response includes
Content-Disposition and correct MIME type so the browser downloads the file.
In `@src/modules/candidates/worklog-actions.ts`:
- Around line 189-206: The insert is defaulting company_id to 0 which is unsafe;
before calling prisma.candidate_work_log_feedback.create (and using
resolved.workLog.candidate_id, workLogUuid, etc.) resolve company_id from
resolved.workLog.store?.company_id into a local variable and if it's
null/undefined throw or return a clear validation error (do not substitute 0),
then use that validated companyId in the create call; this ensures you fail fast
rather than inserting an invalid FK.
In `@src/modules/requests/candidate-actions.ts`:
- Around line 10-35: The addCandidateNoteAction allows any session with
requireCapability("request.suggest") to mutate any candidate by ID; before
performing mutations (after parsing candidateId/requestUuid and before
prisma.candidate.findFirst and any DB writes) enforce ownership: if session.role
!== "admin" query the relevant ownership (e.g., load the request via
prisma.request.findFirst using requestUuid to verify request.staff_id ===
session.staff_id, or if no requestUuid, load candidate ownership via
prisma.candidate.findFirst including its staff_id or linked request owner) and
if the session staff_id does not own the resource call
redirect(`${returnPath}?notice=not-authorized`) and abort; keep the existing
admin bypass (session.role === "admin") and apply this same ownership check in
the other mutated handlers mentioned (lines 56-82, 102-127, 129-155) using the
same pattern.
In `@src/modules/requests/invitation-actions.ts`:
- Around line 39-46: The pre-check for duplicates (prisma.invitation.findFirst)
is non-atomic and can still allow concurrent duplicate inserts; remove the
race-prone check and instead enforce a DB-level unique constraint on
(request_uuid, candidate_id) in the Prisma schema for the Invitation model, then
attempt the insert (prisma.invitation.create) and catch Prisma unique-constraint
errors (PrismaClientKnownRequestError code "P2002") to treat them as duplicates
(redirect to detailPath?notice=duplicate-invitation). Also apply the same change
to the other creation site referenced in lines 52-69 so both paths rely on the
DB constraint plus the P2002 handling rather than the non-atomic findFirst
check.
In `@src/modules/requests/pipeline-actions.ts`:
- Around line 123-131: The code currently hardcodes version: 1 in
prisma.interview_evaluation_note_version.create which breaks version
progression; change this to compute the next version for the given
interview_evaluation_uuid by first querying
prisma.interview_evaluation_note_version to get the current max(version) for
interview_evaluation_uuid (or 0 if none) and set version = max + 1 before
calling create; perform both the select and the create inside a single
transaction (or use a serializable transaction/row lock pattern) to avoid race
conditions; update references around noteVersionUuid, evaluationUuid, staffId,
and now so the create uses the computed version instead of the literal 1.
In `@src/modules/requests/RequestFulfillmentOS.tsx`:
- Around line 20-27: The merged list of ActionableRow items can contain
duplicate id values across the two collections, causing React key collisions;
when combining datasets (the merge step that builds the rows for rendering) and
where rows are rendered (the map that currently uses row.id as the key), ensure
keys are globally unique by prefixing or namespacing the id with the source or
by synthesizing a unique uid (e.g., "story-<id>" vs "activity-<id>"); update the
merge logic that produces the rows and the render/map site that uses
key={row.id} to use the new unique identifier (or add a new property like uid on
the ActionableRow objects) so React reconciliation is stable.
In `@src/modules/workspace/data.ts`:
- Around line 1936-1937: The Prisma query in getCandidateTransferDetail
currently filters only by tc_id and deleted, then checks tc.candidate_id !==
candidateId after fetching; modify the query's where clause to include
candidate_id: candidateId (in addition to tc_id: tcId and deleted: 0) so the DB
only returns rows that belong to the requesting candidate and prevents
unauthorized row loading; update the where object used in the Prisma call that
contains tc_id, deleted to also include candidate_id.
---
Outside diff comments:
In `@src/modules/requests/RequestFulfillmentOS.tsx`:
- Around line 190-236: The Inbound (Applications), Evaluation (Interviews) and
Stories cards lack id attributes that match the stage anchor links
(`#${stage.id}`), so add the corresponding id props to those Card elements (the
Card wrapping the Applications RequestRows, the Card wrapping the Interviews
RequestRows, and the Card for Stories) so the stage anchor links can scroll to
them; locate the Cards in RequestFulfillmentOS.tsx (the blocks using RequestRows
with ApplicationStatusActions and InterviewStatusActions) and set id={stage.id}
(or the appropriate stage identifier from props/data) on each Card to match the
anchor target.
In `@src/modules/workspace/WorkTabs.tsx`:
- Around line 89-101: The closeTab updater currently performs a side effect
(router.push) inside the setTabs callback which must be a pure function; move
navigation out of the state updater in the closeTab function: compute the new
tabs array first (using the existing filter logic from closeTab), call writeTabs
and setTabs(next) with that array, and then if pathname === path and next.length
> 0 call router.push(next[0].path as Route) outside the setTabs callback (or,
alternatively, set a ref/flag and perform the navigation in a useEffect or via
flushSync to avoid concurrent React re-run issues). Ensure you update the
closeTab function and keep writeTabs usage intact.
---
Nitpick comments:
In `@e2e/smoke/login.spec.ts`:
- Around line 28-38: The test named "session redirects logged-in user away from
login" currently has an unconditional test.skip() that prevents validation;
change it to a gated test by replacing the unconditional test.skip() with either
a conditional test.fixme/skip that only runs in CI without DB (e.g., if
(process.env.CI && !process.env.PLAYWRIGHT_DB_TESTS) { test.fixme("requires DB
fixture"); return; }) or implement the actual test logic that sets a valid
session cookie and asserts navigation from /login to /app; update the test block
referenced by the string "session redirects logged-in user away from login"
accordingly so the behavior is covered or explicitly marked as FIXME with a
tracking reference.
In `@src/app/candidate/payments/page.tsx`:
- Line 18: The current rowHref uses an unsafe double-cast ("as unknown as
Route") which defeats the type system; update the rowHref function to assert the
final template string directly as Route (e.g. return
(`/candidate/payments/${row.id}`) as Route) and remove the intermediate "as
unknown" cast, making sure the string interpolation in rowHref produces the
exact Route shape expected by the table component and that row.id is the correct
type for the path.
In `@src/modules/candidates/actions.ts`:
- Around line 615-635: The removeStaffCandidateWarning function currently
performs a hard delete via prisma.candidate_warning.delete which is inconsistent
with the soft-delete pattern used elsewhere (e.g., tags/skills which set
deleted: 1); change the deletion to a soft-delete update (use
prisma.candidate_warning.update to set deleted: 1 and any audit fields like
deleted_at/deleted_by if present) and adjust any subsequent logic that expects
existence (revalidatePath calls can remain); ensure you reference
prisma.candidate_warning.update and the deleted field so behavior matches other
entities.
In `@src/modules/candidates/CandidateEditForm.tsx`:
- Around line 314-318: Remove the redundant hidden input element that sets
name="type" in CandidateEditForm.tsx: the server action uploadDocument now
derives the document type from the file input name (`file_${type}`), so delete
the <input type="hidden" name="type" value={type} /> element (and ensure there
are no remaining references to this hidden field elsewhere in CandidateEditForm
or related handlers like uploadDocument).
In `@src/modules/candidates/export-actions.ts`:
- Around line 54-57: The Promise.all call that fetches candidate details
(scopedIds.map(id => getCandidateDetail(id))) will fail-fast; replace it with
Promise.allSettled to collect per-id results, keep all fulfilled values for the
export (assign to details), log or surface warnings for rejected entries
including the candidate id and error, and handle the case where all fetches fail
by throwing or returning an appropriate error; update references to details in
this module (export-actions.ts, getCandidateDetail, scopedIds) to operate on the
filtered fulfilled results.
In `@src/modules/candidates/worklog-actions.ts`:
- Around line 136-159: The operations array is typed as any[], disabling TS
checks; change it to use PrismaPromise types by importing PrismaPromise from
`@prisma/client` and typing operations as PrismaPromise<unknown>[] (or a more
specific PrismaPromise<ReturnType>[] if you want exact results) so the calls to
prisma.candidate_working_hour_appeal.update and
prisma.candidate_working_hour_appeal_updates.create are statically checked
before calling prisma.$transaction(operations).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f15fa156-4166-4160-9279-16d6abd44214
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.gitignoredocs/migration/test-coverage.mde2e/fixtures/auth.tse2e/smoke/candidate-search.spec.tse2e/smoke/candidate-self-service.spec.tse2e/smoke/login.spec.tse2e/smoke/request-desk.spec.tse2e/smoke/role-portals.spec.tspackage.jsonplaywright.config.tsscripts/fixtures/discover.mjsscripts/fixtures/validate.mjssrc/app/app/layout.tsxsrc/app/app/page.tsxsrc/app/candidate/payments/[id]/page.tsxsrc/app/candidate/payments/page.tsxsrc/modules/candidates/CandidateEditForm.tsxsrc/modules/candidates/ExportCVsForm.tsxsrc/modules/candidates/actions.tssrc/modules/candidates/export-actions.tssrc/modules/candidates/search.tssrc/modules/candidates/worklog-actions.tssrc/modules/hub/HubContent.tsxsrc/modules/requests/MatchActions.tsxsrc/modules/requests/RequestFulfillmentOS.tsxsrc/modules/requests/StageActions.tsxsrc/modules/requests/application-actions.tssrc/modules/requests/candidate-actions.tssrc/modules/requests/interview-actions.tssrc/modules/requests/invitation-actions.tssrc/modules/requests/pipeline-actions.tssrc/modules/requests/story-actions.tssrc/modules/workspace/WorkTabs.tsxsrc/modules/workspace/data.ts
| ## Playwright Smoke Tests (added STU-26) | ||
|
|
||
| Playwright smoke tests live in `e2e/smoke/` and use fixture-signed session cookies | ||
| for authenticated brower tests. Fixtures are discovered from the database via |
There was a problem hiding this comment.
Fix typo in Playwright section.
Line 86 has a typo: brower → browser.
🧰 Tools
🪛 LanguageTool
[grammar] ~86-~86: Ensure spelling is correct
Context: ...igned session cookies for authenticated brower tests. Fixtures are discovered from the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/migration/test-coverage.md` at line 86, Replace the misspelled word
"brower" with "browser" in the Playwright section text (the offending token
"brower" is what to search for) so the sentence reads "for authenticated browser
tests. Fixtures are discovered from the database via ..." and ensure surrounding
punctuation/spacing remains consistent.
| @@ -0,0 +1,99 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
| import { getFixtures, disconnectPrisma, signSession } from "../fixtures/auth"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use @/ alias for internal fixture import.
Please replace the relative import with the repository alias to match project import policy.
Proposed change
-import { getFixtures, disconnectPrisma, signSession } from "../fixtures/auth";
+import { getFixtures, disconnectPrisma, signSession } from "`@/e2e/fixtures/auth`";As per coding guidelines, Use @/ path alias for all internal imports.
🤖 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 `@e2e/smoke/candidate-search.spec.ts` at line 2, Replace the relative import of
the fixtures with the project alias: update the import that currently brings in
getFixtures, disconnectPrisma, and signSession from "../fixtures/auth" to use
"`@/fixtures/auth`" so the test uses the repository alias for internal imports.
| @@ -0,0 +1,287 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Switch internal auth fixture import to @/ alias.
This should follow the project alias rule instead of relative traversal.
Proposed change
-import { getFixtures, disconnectPrisma } from "../fixtures/auth";
+import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`";As per coding guidelines, Use @/ path alias for all internal imports.
📝 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
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |
| import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`"; |
🤖 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 `@e2e/smoke/candidate-self-service.spec.ts` at line 2, Replace the relative
traversal import in candidate-self-service.spec.ts with the project path alias:
change the import that currently pulls getFixtures and disconnectPrisma from
"../fixtures/auth" to use the "`@/fixtures/auth`" alias so it follows the
project's internal import convention and resolves correctly; update the import
statement referencing getFixtures and disconnectPrisma accordingly.
| @@ -0,0 +1,101 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use @/ path alias for internal fixtures import.
Relative import here should be converted to the repository-standard alias.
Proposed change
-import { getFixtures, disconnectPrisma } from "../fixtures/auth";
+import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`";As per coding guidelines, Use @/ path alias for all internal imports.
📝 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
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |
| import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`"; |
🤖 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 `@e2e/smoke/request-desk.spec.ts` at line 2, Replace the relative import of
test fixtures with the project path-alias: change the import that currently
references getFixtures and disconnectPrisma from "../fixtures/auth" to use the
"`@/fixtures/auth`" alias so the file imports getFixtures and disconnectPrisma via
"`@/fixtures/auth`" instead of a relative path.
| @@ -0,0 +1,263 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Convert internal auth import to @/ alias.
Please align this import with the repo’s required internal import style.
Proposed change
-import { getFixtures, disconnectPrisma } from "../fixtures/auth";
+import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`";As per coding guidelines, Use @/ path alias for all internal imports.
📝 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
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |
| import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`"; |
🤖 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 `@e2e/smoke/role-portals.spec.ts` at line 2, Replace the relative import of the
auth fixtures with the repo path-alias form: change the import that currently
pulls getFixtures and disconnectPrisma from "../fixtures/auth" to use the
"`@/fixtures/auth`" alias so the symbols getFixtures and disconnectPrisma are
imported via the `@/` path as required by the project's internal import style.
Comment on lines
+10
to
+35
| export async function addCandidateNoteAction(formData: FormData) { | ||
| const session = await requireCapability("request.suggest"); | ||
|
|
||
| const candidateId = Number(formData.get("candidate_id")); | ||
| const noteText = String(formData.get("note_text") ?? "").trim(); | ||
| const requestUuid = String(formData.get("request_uuid") ?? "").trim() || null; | ||
| const basePath = session.role === "admin" ? "/admin/requests" : "/staff/requests"; | ||
| const returnPath = requestUuid | ||
| ? (`${basePath}/${requestUuid}` as Route) | ||
| : session.role === "admin" | ||
| ? `/admin/candidates/${candidateId}` as Route | ||
| : `/staff/candidates?candidate=${candidateId}` as Route; | ||
|
|
||
| if (!Number.isInteger(candidateId) || candidateId <= 0 || !noteText) { | ||
| redirect(`${returnPath}?notice=missing-fields` as Route); | ||
| } | ||
|
|
||
| const candidate = await prisma.candidate.findFirst({ | ||
| where: { candidate_id: candidateId, deleted: 0 }, | ||
| select: { candidate_id: true } | ||
| }); | ||
|
|
||
| if (!candidate) { | ||
| redirect(`${returnPath}?notice=not-found` as Route); | ||
| } | ||
|
|
There was a problem hiding this comment.
Enforce staff ownership checks before candidate mutations.
These actions allow any request.suggest staff session to mutate arbitrary candidates by posting candidate_id; there is no staff-to-request/candidate ownership validation in the mutation path.
🔒 Suggested fix pattern
export async function addCandidateNoteAction(formData: FormData) {
const session = await requireCapability("request.suggest");
@@
const requestUuid = String(formData.get("request_uuid") ?? "").trim() || null;
@@
+ if (session.role === "staff") {
+ if (!requestUuid) {
+ redirect(`${returnPath}?notice=not-found` as Route);
+ }
+ const owned = await prisma.request.findFirst({
+ where: { request_uuid: requestUuid, staff_id: Number(session.id) },
+ select: { request_uuid: true }
+ });
+ if (!owned) {
+ redirect(`${returnPath}?notice=not-found` as Route);
+ }
+ }Also applies to: 56-82, 102-127, 129-155
🤖 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 `@src/modules/requests/candidate-actions.ts` around lines 10 - 35, The
addCandidateNoteAction allows any session with
requireCapability("request.suggest") to mutate any candidate by ID; before
performing mutations (after parsing candidateId/requestUuid and before
prisma.candidate.findFirst and any DB writes) enforce ownership: if session.role
!== "admin" query the relevant ownership (e.g., load the request via
prisma.request.findFirst using requestUuid to verify request.staff_id ===
session.staff_id, or if no requestUuid, load candidate ownership via
prisma.candidate.findFirst including its staff_id or linked request owner) and
if the session staff_id does not own the resource call
redirect(`${returnPath}?notice=not-authorized`) and abort; keep the existing
admin bypass (session.role === "admin") and apply this same ownership check in
the other mutated handlers mentioned (lines 56-82, 102-127, 129-155) using the
same pattern.
Comment on lines
+39
to
+46
| const duplicate = await prisma.invitation.findFirst({ | ||
| where: { request_uuid: requestUuid, candidate_id: candidateId }, | ||
| select: { invitation_uuid: true } | ||
| }); | ||
|
|
||
| if (duplicate) { | ||
| redirect(`${detailPath}?notice=duplicate-invitation` as Route); | ||
| } |
There was a problem hiding this comment.
Protect invitation creation against race-condition duplicates.
The duplicate pre-check and create are non-atomic; concurrent submissions can still insert two invitations for the same (request_uuid, candidate_id).
🧱 Suggested hardening
- const duplicate = await prisma.invitation.findFirst({
- where: { request_uuid: requestUuid, candidate_id: candidateId },
- select: { invitation_uuid: true }
- });
-
- if (duplicate) {
- redirect(`${detailPath}?notice=duplicate-invitation` as Route);
- }
-
const now = new Date();
const invitationUuid = `invitation_${crypto.randomUUID()}`;
@@
- await prisma.$transaction([
- prisma.invitation.create({
+ try {
+ await prisma.$transaction([
+ prisma.invitation.create({
data: {
@@
- prisma.request.update({
+ prisma.request.update({
where: { request_uuid: requestUuid },
data: { request_updated_datetime: now }
- })
- ]);
+ })
+ ]);
+ } catch (error: any) {
+ if (error?.code === "P2002") {
+ redirect(`${detailPath}?notice=duplicate-invitation` as Route);
+ }
+ throw error;
+ }Also add a DB unique constraint on (request_uuid, candidate_id) for full protection.
Also applies to: 52-69
🤖 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 `@src/modules/requests/invitation-actions.ts` around lines 39 - 46, The
pre-check for duplicates (prisma.invitation.findFirst) is non-atomic and can
still allow concurrent duplicate inserts; remove the race-prone check and
instead enforce a DB-level unique constraint on (request_uuid, candidate_id) in
the Prisma schema for the Invitation model, then attempt the insert
(prisma.invitation.create) and catch Prisma unique-constraint errors
(PrismaClientKnownRequestError code "P2002") to treat them as duplicates
(redirect to detailPath?notice=duplicate-invitation). Also apply the same change
to the other creation site referenced in lines 52-69 so both paths rely on the
DB constraint plus the P2002 handling rather than the non-atomic findFirst
check.
Comment on lines
+123
to
+131
| prisma.interview_evaluation_note_version.create({ | ||
| data: { | ||
| ienv_uuid: noteVersionUuid, | ||
| interview_evaluation_uuid: evaluationUuid, | ||
| version: 1, | ||
| staff_id: staffId, | ||
| created_at: now, | ||
| updated_at: now | ||
| } |
There was a problem hiding this comment.
Do not hardcode interview evaluation note version to 1.
Every note insert uses version: 1, which breaks version progression and can fail if (interview_evaluation_uuid, version) is constrained.
🔢 Suggested fix
+ const latestVersion = await prisma.interview_evaluation_note_version.findFirst({
+ where: { interview_evaluation_uuid: evaluationUuid },
+ orderBy: { version: "desc" },
+ select: { version: true }
+ });
+ const nextVersion = (latestVersion?.version ?? 0) + 1;
@@
prisma.interview_evaluation_note_version.create({
data: {
ienv_uuid: noteVersionUuid,
interview_evaluation_uuid: evaluationUuid,
- version: 1,
+ version: nextVersion,
staff_id: staffId,📝 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
| prisma.interview_evaluation_note_version.create({ | |
| data: { | |
| ienv_uuid: noteVersionUuid, | |
| interview_evaluation_uuid: evaluationUuid, | |
| version: 1, | |
| staff_id: staffId, | |
| created_at: now, | |
| updated_at: now | |
| } | |
| const latestVersion = await prisma.interview_evaluation_note_version.findFirst({ | |
| where: { interview_evaluation_uuid: evaluationUuid }, | |
| orderBy: { version: "desc" }, | |
| select: { version: true } | |
| }); | |
| const nextVersion = (latestVersion?.version ?? 0) + 1; | |
| prisma.interview_evaluation_note_version.create({ | |
| data: { | |
| ienv_uuid: noteVersionUuid, | |
| interview_evaluation_uuid: evaluationUuid, | |
| version: nextVersion, | |
| staff_id: staffId, | |
| created_at: now, | |
| updated_at: now | |
| } |
🤖 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 `@src/modules/requests/pipeline-actions.ts` around lines 123 - 131, The code
currently hardcodes version: 1 in
prisma.interview_evaluation_note_version.create which breaks version
progression; change this to compute the next version for the given
interview_evaluation_uuid by first querying
prisma.interview_evaluation_note_version to get the current max(version) for
interview_evaluation_uuid (or 0 if none) and set version = max + 1 before
calling create; perform both the select and the create inside a single
transaction (or use a serializable transaction/row lock pattern) to avoid race
conditions; update references around noteVersionUuid, evaluationUuid, staffId,
and now so the create uses the computed version instead of the literal 1.
Comment on lines
+20
to
+27
| type ActionableRow = { | ||
| id: string | number; | ||
| title: string; | ||
| subtitle: string; | ||
| meta?: string; | ||
| href?: string; | ||
| status?: number | null; | ||
| }; |
There was a problem hiding this comment.
Avoid key collisions when mixing stories and activities.
Line 254 combines two datasets, and Line 283 keys rows only by row.id. If IDs overlap across collections, React reconciliation becomes unstable.
Suggested fix
type ActionableRow = {
+ rowKey?: string;
id: string | number;
title: string;
subtitle: string;
meta?: string;
href?: string;
status?: number | null;
};
...
<RequestRows
- rows={[...data.stories, ...data.activities].slice(0, 12)}
+ rows={[
+ ...data.stories.map((row) => ({ ...row, rowKey: `story:${row.id}` })),
+ ...data.activities.map((row) => ({ ...row, rowKey: `activity:${row.id}` }))
+ ].slice(0, 12)}
actions={(row) =>
row.status !== undefined ? (
<StoryStatusActions
storyUuid={String(row.id)}
requestUuid={requestUuid}
currentStatus={row.status}
/>
) : null
}
/>
...
- <div key={row.id} className="requestRow">
+ <div key={row.rowKey ?? String(row.id)} className="requestRow">Also applies to: 253-255, 283-283
🤖 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 `@src/modules/requests/RequestFulfillmentOS.tsx` around lines 20 - 27, The
merged list of ActionableRow items can contain duplicate id values across the
two collections, causing React key collisions; when combining datasets (the
merge step that builds the rows for rendering) and where rows are rendered (the
map that currently uses row.id as the key), ensure keys are globally unique by
prefixing or namespacing the id with the source or by synthesizing a unique uid
(e.g., "story-<id>" vs "activity-<id>"); update the merge logic that produces
the rows and the render/map site that uses key={row.id} to use the new unique
identifier (or add a new property like uid on the ActionableRow objects) so
React reconciliation is stable.
Comment on lines
+1936
to
+1937
| where: { tc_id: tcId, deleted: 0 }, | ||
| select: { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section around the flagged lines
sed -n '1910,2010p' src/modules/workspace/data.ts | cat -n
# Search for the exact query pattern in this file (tc_id / candidate_id / deleted)
rg -n "transfer_candidate\.findFirst|tc_id: tcId|candidate_id" src/modules/workspace/data.ts
# Show the specific line(s) referenced by the review comment, with a bit more context
sed -n '1925,1990p' src/modules/workspace/data.ts | nl -ba | sed -n '1,120p'
# Check if there are other similar queries for transfer_candidate in this file
rg -n "transfer_candidate" src/modules/workspace/data.tsRepository: BAWES/studenthub-codex
Length of output: 11807
Scope transfer_candidate lookup by candidate_id in getCandidateTransferDetail to prevent unauthorized row loading.
src/modules/workspace/data.ts currently fetches with where: { tc_id: tcId, deleted: 0 } and only rejects mismatches after the query via tc.candidate_id !== candidateId. Add candidate_id to the Prisma where clause.
Suggested fix
const tc = await prisma.transfer_candidate.findFirst({
- where: { tc_id: tcId, deleted: 0 },
+ where: { tc_id: tcId, candidate_id: candidateId, deleted: 0 },
select: {
tc_id: true,
candidate_id: true,
@@
- if (!tc || tc.candidate_id !== candidateId) return null;
+ if (!tc) return null;📝 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
| where: { tc_id: tcId, deleted: 0 }, | |
| select: { | |
| where: { tc_id: tcId, candidate_id: candidateId, deleted: 0 }, | |
| select: { |
🤖 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 `@src/modules/workspace/data.ts` around lines 1936 - 1937, The Prisma query in
getCandidateTransferDetail currently filters only by tc_id and deleted, then
checks tc.candidate_id !== candidateId after fetching; modify the query's where
clause to include candidate_id: candidateId (in addition to tc_id: tcId and
deleted: 0) so the DB only returns rows that belong to the requesting candidate
and prevents unauthorized row loading; update the where object used in the
Prisma call that contains tc_id, deleted to also include candidate_id.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/modules/candidates/CandidateSearchOS.tsx (1)
8-8: ⚡ Quick winUse
@/alias for this new internal import.Line 8 uses a relative internal import; this should use the project alias for consistency and lint compliance.
Suggested change
-import { ExportCVsForm } from "./ExportCVsForm"; +import { ExportCVsForm } from "`@/modules/candidates/ExportCVsForm`";As per coding guidelines,
Use@/path alias for all internal imports.🤖 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 `@src/modules/candidates/CandidateSearchOS.tsx` at line 8, Replace the relative import of ExportCVsForm with the project path alias: update the import statement that currently references "./ExportCVsForm" in CandidateSearchOS.tsx to use the "`@/`..." alias (e.g., "`@/modules/candidates/ExportCVsForm`") so it follows the project's internal import conventions and linting rules.
🤖 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 `@e2e/m5-pipeline-qa.spec.ts`:
- Around line 60-73: Replace the silent-pass branch by making the presence of
the suggest form a deterministic precondition: before interacting with
suggestForm call await expect(suggestForm).toBeVisible({ timeout: 10000 }) and
remove the else console.log path (or call test.skip with a clear reason if the
test genuinely cannot run), then proceed to fill and submit and assert
page.waitForURL(/notice=/) and the URL match; apply this same pattern to the
other similar blocks referenced (the blocks around the other match-card
interactions) so tests fail fast when the element is absent instead of silently
passing.
- Line 5: The test is using a hardcoded REQUEST_UUID constant which couples the
suite to one dataset; replace the fixed REQUEST_UUID with a runtime-resolved
value by reading from an environment variable (e.g., process.env.REQUEST_UUID)
or by adding a setup helper (e.g., a beforeAll/prepareRequestFixture function)
that creates or fetches a deterministic request and assigns it to REQUEST_UUID
(or a new variable used by tests); update references in this file to use the
resolved value so the suite can run across environments and after resets.
- Around line 3-4: Replace the hardcoded STAFF_EMAIL and STAFF_PASSWORD
constants with environment-driven values (read from process.env.STAFF_EMAIL and
process.env.STAFF_PASSWORD) and add a fail-fast check at test startup (e.g.,
throw or call fail) if either is undefined or empty so the test run errors
immediately when credentials are missing; update any references to
STAFF_EMAIL/STAFF_PASSWORD in this spec to use the new env-backed variables and
ensure secrets are not committed.
---
Nitpick comments:
In `@src/modules/candidates/CandidateSearchOS.tsx`:
- Line 8: Replace the relative import of ExportCVsForm with the project path
alias: update the import statement that currently references "./ExportCVsForm"
in CandidateSearchOS.tsx to use the "`@/`..." alias (e.g.,
"`@/modules/candidates/ExportCVsForm`") so it follows the project's internal
import conventions and linting rules.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d913ea4e-3a99-4b07-9958-ffb357eceea2
📒 Files selected for processing (2)
e2e/m5-pipeline-qa.spec.tssrc/modules/candidates/CandidateSearchOS.tsx
Comment on lines
+3
to
+4
| const STAFF_EMAIL = "alaa.jawad@bawes.net"; | ||
| const STAFF_PASSWORD = "test1234"; |
There was a problem hiding this comment.
Remove hardcoded staff credentials from source.
Committing plaintext credentials is a security/compliance risk. Load these from environment variables and fail fast when missing.
Suggested patch
-const STAFF_EMAIL = "alaa.jawad@bawes.net";
-const STAFF_PASSWORD = "test1234";
+const STAFF_EMAIL = process.env.E2E_STAFF_EMAIL;
+const STAFF_PASSWORD = process.env.E2E_STAFF_PASSWORD;
+
+if (!STAFF_EMAIL || !STAFF_PASSWORD) {
+ throw new Error("Missing E2E_STAFF_EMAIL / E2E_STAFF_PASSWORD");
+}📝 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
| const STAFF_EMAIL = "alaa.jawad@bawes.net"; | |
| const STAFF_PASSWORD = "test1234"; | |
| const STAFF_EMAIL = process.env.E2E_STAFF_EMAIL; | |
| const STAFF_PASSWORD = process.env.E2E_STAFF_PASSWORD; | |
| if (!STAFF_EMAIL || !STAFF_PASSWORD) { | |
| throw new Error("Missing E2E_STAFF_EMAIL / E2E_STAFF_PASSWORD"); | |
| } |
🤖 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 `@e2e/m5-pipeline-qa.spec.ts` around lines 3 - 4, Replace the hardcoded
STAFF_EMAIL and STAFF_PASSWORD constants with environment-driven values (read
from process.env.STAFF_EMAIL and process.env.STAFF_PASSWORD) and add a fail-fast
check at test startup (e.g., throw or call fail) if either is undefined or empty
so the test run errors immediately when credentials are missing; update any
references to STAFF_EMAIL/STAFF_PASSWORD in this spec to use the new env-backed
variables and ensure secrets are not committed.
|
|
||
| const STAFF_EMAIL = "alaa.jawad@bawes.net"; | ||
| const STAFF_PASSWORD = "test1234"; | ||
| const REQUEST_UUID = "request_e6379cee-052b-11f0-b1cd-a2aacba78f94"; |
There was a problem hiding this comment.
Hardcoded request UUID tightly couples the suite to one dataset.
A fixed record ID makes the suite fragile across environments/resets. Prefer an env-configured ID or a setup step that creates/fetches a known request fixture first.
🤖 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 `@e2e/m5-pipeline-qa.spec.ts` at line 5, The test is using a hardcoded
REQUEST_UUID constant which couples the suite to one dataset; replace the fixed
REQUEST_UUID with a runtime-resolved value by reading from an environment
variable (e.g., process.env.REQUEST_UUID) or by adding a setup helper (e.g., a
beforeAll/prepareRequestFixture function) that creates or fetches a
deterministic request and assigns it to REQUEST_UUID (or a new variable used by
tests); update references in this file to use the resolved value so the suite
can run across environments and after resets.
Comment on lines
+60
to
+73
| if (await suggestForm.isVisible()) { | ||
| // Fill in the reason | ||
| await suggestForm.locator('input[name="reason"]').fill("QA test suggestion - good skill match"); | ||
| // Click Suggest | ||
| await suggestForm.locator('button[type="submit"]').click(); | ||
|
|
||
| // Should redirect back with notice | ||
| await page.waitForURL(/notice=/, { timeout: 10000 }); | ||
| const url = page.url(); | ||
| expect(url).toMatch(/suggestion-added|duplicate-suggestion/); | ||
| console.log("Suggest action result:", url); | ||
| } else { | ||
| console.log("No match cards with suggestion forms available — all candidates may already be in pipeline"); | ||
| } |
There was a problem hiding this comment.
Avoid silent-pass branches in action tests.
These tests can pass without executing the stated action when elements are missing (only console.log in else). That weakens smoke-test reliability and can hide regressions. Use deterministic preconditions (expect(...).toBeVisible()), or explicitly test.skip(...) with a clear reason.
Also applies to: 84-94, 110-130, 144-169
🤖 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 `@e2e/m5-pipeline-qa.spec.ts` around lines 60 - 73, Replace the silent-pass
branch by making the presence of the suggest form a deterministic precondition:
before interacting with suggestForm call await expect(suggestForm).toBeVisible({
timeout: 10000 }) and remove the else console.log path (or call test.skip with a
clear reason if the test genuinely cannot run), then proceed to fill and submit
and assert page.waitForURL(/notice=/) and the URL match; apply this same pattern
to the other similar blocks referenced (the blocks around the other match-card
interactions) so tests fail fast when the element is absent instead of silently
passing.
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
Note on STU-22
The M4 Candidate Mobile Self-Service work was committed in
4a2a9e8and is already merged intomain. This PR covers the remaining M5 staff request fulfillment work on top.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores