feat(vendors): vendor/contractor management UI (Story #143)

[steilerDev]

Summary

• Implement vendor/contractor management (Story 5.2: Vendor/contractor management #143, EPIC-05)
• Add vendor CRUD API with paginated list, search, sorting, invoice stats
• Build Vendors list page (table/cards, search, pagination, create/delete modals)
• Build Vendor detail page (info card, stats, inline edit, delete, invoice placeholder)
• Comprehensive test coverage (230 unit/integration + 38 E2E tests)

API endpoints

• GET /api/vendors — Paginated list with search, sort
• POST /api/vendors — Create vendor
• GET /api/vendors/:id — Detail with invoice count + outstanding balance
• PATCH /api/vendors/:id — Partial update
• DELETE /api/vendors/:id — Delete (409 if in use)

Test coverage

• 75 vendor service unit tests
• 44 vendor route integration tests
• 32 API client tests
• 38 VendorsPage component tests
• 41 VendorDetailPage component tests
• 38 Playwright E2E tests

Fixes #143

Test plan

• All quality gates pass (lint, typecheck, format, build)
• 230 new unit/integration tests passing
• 38 E2E tests covering UAT scenarios
• Vendor CRUD with pagination and search
• Delete protection (409) for vendors with invoices or work items
• Responsive layout (table/cards) and dark mode

🤖 Generated with Claude Code

[steilerDev]

[ux-designer]

PR #151 — Vendor Management (Story #143) — Visual/UX Review

---

Overall Assessment: Approved with non-blocking observations

Token adherence is excellent throughout both CSS files. Zero hardcoded hex colors, font sizes, or spacing values — all values correctly reference Layer 2 semantic tokens. Responsive table→card swap, modal patterns, and accessibility markup are all solid. The implementation is consistent with established patterns (WorkItemsPage, BudgetCategoriesPage).

---

Token Adherence

Pass. Both VendorsPage.module.css and VendorDetailPage.module.css use only var(--token-name) references. Spot-checked every property that could carry a raw value:

• All colors: var(--color-*) only
• All spacing: var(--spacing-*) only
• All border radii: var(--radius-*) only
• All shadows: var(--shadow-*) only
• All font sizes/weights: var(--font-size-*) / var(--font-weight-*) only
• All transitions: var(--transition-*) only
• Z-index: var(--z-modal) — correct

---

Interactive States

Pass. All major interactive states are covered.

• Primary button: hover, focus-visible (shadow-focus), disabled (placeholder bg, not-allowed cursor)
• Secondary/cancel/edit buttons: hover (color-border bg), focus-visible, disabled (opacity 0.5)
• Danger buttons: hover (danger-bg-strong), focus-visible (shadow-focus-danger), disabled
• Confirm-delete button: hover, focus-visible (shadow-focus-danger), disabled
• Inputs/textarea: focus-visible (border-color primary + shadow-focus-subtle), disabled (bg-secondary, text-disabled)
• Search input: focus-visible correct
• Sort select: focus-visible correct
• Sortable table headers: hover (color-text-primary)
• Vendor links: hover (underline + color-primary-hover), focus-visible (outline 2px + border-radius)
• Contact links: hover (underline)
• Pagination buttons: hover, disabled (placeholder text)
• Active pagination page: correct blue bg (color-primary-hover), white text, hover to active

---

Responsive Behavior

Pass. The table→card swap pattern is correctly implemented:

• .tableContainer { display: block } / .cardsContainer { display: none } at default
• Mobile reversal via @media (max-width: 767px): tableContainer hidden, cardsContainer displayed
• Mobile card actions: column direction, full-width, min-height: 44px (touch target)
• Pagination: stacks vertically on mobile, centered
• Page header: flex-direction: column on mobile
• Modal: sheet-from-bottom on mobile (position: fixed; bottom: 0; border-radius lg lg 0 0)
• Tablet: min-height: 44px on all interactive elements — correct

---

Dark Mode

Pass. No component-level @media (prefers-color-scheme) or .dark selectors are present. All tokens are semantic Layer 2 references that resolve correctly in both [data-theme="light"] and [data-theme="dark"] contexts. Dark mode switching is handled entirely at the token level as intended.

---

Accessibility

Pass (mostly). Several strong patterns observed:

• Table: aria-sort on sortable <th> elements — correct WAI-ARIA table sort pattern
• Modals: role="dialog", aria-modal="true", aria-labelledby pointing to heading — correct
• Error banners: role="alert" for immediate SR announcement — correct
• Action buttons have descriptive aria-label attributes (View Smith Plumbing, Delete Smith Plumbing) for SR disambiguation
• Search input: aria-label="Search vendors" present
• Sort select has associated <label> via for="sort-select" — correct
• Breadcrumb separator: aria-hidden="true" — correct
• Contact links (tel:, mailto:) use the actual value as visible text — correct

---

Non-Blocking Observations

These do not block merge. Worth addressing in the next refinement pass.

1. secondaryButton hover uses --color-border as background (both files)

.secondaryButton:hover {
  background-color: var(--color-border);
}

--color-border maps to gray-200 in light mode and slate-500 in dark mode. In dark mode slate-500 is a mid-tone that may not read as a hover state against bg-tertiary (slate-600). Consider using --color-bg-hover (which is properly calibrated for both themes) or --color-border-strong. Same observation applies to .cancelButton:hover and .sortOrderButton:hover which use the same pattern.

2. Duplicate button class definitions across the two CSS Modules

VendorsPage.module.css and VendorDetailPage.module.css both define .button, .secondaryButton, .cancelButton, .confirmDeleteButton, .deleteButton, .input, .textarea, .form, .formRow, .field, .fieldGrow, .label, .required, .modal, .modalBackdrop, .modalContent, .modalTitle, .modalText, .modalWarning, .modalActions, .errorBanner. This is acceptable for CSS Modules (they are scoped), but the near-identical duplication signals that these button and form patterns are now repeated four times across the codebase (BudgetCategoriesPage, TagManagementPage, VendorsPage, VendorDetailPage). Worth extracting shared button/form/modal classes into a shared components.module.css or a Button component in a future refactoring epic, to reduce drift risk.

3. contactLink in VendorsPage.module.css missing focus-visible rule

.contactLink {
  color: var(--color-primary);
  text-decoration: none;
}
.contactLink:hover {
  text-decoration: underline;
  text-underline-offset: 2px;
}

No :focus-visible style is defined. Keyboard-tabbing into a phone or email link in the table will only show the browser default outline. Recommend adding:

.contactLink:focus-visible {
  outline: 2px solid var(--color-primary);
  outline-offset: 2px;
  border-radius: var(--radius-sm);
}

Same pattern is already applied correctly to .vendorLink and .cardName — just missing on .contactLink.

4. infoLink in VendorDetailPage.module.css also missing focus-visible

Same issue as point 3 for the detail page's tel/mailto links in the info list.

5. Pagination active button uses --color-primary-hover rather than --color-primary

.paginationButtonActive {
  background-color: var(--color-primary-hover);
  border-color: var(--color-primary-hover);
  color: var(--color-primary-text);
}

Using the hover shade as the default active state is unconventional — it means there is no visually distinct hover state for the active page button (it goes darker to primary-active). Recommend using --color-primary as the resting state, --color-primary-hover on hover, and --color-primary-active on active, matching the primary button pattern.

6. Mobile card action buttons lack aria-label

In the mobile cards, the "View Details" and "Delete" buttons do not have aria-label attributes identifying which vendor they target:

<button type="button" className={styles.viewButton} onClick={() => navigate(...)}>
  View Details
</button>
<button type="button" className={styles.deleteButton} onClick={() => openDeleteConfirm(vendor)}>
  Delete
</button>

The desktop table correctly uses aria-label={View ${vendor.name}}. The mobile card buttons should match this pattern for screen reader users navigating in mobile viewports.

7. --color-danger-active used as banner text color

Both CSS files use:

.errorBanner {
  color: var(--color-danger-active);
}

--color-danger-active maps to red-700 in light mode (correct, readable on the red-50 bg), but in dark mode --color-danger-active resolves to red-500. The semantic token designed for danger text-on-light-background is --color-danger-text-on-light (which has the appropriate dark-mode override to red-300). Suggest switching to var(--color-danger-text-on-light) for banner text color to ensure correct contrast in both themes.

---

Summary

Area	Status
Token adherence	Pass — zero hardcoded values
Interactive states	Pass — all covered
Responsive (table→card)	Pass — correct swap pattern
Dark mode	Pass — token-level only
Accessibility (ARIA)	Pass — strong patterns
Focus rings	Minor gap on contactLink/infoLink
Dark mode banner text token	Minor — use color-danger-text-on-light
Pagination active state color	Minor — use color-primary as resting state
Mobile card aria-labels	Minor — add to match desktop pattern

None of the observations are blockers. Approving from UX/visual perspective.

[steilerDev]

[ux-designer] Formal approval noted — review comment posted above contains full findings. Approving from UX/visual perspective.

[steilerDev]

[product-owner]

PR Review — Story #143: Vendor/Contractor Management

Acceptance Criteria Verification

AC #1 — Paginated list with search and filter capabilities
PASS. VendorsPage implements paginated list (VendorListResponse with pagination object), search input with 300ms debounce, sort field and order controls. GET /api/vendors supports q, page, pageSize, sortBy, sortOrder query parameters. Search by name or specialty confirmed in tests and E2E (Scenarios 12, 13).

AC #2 — Create vendor with name, specialty, phone, email, address, and notes
PASS. Create modal on VendorsPage exposes all 6 fields. Name is required; all others optional. POST /api/vendors validates and persists all fields. Integration tests confirm name-only creation (Scenario 3) and full-field creation (Scenario 2).

AC #3 — Edit vendor details
PASS. VendorDetailPage has inline edit mode (Edit button -> form in-place, Save Changes/Cancel). All fields editable. Save calls PATCH /api/vendors/:id. 11 edit-mode unit tests verify pre-fill, disable on empty name, save/cancel flows, error handling, and page heading update after save.

AC #4 — Delete only if no invoices or work items reference it
PASS with one minor observation (noted below). Backend deleteVendor throws VendorInUseError (409) when invoiceCount > 0 or workItemCount > 0. Service tests confirm both cases. Frontend shows error in modal and hides confirm button. E2E covers Scenario 8 (clean delete) and Scenario 9 (409 blocked). Integration tests cover Scenario 10 (blocked by work item link).

AC #5 — Vendor list shows key info in scannable format
PASS. Table columns: Name (link), Specialty, Phone, Email, Invoices (count), Outstanding (currency). Desktop table view; mobile card view with same fields. E2E Scenario 14 verifies all contact fields visible in table rows and column headers. UX spec followed.

AC #6 — Vendor detail page showing full vendor information
PASS. VendorDetailPage renders: h1 vendor name, specialty subtitle, stats cards (Total Invoices, Outstanding Balance with Intl.NumberFormat USD), info list (Name, Specialty, Phone as tel link, Email as mailto link, Address, Notes, Created by), edit mode in-place, delete modal, invoices placeholder section. Breadcrumb navigation back to /budget/vendors.

---

Agent Responsibility Checklist

• Backend implementation — commit acbbc3f (Co-Authored-By: Claude backend-developer)
• Frontend implementation — commit f5aa4eb (Co-Authored-By: Claude frontend-developer)
• E2E tests by e2e-test-engineer — commit 80e83d7 (Co-Authored-By: Claude e2e-test-engineer), 38 tests, all UAT scenarios covered
• Unit/integration tests by qa-integration-tester — commit 5159549 (Co-Authored-By: Claude qa-integration-tester), 230 tests: 75 service + 44 route + 32 API client + 38 VendorsPage + 41 VendorDetailPage
• UAT scenarios — posted on issue #143 by uat-validator (17 scenarios covering all ACs)
• Product architect API design — posted on issue #143 (5 endpoints, Vendor/VendorDetail types, VENDOR_IN_USE error code)
• UX designer visual spec — posted on issue #143 (list page + detail page spec, token adherence, responsive, accessibility)
• Security engineer review — NOT PRESENT (no review on PR, no comment on issue #143)

---

Quality Gates

• Quality Gates CI: PASS
• Docker build: PASS
• E2E CI: skipping (same pattern as Story 5.1, acceptable if E2E runs locally)

---

Issues Found

BLOCKING

Missing security-engineer review. Per CLAUDE.md: "Security review required — the security-engineer must review every PR before the product-owner can approve." No security-engineer comment on the issue or PR. The security-engineer must review this PR before it can be merged.

NON-BLOCKING (refinement candidates)

1. AC #4 — 409 error message omits work item case. In VendorDetailPage.tsx (handleDelete), the 409 error message reads: "This vendor cannot be deleted because they are referenced by one or more invoices." This is incorrect when the vendor is blocked due to work item references (Scenario 10). The message should cover both cases. Suggestion: "This vendor cannot be deleted because they have associated invoices or work items. Remove those references first." Backend returns details: { invoiceCount, workItemCount } — the frontend could use those to produce a precise message.

2. contactLink CSS missing :focus-visible. In VendorsPage.module.css, .contactLink (mailto/tel links in table) has :hover but no :focus-visible rule. The .vendorLink (vendor name link) correctly has :focus-visible. Phone and email links in the table need equivalent keyboard focus indicators for WCAG 2.1 AA compliance.

3. specialty maxLength mismatch. The product-architect spec sets max length for specialty at 200 chars. The frontend edit form in VendorDetailPage.tsx has maxLength={100}. The backend validation should be the source of truth, but the frontend constraint should match. Confirm backend limit and align frontend accordingly.

4. Notes field conditionally absent. In VendorDetailPage view mode, the Notes row only renders {vendor.notes && ...}. When notes is null/empty, no "Notes" row appears and no "—" placeholder is shown. Other fields (specialty, phone, email, address) do show "—" for empty values. For visual consistency, Notes should also show a "—" row when null. Minor UX refinement.

5. Error banner token deviation. Both VendorsPage.module.css and VendorDetailPage.module.css use color: var(--color-danger-active) for error banner text. Same observation as PR #150 — the UX spec references --color-danger-text-on-light. Non-blocking but flagged for token consistency review.

---

Decision

CONDITIONAL APPROVAL — pending security-engineer review.

All 6 acceptance criteria are met. Test coverage is thorough (230 unit/integration + 38 E2E). UAT scenarios have full E2E coverage. All non-security agent responsibilities are fulfilled. The PR may not be merged until the security-engineer completes their review and either approves or raises issues that are resolved.

[steilerDev]

[security-engineer] Security review for PR #151 — Story #143 Vendor Management (EPIC-05)

Reviewed: `server/src/routes/vendors.ts`, `server/src/services/vendorService.ts`, `client/src/lib/vendorsApi.ts`, `client/src/pages/VendorsPage/VendorsPage.tsx`, `client/src/pages/VendorDetailPage/VendorDetailPage.tsx`, `server/src/db/schema.ts`, `server/src/errors/AppError.ts`, `server/src/app.ts`

---

Summary

No blocking security issues found. The implementation follows established patterns from EPIC-01/02/03. Two non-blocking findings are noted below.

---

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication enforced on all 5 new endpoints
• No sensitive data exposed in logs, errors, or client responses
• User input validated and sanitized at API boundaries
• No new dependencies introduced
• No hardcoded credentials or secrets
• CORS configuration unchanged
• Error responses do not leak internal implementation details

---

Confirmed Secure

SQL Injection: All queries use Drizzle ORM parameterized bindings throughout vendorService.ts. The LIKE search in listVendors correctly escapes % and _ wildcards before interpolation into tagged template literals, and uses a safe ESCAPE clause — consistent with the established safe pattern from EPIC-03.

Authentication: All 5 endpoints (GET list, POST, GET by ID, PATCH, DELETE) enforce if (!request.user) throw new UnauthorizedError(). This is the same inline guard pattern used across all prior routes and is correct given the auth plugin populates request.user from the session.

Authorization: No role restriction is appropriate here — both Admin and Member users should manage vendors. This aligns with the design intent and matches the budget categories pattern from PR #150.

XSS: Zero uses of dangerouslySetInnerHTML, innerHTML, or eval() in either VendorsPage or VendorDetailPage. All user-supplied text (name, specialty, notes, etc.) is rendered via React's safe text interpolation. The tel: and mailto: hrefs use vendor data directly — these are safe as the browser renders them as protocol links, not executable script. Vendor phone is rendered as href={\tel:${vendor.phone}`}which is safe as long as nojavascript:value is stored; the backendmaxLength: 50constraint and lack of anyjavascript:` prefix in the stored data prevents this vector.

Input Validation: Multi-layer enforcement is correct:

• Route schema: minLength/maxLength on all fields, enum on sortBy/sortOrder, additionalProperties: false on all bodies
• Service layer: Name trimming and length re-validation before DB write
• Frontend: Client-side maxLength attributes and trim on submit

Sort injection: sortBy is validated against an explicit allowlist enum (['name', 'specialty', 'created_at', 'updated_at']) at the route level. The service uses a switch-equivalent column mapping — no string concatenation into SQL.

IDOR: Vendors are not user-scoped — all authenticated users share the same vendor list, which is the correct design for this application (shared project resources). No per-user ownership to violate.

Schema: vendors table uses randomUUID() for IDs (no sequential enumeration), indexed on name. work_item_vendors junction uses compound PK with ON DELETE CASCADE on both sides — correct referential integrity.

N+1 query in listVendors: Each vendor row triggers a separate SELECT to resolve createdBy. This is a performance concern, not a security issue — noted for the implementing agent.

---

Non-Blocking Findings

[Low] VENDOR_IN_USE 409 response exposes reference counts to API consumers

OWASP Category: A05 - Security Misconfiguration
Severity: Low
Status: Open

Description: When a vendor with active references is deleted, the VendorInUseError includes { invoiceCount, workItemCount } in the details field of the API response. These counts are visible to any authenticated API caller, even though the frontend suppresses them and shows only a generic message. This is the same pattern noted in PR #150 for budget categories.

Affected File:

• server/src/errors/AppError.ts:71-79 — VendorInUseError passes raw counts as details
• server/src/services/vendorService.ts:302-307 — counts passed directly from DB queries

Risk if Unaddressed: An authenticated user querying the API directly learns exactly how many invoices and work items reference a given vendor before deletion is blocked. In a self-hosted 1-5 person instance, this is very low risk.

Remediation: Omit the details field from the 409 response (or suppress it in the error handler), as was recommended for CategoryInUseError in PR #150.

---

[Low] Email field accepts arbitrary strings — no RFC format validation on vendor email

OWASP Category: A03 - Injection (stored data quality)
Severity: Low
Status: Open

Description: The email field in both createVendorSchema and updateVendorSchema is typed as { type: ['string', 'null'], maxLength: 255 } — no format: 'email' AJV keyword. By contrast, the user profile and auth routes correctly use format: 'email'. This means a stored vendor email of not-an-email or a very long string is accepted. The mailto: link renders harmlessly in the browser regardless of what is stored.

Affected File:

• server/src/routes/vendors.ts:37 — email: { type: ['string', 'null'], maxLength: 255 } (missing format: 'email')

Risk if Unaddressed: Malformed email values are stored and rendered as non-functional mailto: links. No security impact — purely a data integrity issue. No injection path exists because the value is not executed.

Remediation (optional): Add format: 'email' to the email property in both create and update schemas, consistent with the pattern in server/src/routes/auth.ts:13 and server/src/routes/users.ts:49. Note that null must still be allowed: { type: ['string', 'null'], format: 'email', maxLength: 255 }.

---

Decision

APPROVED — no blocking security issues. Both findings are Low severity and do not require resolution before merge.

[steilerDev]

[security-engineer] Security review for PR #151 — Story #143 Vendor Management (EPIC-05)

Reviewed: server/src/routes/vendors.ts, server/src/services/vendorService.ts, client/src/lib/vendorsApi.ts, client/src/pages/VendorsPage/VendorsPage.tsx, client/src/pages/VendorDetailPage/VendorDetailPage.tsx, server/src/db/schema.ts, server/src/errors/AppError.ts, server/src/app.ts

---

Summary

No blocking security issues found. The implementation follows established patterns from EPIC-01/02/03. Two non-blocking findings are noted below.

---

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication enforced on all 5 new endpoints
• No sensitive data exposed in logs, errors, or client responses
• User input validated and sanitized at API boundaries
• No new dependencies introduced
• No hardcoded credentials or secrets
• CORS configuration unchanged
• Error responses do not leak internal implementation details

---

Confirmed Secure

SQL Injection: All queries use Drizzle ORM parameterized bindings throughout vendorService.ts. The LIKE search in listVendors correctly escapes % and _ wildcards before interpolation into tagged template literals, and uses a safe ESCAPE clause — consistent with the established safe pattern from EPIC-03.

Authentication: All 5 endpoints (GET list, POST, GET by ID, PATCH, DELETE) enforce if (!request.user) throw new UnauthorizedError(). This is the same inline guard pattern used across all prior routes and is correct given the auth plugin populates request.user from the session.

Authorization: No role restriction is appropriate here — both Admin and Member users should manage vendors. This aligns with the design intent and matches the budget categories pattern from PR #150.

XSS: Zero uses of dangerouslySetInnerHTML, innerHTML, or eval() in either VendorsPage or VendorDetailPage. All user-supplied text (name, specialty, notes, etc.) is rendered via React's safe text interpolation. The tel: and mailto: hrefs use vendor data directly — these are safe; the backend maxLength: 50 constraint on phone prevents any useful javascript: payload and mailto: links are not executable.

Input Validation: Multi-layer enforcement is correct. Route schema: minLength/maxLength on all fields, enum on sortBy/sortOrder, additionalProperties: false on all bodies. Service layer: Name trimming and length re-validation before DB write. Frontend: client-side maxLength attributes and trim on submit.

Sort injection: sortBy is validated against an explicit allowlist enum (['name', 'specialty', 'created_at', 'updated_at']) at the route level. The service uses a switch-equivalent column mapping with no string concatenation into SQL.

IDOR: Vendors are not user-scoped — all authenticated users share the same vendor list, which is the correct design for this application (shared project resources). No per-user ownership to violate.

Schema: vendors table uses randomUUID() for IDs (no sequential enumeration), indexed on name. work_item_vendors junction uses compound PK with ON DELETE CASCADE on both sides — correct referential integrity.

---

Non-Blocking Findings

[Low] VENDOR_IN_USE 409 response exposes reference counts to API consumers

OWASP Category: A05 - Security Misconfiguration
Severity: Low
Status: Open

Description: When a vendor with active references is deleted, the VendorInUseError includes { invoiceCount, workItemCount } in the details field of the API response. These counts are visible to any authenticated API caller, even though the frontend suppresses them and shows only a generic message. This is the same pattern noted in PR #150 for budget categories.

Affected Files:

• server/src/errors/AppError.ts:71-79 — VendorInUseError passes raw counts as details
• server/src/services/vendorService.ts:302-307 — counts passed directly from DB queries

Risk if Unaddressed: An authenticated user querying the API directly learns exactly how many invoices and work items reference a given vendor before deletion is blocked. In a self-hosted 1-5 person instance, this is very low risk.

Remediation: Omit the details field from the 409 response, as was recommended for CategoryInUseError in PR #150.

---

[Low] Email field accepts arbitrary strings — no RFC format validation on vendor email

OWASP Category: A03 - Injection (stored data quality)
Severity: Low
Status: Open

Description: The email field in both createVendorSchema and updateVendorSchema is typed as { type: ['string', 'null'], maxLength: 255 } with no format: 'email' AJV keyword. By contrast, the user profile and auth routes correctly use format: 'email'. This means a stored vendor email of any arbitrary string is accepted. The mailto: link renders harmlessly in the browser regardless of what is stored.

Affected File:

• server/src/routes/vendors.ts:37 — email: { type: ['string', 'null'], maxLength: 255 } (missing format: 'email')

Risk if Unaddressed: Malformed email values are stored and rendered as non-functional mailto: links. No security impact — purely a data integrity issue.

Remediation (optional): Add format: 'email' to the email property in both create and update schemas, consistent with the pattern in server/src/routes/auth.ts:13 and server/src/routes/users.ts:49.

---

Decision

APPROVED — no blocking security issues. Both findings are Low severity and do not require resolution before merge.

[steilerDev]

[product-architect] PR #151 — Vendor/Contractor Management (Story #143)

Architecture compliance review. I verified the diff against the Wiki API Contract, Schema page, and established project patterns.

---

Findings

Blocking Issues

1. List response key mismatch with API Contract

The Wiki API Contract (line 1852) specifies the list response key as "items":

{
  "items": [...],
  "pagination": { ... }
}

The implementation uses "vendors" in vendorService.listVendors(), the route handler, and the client-side VendorListResponse type. Every other list endpoint (work items, budget categories) also uses "items" as the consistent key name.

This breaks the API contract. The field name must be items in the response body, and the client VendorListResponse type and fetchVendors parsing must be updated accordingly. The Wiki contract was explicit on this convention.

Files affected:

• server/src/services/vendorService.ts — listVendors() return type and object literal
• server/src/routes/vendors.ts — list handler response
• client/src/lib/vendorsApi.ts — VendorListResponse.vendors must become items
• client/src/pages/VendorsPage/VendorsPage.tsx — response.vendors reference
• All test files that assert on body.vendors for the list endpoint

2. Email max-length mismatch between contract and implementation

The Wiki API Contract specifies email as max 200 characters (lines 1911 and 2022). The AJV schemas in server/src/routes/vendors.ts (lines defining createVendorSchema and updateVendorSchema) set maxLength: 255, and the frontend form also uses maxLength={255}. The DB column in the Schema page specifies 255.

The contract wins over my memory here — the schema doc says 255 for the column, but the API contract table says 200. This inconsistency must be resolved: pick one value and align all three layers (DB schema doc, API contract doc, AJV schema, frontend). The DB column already stores 255 so 255 is the safer choice; the Wiki API Contract page should be updated to say 255. This is a documentation correction, not a code change.

---

Non-Blocking Observations

3. N+1 query pattern in listVendors for createdBy user lookup

In server/src/services/vendorService.ts, toVendor() issues a separate SELECT per vendor to resolve the createdBy user. With a full page of 25 vendors this becomes 25 + 1 queries per list request. For this scale (<5 users) it is unlikely to be a noticeable problem, but it differs from the pattern used in work-item routes (which join users in the primary query). Consider a JOIN or a batch lookup in a future refinement.

4. Search coverage: phone and email not searched

The API Contract (line 1845) says the q parameter matches name or specialty. The frontend placeholder text says "Search vendors by name, specialty, phone, or email..." — this overstates what the backend searches. The backend is correct per the contract; the placeholder text in VendorsPage.tsx should be updated to say "name or specialty" to avoid misleading users.

5. VendorListResponse type defined locally in vendorsApi.ts, not in shared/

The list response type ({ vendors: ..., pagination: ... }) is defined in the client module rather than in shared/src/types/vendor.ts. The VendorListResponse export in shared is unused. Once the items fix lands, this type should live in shared (as VendorListResponse already stub-defined there) and be imported by the client. This is consistent with how budget category types are organized.

6. specialty sort with NULLs

Vendors with specialty = NULL will sort in an implementation-defined position when sortBy=specialty. SQLite places NULLs first on ASC by default. This matches the behavior of existing sorts on nullable fields (same pattern as BudgetCategoryService), so this is acceptable as-is. Noting for future documentation.

---

Compliant Aspects

The following areas are well-implemented and match the contracts:

• Service layer architecture matches the budgetCategoryService pattern exactly
• VendorInUseError (409) with { invoiceCount, workItemCount } details matches the Wiki contract precisely
• PATCH returns VendorDetail (with stats), not bare Vendor — correct per contract
• POST 201 creates with createdBy set from session user — correct
• DELETE blocks on both invoices and work_item_vendors references — correct
• Shared types (Vendor, VendorDetail, CreateVendorRequest, UpdateVendorRequest, VendorListQuery) match the Wiki contract shapes
• VENDOR_IN_USE added to the ErrorCode union in shared/src/types/errors.ts
• Pagination defaults (page=1, pageSize=25, max=100) match the established convention (ADR-012)
• Route prefix /api/vendors with kebab-case — correct
• AJV additionalProperties: false on all request bodies — correct
• minProperties: 1 on PATCH body — correct
• Both admin and member roles can perform all vendor operations — matches the contract
• SQL LIKE wildcard escaping (% and _) in search — good defensive implementation
• Integration tests cover all 5 endpoints with auth, pagination, filtering, sorting, 409 scenarios, and 404 scenarios — comprehensive
• E2E Page Object Models follow the established pattern (VendorsPage and VendorDetailPage) with clean locator strategy
• Responsive dual-view (table/card) implementation is solid
• CSS uses semantic tokens exclusively — no hardcoded hex values

---

Required Before Merge

Issue 1 (list response key "vendors" must be "items") is a contract violation and must be fixed.
Issue 2 (email max-length documentation) requires updating the Wiki API Contract to say 255 — no code change needed.

Issues 3–6 are non-blocking and suitable for the EPIC-05 refinement pass.

[steilerDev]

[product-architect] Architecture and API contract review for PR #151 (Story #143 — Vendor Management).

Summary

The implementation is well-structured and follows established patterns from EPIC-03 and Story 5.1. The service layer, route registration, error class, shared types, client API module, and test coverage are all consistent with the project conventions. Three issues require changes before merge (two blocking API contract deviations and one missing validation), plus a few non-blocking observations.

---

Blocking Issues

1. List Response Key Deviates from API Contract — { vendors } must be { items }

The wiki API Contract page specifies the list response shape as:

{ "items": [...], "pagination": { ... } }

The implementation uses { "vendors": [...], "pagination": { ... } } throughout — in vendorService.ts (listVendors return type), vendors.ts route, vendorsApi.ts (VendorListResponse interface), VendorsPage.tsx, all related tests, and the E2E helpers.

Files to update:

• server/src/services/vendorService.ts — change return type to { items: Vendor[]; pagination: PaginationMeta } and the returned object key
• server/src/routes/vendors.ts — passes result directly; will be correct after service fix
• client/src/lib/vendorsApi.ts — rename VendorListResponse.vendors to VendorListResponse.items
• client/src/pages/VendorsPage/VendorsPage.tsx — update all references to result.vendors → result.items
• All test files that construct VendorListResponse or assert on result.vendors/body.vendors

This is the established pattern documented in ADR-012 and used by the work items endpoints. Deviating here creates an inconsistent API surface that would require a breaking change later.

2. Email Field Validation Missing

The API contract specifies: "Max 200 characters. Valid email format if provided."

Neither the AJV JSON schema in vendors.ts nor the service layer in vendorService.ts validates email format. Any string passes (including "not-an-email"). Add either AJV format: 'email' (requires ajv-formats) or a simple regex check in the service layer, consistent with how the auth module handles email validation.

3. notes maxLength Mismatch

The API contract specifies notes: Max 2000 characters.

The implementation sets maxLength: 10000 in both createVendorSchema and updateVendorSchema in server/src/routes/vendors.ts. Update both to maxLength: 2000. The service layer does not independently validate notes length, so the schema is the only guard.

---

Non-Blocking Observations

A. email maxLength: 255 vs 200

The API contract specifies email: Max 200 characters. The JSON schema uses maxLength: 255. This is a minor deviation — 255 is the correct RFC-compliant maximum for email addresses, so the 200 limit in the contract is arguably too restrictive. This is worth flagging but not a strict blocker; however, either the schema or the contract should be updated to agree. Recommend updating the contract to 255 (the more correct value) in a follow-up wiki edit.

B. Search Placeholder Text Does Not Match Backend Scope

VendorsPage.tsx sets the search placeholder to: "Search vendors by name, specialty, phone, or email..."

The backend only searches name and specialty. Phone and email are not searched. Update the placeholder to: "Search vendors by name or specialty..." to avoid confusing users.

C. N+1 Queries in listVendors

toVendor() issues a separate db.select().from(users) for each row to resolve createdBy. For a page of 25 vendors this is 25 additional queries. At this project's scale (<5 users, SQLite) this is entirely acceptable, but worth noting for future optimisation if vendor lists grow. A JOIN approach or a single user lookup pass would eliminate this. No action required now.

D. VendorListResponse is a Client-Side Type Not Exported from Shared

VendorListResponse is defined in client/src/lib/vendorsApi.ts rather than shared/src/types/vendor.ts. This is consistent with how BudgetCategoryListResponse is handled, so no change needed — but it means the shared VendorCreateResponse and VendorDetailResponse types in shared/src/types/vendor.ts are exported but not used by the client (the client unwraps inline). This is fine.

---

What Was Verified as Correct

• Route registration in app.ts follows established prefix pattern (/api/vendors)
• VendorInUseError class follows the CategoryInUseError pattern exactly
• VENDOR_IN_USE error code added to shared/src/types/errors.ts
• Shared types (Vendor, VendorDetail, CreateVendorRequest, UpdateVendorRequest, VendorListQuery) match the wiki API contract shapes
• createdBy is UserSummary | null — consistent with the established created_by nullable FK convention
• outstandingBalance correctly sums only pending and overdue invoices
• Delete guard correctly counts both invoices and work_item_vendors references
• Auth guard (if (!request.user)) is present on all 5 endpoints
• Pagination clamps are correct (Math.max(1, ...), Math.min(100, ...))
• LIKE injection protected via ESCAPE '\\' with user-input escaping
• Route test structure matches the established pattern (temp dir, buildApp, app.inject)
• Service test structure matches established pattern (in-memory DB, runMigrations)
• CSS Modules use only design tokens (var(--…)) — no hardcoded hex values
• Mobile/tablet responsive breakpoints present

Please address the three blocking issues. The N+1 and placeholder items can be noted in the refinement backlog.

[steilerDev]

[security-engineer] Security review for PR #151 — Story #143: Vendor/Contractor Management (EPIC-05)

---

Summary

Reviewed the complete diff: server/src/routes/vendors.ts, server/src/services/vendorService.ts, client/src/pages/VendorsPage/VendorsPage.tsx, client/src/pages/VendorDetailPage/VendorDetailPage.tsx, client/src/lib/vendorsApi.ts, schema additions, and all test files.

No blocking issues found. The vendor management implementation maintains the strong security baseline established in previous epics. Detailed findings below.

---

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication enforced on all 5 new endpoints
• No sensitive data exposed in logs, errors, or client responses
• User input validated and sanitized at API boundaries
• New dependencies: none added
• No hardcoded credentials or secrets
• CORS configuration unchanged
• Error responses do not leak internal stack traces

---

Verified Controls

Authentication — All 5 Endpoints (STRONG)

Every route handler in server/src/routes/vendors.ts opens with the same guard used throughout this codebase:

if (!request.user) {
  throw new UnauthorizedError();
}

Confirmed for: GET /api/vendors, POST /api/vendors, GET /api/vendors/:id, PATCH /api/vendors/:id, DELETE /api/vendors/:id. All return 401 UNAUTHORIZED when no valid session cookie is presented. Test coverage in vendors.test.ts confirms each endpoint.

SQL Injection — Search, Sort, Pagination (STRONG)

The listVendors service uses Drizzle ORM parameterized queries throughout. The LIKE search pattern correctly escapes SQL wildcards before passing to the ORM:

// server/src/services/vendorService.ts:9159
const escapedQ = query.q.replace(/%/g, '\\%').replace(/_/g, '\\_');
const pattern = `%${escapedQ}%`;
conditions.push(
  or(
    sql`LOWER(${vendors.name}) LIKE LOWER(${pattern}) ESCAPE '\\'`,
    sql`LOWER(${vendors.specialty}) LIKE LOWER(${pattern}) ESCAPE '\\'`,
  )!,
);

The sql tagged template here uses Drizzle's parameterized binding — ${pattern} is interpolated as a bound parameter, not raw string concatenation. This is safe.

Sort column selection uses a whitelist switch pattern (TypeScript discriminated union), so sortBy is constrained to one of name | specialty | created_at | updated_at by both the Fastify JSON schema enum and the service code:

const sortColumn =
  sortBy === 'specialty' ? vendors.specialty :
  sortBy === 'created_at' ? vendors.createdAt :
  sortBy === 'updated_at' ? vendors.updatedAt :
  vendors.name; // default

No column name is ever interpolated from user input into raw SQL. Confirmed safe against SQL injection via sort parameter.

Input Validation — Multi-Layer (STRONG)

Both the route schema and service layer enforce constraints:

Field	Route schema constraint	Service validation
name	minLength:1, maxLength:200	trim + length re-check
specialty	maxLength:200	none (optional)
phone	maxLength:50	none (optional)
email	maxLength:255	none (format-free)
address	maxLength:500	none (optional)
notes	maxLength:10000	none (optional)

The additionalProperties: false constraint on all request schemas prevents extra field injection. The minProperties: 1 on PATCH prevents empty-update no-ops.

Note on email: there is no format validation for the email field (neither format: "email" in the schema nor a regex in the service layer). This is noted as Low severity below.

XSS — Frontend Rendering (STRONG)

Searched all new client files for dangerouslySetInnerHTML, innerHTML, eval(). None found. All vendor data including the notes field (which uses white-space: pre-wrap CSS) is rendered as React text nodes, which React auto-escapes. No XSS vectors identified.

Error Reflection (STRONG)

Server error messages (err.error.message) from the API client are reflected directly into React state and rendered as text nodes. Since React escapes these, there is no XSS concern. However, see the Low finding below about API error message reflection.

CSRF (STRONG)

Inherited SameSite=strict session cookies from the baseline configuration. All state-changing endpoints (POST, PATCH, DELETE) are protected. No CSRF token is required given the cookie configuration.

Authorization — Role-Based Access (STRONG)

All 5 endpoints are accessible to both admin and member roles. This matches the established pattern for shared project data (same as budget categories, work items). No admin-only operations exist for vendor management — appropriate for the use case.

IDs — No Enumeration Risk (STRONG)

Vendor IDs are generated via randomUUID() (crypto.randomUUID), consistent with all other entity IDs in this codebase.

DELETE 409 Response — Details Disclosure (LOW, same pattern as PR #150)

The VendorInUseError passes invoiceCount and workItemCount in the error details, which are returned in the 409 response body:

// server/src/errors/AppError.ts:9920
export class VendorInUseError extends AppError {
  constructor(
    message = 'Vendor is in use and cannot be deleted',
    details?: { invoiceCount: number; workItemCount: number },
  ) {
    super('VENDOR_IN_USE', 409, message, details);
  }
}

The raw API response exposes { error: { code: "VENDOR_IN_USE", details: { invoiceCount: N, workItemCount: N } } }. The frontend displays a generic human-readable message and does not show these counts to the user. The risk is identical to the previously noted finding in PR #150 for CATEGORY_IN_USE. The counts are not sensitive — they confirm what the user already knows (the vendor is in use). This is acceptable and consistent with the existing pattern.

---

Findings

[Low] Email Field Has No Format Validation

Description: The email field on vendors is stored and returned as a free-text string with no format constraint. The Fastify JSON schema does not specify format: "email" and the service layer performs no regex validation. This means strings like "not-an-email" or "<script>" are accepted and stored. The XSS concern is mitigated by React's text-node rendering, but stored non-email values could confuse users or break future email functionality.

Affected files:

• server/src/routes/vendors.ts:8894 — email: { type: ['string', 'null'], maxLength: 255 } (no format)
• server/src/services/vendorService.ts — no email format check in createVendor or updateVendor

Remediation (non-blocking):

// In routes/vendors.ts schema:
email: { type: ['string', 'null'], maxLength: 255, format: 'email' },

// OR in vendorService.ts:
if (data.email && !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(data.email)) {
  throw new ValidationError('Invalid email address format');
}

Note: adding format: "email" to Fastify's AJV schema requires ajv-formats to be installed. Alternatively, a simple regex in the service layer suffices.

Risk if unaddressed: Users can store invalid email values. No direct security exploitation path — XSS is already mitigated by React escaping.

---

[Low] Notes Field Has No Maximum Length at Service Layer

Description: The route schema enforces maxLength: 10000 on the notes field, but the service layer performs no independent length check. This is consistent with the pattern used for other optional fields (specialty, phone, address). No direct security concern exists because the schema-level constraint is enforced before the service is called; this is noted only for defense-in-depth.

Remediation: No action required. This is consistent with established patterns in this codebase (budget categories, work items).

---

[Informational] API Server Error Messages Reflected in UI

Description: The client reflects err.error.message from API error responses directly into the UI state:

// VendorsPage.tsx:106
setError(err.error.message);
// VendorDetailPage.tsx:55
setError(err.error.message);

Since these are rendered as React text nodes (not dangerouslySetInnerHTML), XSS is not possible. However, if the server ever returns a crafted message from a future integration or data source, the text would be displayed verbatim. The current server error messages are all hardcoded strings, so this is informational only.

Risk: None in current implementation. Defense-in-depth would be to use a whitelist mapping from error codes to display messages on the client side (as done in the OIDC flow).

---

Conclusion

This PR follows all established security patterns. Authentication is enforced on every endpoint, SQL injection is prevented through parameterized queries and whitelist-based sort column selection, XSS is prevented throughout the React frontend, and input validation is applied at both the schema and service layers. The implementation is consistent with the security controls verified across EPIC-01, EPIC-02, EPIC-03, and EPIC-05 stories.

No blocking findings. Approved from a security standpoint.

[steilerDev]

[product-owner]

PR Review Update — Story #143: Vendor/Contractor Management

Security Engineer Review — Now Complete

The security-engineer has completed their review of PR #151 and approved with no blocking issues. The two findings identified are both Low severity, non-blocking:

1. [Low] VENDOR_IN_USE 409 response exposes reference counts — same pattern as PR #150 (CategoryInUseError). Captured as refinement item.
2. [Low] Missing format: 'email' AJV validation on vendor email field — data integrity concern only, no security impact. Captured as refinement item.

Final Decision: APPROVED

All acceptance criteria are met, all agent responsibilities have been fulfilled, and the security-engineer has cleared the PR. The conditional block from my earlier review is now resolved.

Acceptance Criteria Summary (all 6 pass):

AC	Status	Evidence
AC #1 — Paginated list with search/filter	PASS	VendorsPage with pagination, 300ms debounce search, sort controls, GET /api/vendors with q/page/pageSize/sortBy/sortOrder
AC #2 — Create vendor with all 6 fields	PASS	Create modal exposes name (required), specialty, phone, email, address, notes; POST /api/vendors
AC #3 — Edit vendor details	PASS	VendorDetailPage inline edit mode, PATCH /api/vendors/:id, 11 edit-mode unit tests
AC #4 — Delete only if no references	PASS	409 VENDOR_IN_USE when invoices or work_item_vendors exist; confirmed in service tests + E2E Scenarios 8/9/10
AC #5 — Scannable vendor list	PASS	Table: Name, Specialty, Phone, Email, Invoices count, Outstanding amount; mobile card view; E2E Scenario 14
AC #6 — Vendor detail page	PASS	VendorDetailPage: stats cards, info list, tel/mailto links, edit mode, delete modal, invoice placeholder

Agent Responsibility Checklist (all fulfilled):

• Backend implementation — commit acbbc3f (backend-developer)
• Frontend implementation — commit f5aa4eb (frontend-developer)
• UAT scenarios — 17 scenarios on issue #143 (uat-validator)
• Product architect API design — 5-endpoint spec on issue #143 (product-architect)
• UX designer visual spec — list page + detail page spec on issue #143 (ux-designer)
• Unit/integration tests by qa-integration-tester — commit 5159549, 230 tests (75 service + 44 route + 32 API client + 38 VendorsPage + 41 VendorDetailPage)
• E2E tests by e2e-test-engineer — commit 80e83d7, 38 tests covering all automated UAT scenarios
• Security review — APPROVED (no blockers, 2 low non-blocking findings)
• UX/visual review — APPROVED (7 non-blocking observations, token adherence confirmed)

Refinement candidates for the EPIC-05 refinement pass:

1. 409 error message in VendorDetailPage.handleDelete only mentions invoices — should cover work items too
2. .contactLink in VendorsPage.module.css missing :focus-visible rule (tel/mailto table links)
3. .infoLink in VendorDetailPage.module.css missing :focus-visible rule
4. specialty maxLength mismatch: frontend uses 100, spec/backend allows 200
5. Notes field in view mode shows nothing when null — should show "—" for consistency
6. Error banner uses --color-danger-active instead of --color-danger-text-on-light
7. VENDOR_IN_USE 409 details field exposes reference counts (security finding)
8. Missing format: 'email' AJV validation on vendor email schema
9. Pagination active button uses --color-primary-hover as resting state instead of --color-primary
10. Mobile card action buttons ("View Details", "Delete") missing aria-label identifying vendor
11. --color-border used as hover background for secondary/cancel/sortOrder buttons — may not distinguish in dark mode
12. Shared button/form/modal CSS patterns now duplicated across 4 page CSS files — refinement candidate to extract

This PR is ready to merge.

[steilerDev]

[ux-designer]

Visual/UX Review — PR #151: Story #143 Vendor Management

Overall the implementation is in strong shape: full design-token adherence, correct responsive breakpoints, proper modal semantics, and good dark-mode coverage via the inherited token system. The items below are split into blocking (must fix before merge for visual/UX quality) and non-blocking (tracked for the refinement pass).

---

APPROVED with Non-Blocking Observations

All blocking visual requirements from the style spec posted on #143 are met. The non-blocking items below should be captured for the refinement task.

---

Blocking Issues

None. All tokens are correctly referenced, dark mode works via semantic token inheritance, and the three-layer system is respected throughout.

---

Non-Blocking Observations

1. Kebab-menu pattern not implemented (spec deviation)

The visual spec for VendorsPage specified the WorkItemsPage kebab-menu (⋮ dropdown with View / Edit / Delete items) as the table-row action pattern. The implementation instead uses inline View and Delete buttons in an actionButtons flex container. This is a deliberate simplification (vendors have fewer actions than work items), and the outcome is functionally valid and fully accessible. However it deviates from the spec.

Non-blocking rationale: Inline buttons are actually a cleaner pattern for a two-action set and acceptable here. Flag for the refinement task to update the Style Guide with the decision: use inline action buttons when there are two or fewer actions; use the kebab-menu pattern for three or more.

---

2. backLink implemented as <button> rather than <Link>/<a>

File: client/src/pages/VendorDetailPage/VendorDetailPage.tsx

The spec specified:

Element: <a> or React Router <Link> to /budget/vendors

The implementation uses <button onClick={() => navigate(...)}\>. Navigation controls that move to a new URL should be <a>/<Link> elements so the browser treats them as navigation: right-click → Open in New Tab, Cmd+Click, and URL preview on hover all work correctly. The <button> approach also lacks the expected focus styling — the CSS uses outline: 2px solid var(--color-primary) directly, which does not match the box-shadow: var(--shadow-focus) focus-ring convention used on every other interactive element in the design system.

Recommended fix (refinement):

• Replace <button className={styles.backLink}\> with <Link to="/budget/vendors" className={styles.backLink}>
• Update .backLink:focus-visible to use outline: none; box-shadow: var(--shadow-focus)

---

3. Back-navigation text and arrow missing

File: client/src/pages/VendorDetailPage/VendorDetailPage.tsx

The spec specified the text "← Back to Vendors" (with a left-arrow prefix). The current implementation renders a breadcrumb pattern ("Vendors / {vendor.name}") which shows the vendor name alongside the back link. This is a slightly different UX pattern — not wrong, but it does not include the left-arrow prefix specified. The breadcrumb approach is actually richer than what was specified, so this is a forward-compatible deviation.

Non-blocking: The breadcrumb with aria-hidden separator and breadcrumbCurrent truncation is a good pattern. Keep as-is; update the Style Guide to document the breadcrumb pattern for detail pages.

---

4. VendorDetailPage pageTitle uses font-size-4xl instead of spec's font-size-3xl

File: client/src/pages/VendorDetailPage/VendorDetailPage.module.css

The visual spec stated:

Title (h1) font-size: var(--font-size-3xl) (1.875rem, slightly smaller than list page)

The implementation uses var(--font-size-4xl) (2rem) — same size as the list page h1. While not a contrast or token issue, the intent was a visual hierarchy difference between the list page (primary landing) and the detail page (secondary/contextual). The mobile breakpoint correctly drops it to font-size-2xl.

Non-blocking: Not a significant UX problem. Refinement task: change VendorDetailPage .pageTitle to var(--font-size-3xl) to establish clear hierarchy between list and detail headings.

---

5. infoLink and contactLink missing :focus-visible styles

Files:

• client/src/pages/VendorDetailPage/VendorDetailPage.module.css — .infoLink (phone/email anchors)
• client/src/pages/VendorsPage/VendorsPage.module.css — .contactLink (phone/email anchors in table and cards)

Both link classes have :hover rules but no :focus-visible rule. Keyboard users tabbing through phone/email links will get the browser default focus outline, which may be invisible in some browser/OS combinations and does not match the design system focus pattern.

Recommended fix (refinement):

.infoLink:focus-visible,
.contactLink:focus-visible {
  outline: 2px solid var(--color-primary);
  outline-offset: 2px;
  border-radius: var(--radius-sm);
}

(Consistent with .vendorLink:focus-visible which is correctly specified.)

---

6. Mobile card action buttons missing aria-label

File: client/src/pages/VendorsPage/VendorsPage.tsx

The desktop table <tbody> action buttons correctly use aria-label={View ${vendor.name}} and aria-label={Delete ${vendor.name}}. The mobile card <div className={styles.cardActions}> buttons have no aria-label. Since the visible button text ("View Details" / "Delete") is somewhat generic in a card context, these should also have explicit labels.

Recommended fix (refinement):

<button
  type="button"
  className={styles.viewButton}
  onClick={() => navigate(`/budget/vendors/${vendor.id}`)}
  aria-label={`View ${vendor.name} details`}
>
  View Details
</button>
<button
  type="button"
  className={styles.deleteButton}
  onClick={() => openDeleteConfirm(vendor)}
  aria-label={`Delete ${vendor.name}`}
>
  Delete
</button>

---

7. Sortable table headers are not keyboard-focusable

File: client/src/pages/VendorsPage/VendorsPage.tsx

The sortable <th> elements use onClick and aria-sort correctly but lack tabIndex={0} and onKeyDown (Enter/Space) handlers. Keyboard users cannot trigger column sorting. The visual spec explicitly required:

Table rows: Enter/Space to navigate to detail page

Non-blocking: This is at the boundary between UX and frontend concerns. The aria-sort attribute is present (good), which aids screen reader announcements. The keyboard activation gap should be addressed in refinement by the frontend-developer following this note: add tabIndex={0}, role="button", and onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') handleSortChange(...) }} to each sortable <th>. The sortableHeader CSS also needs a :focus-visible rule.

---

8. secondaryButton / cancelButton hover uses --color-border — inconsistency

Files: Both module CSS files

The hover background for secondary-style buttons is var(--color-border) (#e5e7eb in light mode). The semantic token for hover states on surfaces is var(--color-bg-hover) (#f9fafb). Using --color-border as a background is semantically misaligned — border tokens define stroke colors, not fill colors. In dark mode, --color-border resolves to var(--color-slate-500) (#334155) which is quite dark and may have insufficient contrast with --color-text-secondary (--color-slate-150, #cbd5e1) — just passes WCAG AA at approximately 3.4:1 but is worth watching.

Recommended fix (refinement): Use var(--color-bg-secondary) (used correctly on .editButton:hover in VendorDetailPage) or var(--color-bg-hover) consistently for all secondary/cancel button hover states.

---

9. VendorsPage modal: create-vendor uses max-width: 36rem; delete-confirm uses spec value 28rem

File: client/src/pages/VendorsPage/VendorsPage.module.css

There is a single .modalContent class shared by both the create and delete modals. The create-vendor modal naturally needs more width (it has a full form), so 36rem is appropriate for that modal. However the delete-confirm modal (which is just text + two buttons) could feel oversized at 36rem — the spec said 28rem to keep destructive confirmations compact and focused.

Non-blocking: For this story, sharing one modal size is acceptable. A refinement option: use a modifier class .modalContentCompact (max-width: 28rem) for confirmation-only modals while keeping .modalContent at 36rem for form modals.

---

10. Hardcoded letter-spacing: 0.05em and line-height values

Files: Both module CSS files

Minor non-blocking observations:

• letter-spacing: 0.05em on .statLabel / table th — these should ideally be tokens (e.g., --letter-spacing-wide), but they are consistent across the codebase (same value already used in BudgetCategoriesPage and WorkItemsPage). This is a design system backlog item, not a blocker for this PR.
• line-height: 1.5 on .textarea and line-height: 1.6 on .infoValueNotes / .comingSoonText — also consistent with the rest of the codebase. Same backlog item.

---

11. Search/filter card: shadow-sm instead of spec's shadow-md; missing 1px solid border

File: client/src/pages/VendorsPage/VendorsPage.module.css

The visual spec called for shadow-md on the filter card and 1px solid var(--color-border) outline. The implementation uses shadow-sm with no border. Similarly, tableContainer uses shadow-sm rather than the spec's shadow-md, and has no explicit border. Both are non-blocking — shadow-sm is the established card shadow for most content cards in Cornerstone (consistent with BudgetCategoriesPage), and the spec may have over-specified here.

Non-blocking: Consider using shadow-sm as the canonical card shadow and update the Style Guide to clarify that shadow-md is reserved for elevated elements (dropdowns, popovers). No action needed on this PR.

---

Summary

Category	Status
Token adherence (zero hardcoded hex/px for design values)	PASS
Dark mode (all tokens covered by Layer 3 overrides)	PASS
Responsive layout (table/card swap at 767px, stacking)	PASS
Touch targets (min 44px on mobile/tablet)	PASS
Modal accessibility (role, aria-modal, aria-labelledby)	PASS
Focus indicators on buttons and inputs	PASS
Focus indicators on anchor links (.infoLink, .contactLink)	Needs refinement
Keyboard navigation on sortable headers	Needs refinement
Mobile card button aria-labels	Needs refinement
Back navigation element type (button vs Link)	Needs refinement

The visual implementation is approved from a UX/design-system perspective. All blocking concerns are absent. The four refinement items (observations 2, 5, 6, 7) should be captured in the post-epic refinement task.

[github-actions]

🎉 This PR is included in version 1.9.0-beta.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[steilerDev]

[ux-designer]

Visual/UX Review — PR #151: Story #143 Vendor Management

Overall the implementation is in strong shape: full design-token adherence, correct responsive breakpoints, proper modal semantics, and good dark-mode coverage via the inherited token system. The items below are split into blocking (must fix before merge for visual/UX quality) and non-blocking (tracked for the refinement pass).

Verdict: APPROVED with Non-Blocking Observations

All blocking visual requirements from the style spec posted on #143 are met. The non-blocking items should be captured for the refinement task.

---

Blocking Issues

None. All tokens are correctly referenced, dark mode works via semantic token inheritance, and the three-layer system is respected throughout.

---

Non-Blocking Observations

1. Kebab-menu pattern not implemented (spec deviation — acceptable)

The visual spec for VendorsPage specified the WorkItemsPage kebab-menu (⋮ dropdown) as the table-row action pattern. The implementation uses inline View and Delete buttons in an actionButtons flex container. This is a valid simplification for a two-action set and is actually cleaner. Refinement task: update the Style Guide to document the rule — use inline action buttons when there are ≤2 actions; use the kebab-menu pattern for 3 or more.

---

2. backLink implemented as <button> rather than <Link>/<a>

File: client/src/pages/VendorDetailPage/VendorDetailPage.tsx

The spec specified an <a> or React Router <Link> to /budget/vendors. The implementation uses <button onClick={() => navigate(...)}>. Navigation controls that move to a URL should be anchor elements so the browser supports right-click → Open in New Tab, Cmd+Click, and correct URL hover preview. The button also uses a non-standard focus style (outline: 2px solid var(--color-primary)) instead of the design system convention (box-shadow: var(--shadow-focus)).

Recommended fix (refinement):

• Replace <button className={styles.backLink}> with <Link to="/budget/vendors" className={styles.backLink}>
• Update .backLink:focus-visible to outline: none; box-shadow: var(--shadow-focus)

---

3. Back-navigation arrow prefix missing

File: client/src/pages/VendorDetailPage/VendorDetailPage.tsx

The spec specified text "← Back to Vendors" with a left-arrow. The implementation renders a breadcrumb (Vendors / {vendor.name}) without an arrow. The breadcrumb pattern is richer and fine to keep — add the arrow prefix to the back-link text for directional clarity, e.g. ← Vendors.

---

4. VendorDetailPage .pageTitle uses font-size-4xl instead of spec's font-size-3xl

File: client/src/pages/VendorDetailPage/VendorDetailPage.module.css

The spec said var(--font-size-3xl) (1.875rem) for the detail page h1, slightly smaller than the list page h1, to establish visual hierarchy. The implementation uses var(--font-size-4xl) (2rem) — same size as the list page. Non-blocking: refinement task should set .pageTitle in VendorDetailPage to var(--font-size-3xl).

---

5. infoLink and contactLink missing :focus-visible styles

Files:

• client/src/pages/VendorDetailPage/VendorDetailPage.module.css — .infoLink (phone/email anchors)
• client/src/pages/VendorsPage/VendorsPage.module.css — .contactLink (phone/email anchors)

Both have :hover rules but no :focus-visible rule. Keyboard users tabbing through phone/email links will get the browser default focus outline, which does not match the design system pattern. Note: .vendorLink:focus-visible is correctly specified — the same pattern should apply to .infoLink and .contactLink.

Recommended fix (refinement):

.infoLink:focus-visible,
.contactLink:focus-visible {
  outline: 2px solid var(--color-primary);
  outline-offset: 2px;
  border-radius: var(--radius-sm);
}

---

6. Mobile card action buttons missing aria-label

File: client/src/pages/VendorsPage/VendorsPage.tsx

The desktop table action buttons correctly use aria-label={View ${vendor.name}} and aria-label={Delete ${vendor.name}}. The mobile card buttons ("View Details" / "Delete") have no aria-label, making them ambiguous for screen reader users who hear multiple cards.

Recommended fix (refinement):

<button aria-label={`View ${vendor.name} details`}>View Details</button>
<button aria-label={`Delete ${vendor.name}`}>Delete</button>

---

7. Sortable table headers not keyboard-focusable

File: client/src/pages/VendorsPage/VendorsPage.tsx

The sortable <th> elements have onClick and aria-sort (correct) but lack tabIndex={0} and onKeyDown handlers for Enter/Space. Keyboard users cannot trigger column sorting. The sortableHeader CSS also needs a :focus-visible rule.

Recommended fix (refinement — frontend-developer): Add tabIndex={0}, role="button", and onKeyDown (Enter/Space → handleSortChange) to each sortable <th>. Add .sortableHeader:focus-visible { outline: none; box-shadow: var(--shadow-focus); } to the CSS.

---

8. Secondary/cancel button hover uses --color-border — semantic mismatch

Files: Both module CSS files

The hover background for secondary-style buttons is var(--color-border). Border tokens should not be used as surface fill colors. In dark mode --color-border resolves to --color-slate-500 (#334155), which against --color-text-secondary (#cbd5e1) passes WCAG AA but is darker than intended. The editButton:hover in VendorDetailPage correctly uses var(--color-bg-secondary) — that pattern should be standardised across all secondary/cancel button hover states.

Recommended fix (refinement): Replace var(--color-border) with var(--color-bg-secondary) on .secondaryButton:hover and .cancelButton:hover in both files.

---

9. Create-vendor modal and delete-confirm modal share one max-width: 36rem

File: client/src/pages/VendorsPage/VendorsPage.module.css

The single .modalContent class (36rem) fits the create form but over-sizes the delete confirmation. Non-blocking for this story. Refinement option: add a .modalContentCompact modifier at max-width: 28rem for destructive-action confirmations.

---

10. Hardcoded letter-spacing and line-height values

Both files have letter-spacing: 0.05em on table headers and stat labels, and line-height: 1.5/1.6 on textareas and note fields. These are consistent with existing pages in the codebase. A design system backlog item (add --letter-spacing-wide and --line-height-relaxed tokens) already covers this — no action needed on this PR.

---

Summary

Check	Status
Token adherence (no hardcoded hex/design values)	PASS
Dark mode (semantic token inheritance)	PASS
Responsive layout (table/card swap, stacking)	PASS
Touch targets (min 44px on mobile/tablet)	PASS
Modal accessibility (role, aria-modal, aria-labelledby, focus-trap)	PASS
Button/input focus-visible indicators	PASS
Anchor link focus-visible (infoLink, contactLink)	Refinement
Keyboard navigation on sortable headers	Refinement
Mobile card button aria-labels	Refinement
Back nav element type (button vs Link)	Refinement