From e0a4d69439e4bcec36400e628fdff0f3902bc5fd Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 00:32:53 +0800 Subject: [PATCH 01/35] fix(publish): decouple report delivery from the order lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Publishing a report required inspections.status === 'completed', and submitting one for review required the same. Nothing in the product ever wrote that value from the UI, so every publish attempt failed with "Inspection must be completed before publishing the report." while the publish modal reported the report itself as 100% ready — the error named a gate the user had no way to open. The two axes are independent: inspections.status tracks the order (scheduling, dispatch, metrics) and report_status tracks report production. Neither implies the other, so remove the coupling from both paths. Report content completeness is still gated, separately, by publishReadiness — that check is about the report and stays. The two tests that locked the old behaviour now assert the new one: publishing works from every order state and leaves status untouched. --- .../inspection/inspection-publish.service.ts | 4 +- .../inspection/inspection-status.service.ts | 8 ++-- .../reports/report-review-workflow.spec.ts | 46 ++++++++++++++----- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/server/services/inspection/inspection-publish.service.ts b/server/services/inspection/inspection-publish.service.ts index f7902c39..759a1621 100644 --- a/server/services/inspection/inspection-publish.service.ts +++ b/server/services/inspection/inspection-publish.service.ts @@ -5,7 +5,6 @@ import { safeISODate } from '../../lib/date'; import { resolveLocale } from '../../lib/locale'; import { InvoiceService } from '../invoice.service'; import { PeopleService } from '../people.service'; -import { INSPECTION_STATUS } from '../../lib/status/inspection-status'; import { REPORT_STATUS } from '../../lib/status/report-status'; import type { AgreementService } from '../agreement.service'; import type { TemplateSchemaV2 } from '../../types/template-schema'; @@ -408,7 +407,8 @@ export class InspectionPublishService extends InspectionSubService { .where(and(eq(inspections.id, inspectionId), eq(inspections.tenantId, tenantId))) .get(); if (!inspection) throw Errors.NotFound('Inspection not found'); - if (inspection.status !== INSPECTION_STATUS.COMPLETED) throw Errors.BadRequest('Inspection must be completed before publishing the report.'); + // The order lifecycle does not gate delivery — a report can ship while + // the order is still scheduled. Content completeness: publishReadiness. await db.update(inspections) .set({ reportStatus: REPORT_STATUS.PUBLISHED }) diff --git a/server/services/inspection/inspection-status.service.ts b/server/services/inspection/inspection-status.service.ts index d3936dc9..416b65c9 100644 --- a/server/services/inspection/inspection-status.service.ts +++ b/server/services/inspection/inspection-status.service.ts @@ -54,14 +54,14 @@ export class InspectionStatusService extends InspectionSubService { } /** - * Submits a completed inspection's report for manager review. + * Submits an inspection's report for manager review. * Transitions: reportStatus in_progress → submitted. */ async submitReport(inspectionId: string, tenantId: string): Promise { const { db, inspection } = await this.fetchForStatusChange(tenantId, inspectionId); - if (inspection.status !== INSPECTION_STATUS.COMPLETED) { - throw Errors.BadRequest('Inspection must be completed before submitting the report.'); - } + // Sending a report up for review is a report axis move; it carries no + // claim about whether the on-site work is wrapped up. Keeping the two + // axes coupled here would also make the two submit paths disagree. const reportStatus = inspection.reportStatus as string; if (reportStatus !== REPORT_STATUS.IN_PROGRESS) { throw Errors.BadRequest(`Cannot submit a report in status ${reportStatus}.`); diff --git a/tests/unit/reports/report-review-workflow.spec.ts b/tests/unit/reports/report-review-workflow.spec.ts index 77885f50..87650fa7 100644 --- a/tests/unit/reports/report-review-workflow.spec.ts +++ b/tests/unit/reports/report-review-workflow.spec.ts @@ -76,17 +76,34 @@ describe('Report review workflow (submit / publish / return / unpublish)', () => expect(row.status).toBe(INSPECTION_STATUS.COMPLETED); }); - it('2. publish throws when inspection status !== completed (e.g. scheduled)', async () => { - await testDb.insert(schema.inspections).values([ - makeInspection({ id: 'insp-pub-2', status: INSPECTION_STATUS.SCHEDULED }), - ]); - - await expect( - svc.publishInspection('insp-pub-2', TENANT, { + // The order lifecycle axis (inspections.status) and the report axis + // (inspections.report_status) advance independently: a report can ship + // while the order is still scheduled or confirmed. Publishing used to + // require status=completed, which made the order axis a hidden gate on + // report delivery. + it('2. publish works from every order lifecycle state, leaving status untouched', async () => { + const states = [ + INSPECTION_STATUS.REQUESTED, + INSPECTION_STATUS.SCHEDULED, + INSPECTION_STATUS.CONFIRMED, + INSPECTION_STATUS.COMPLETED, + ] as const; + + for (const status of states) { + const id = `insp-pub-2-${status}`; + await testDb.insert(schema.inspections).values([ + makeInspection({ id, status, reportStatus: REPORT_STATUS.IN_PROGRESS }), + ]); + + await svc.publishInspection(id, TENANT, { theme: 'default', notifyClient: false, notifyAgent: false, requireSignature: false, requirePayment: false, - }) - ).rejects.toThrow(/must be completed/i); + }); + + const row = await readStatuses(testDb, id); + expect(row.reportStatus).toBe(REPORT_STATUS.PUBLISHED); + expect(row.status).toBe(status); + } }); it('6. re-publish an already-published completed inspection stays published', async () => { @@ -119,13 +136,18 @@ describe('Report review workflow (submit / publish / return / unpublish)', () => expect(row.status).toBe(INSPECTION_STATUS.COMPLETED); }); - it('submitReport throws when inspection status !== completed (e.g. scheduled)', async () => { + // Same decoupling as publish: sending a report up for review is a report + // axis move and says nothing about whether the on-site work is wrapped up. + it('submitReport works while the order is still scheduled', async () => { await testDb.insert(schema.inspections).values([ makeInspection({ id: 'insp-sub-2', status: INSPECTION_STATUS.SCHEDULED }), ]); - await expect(svc.submitReport('insp-sub-2', TENANT)) - .rejects.toThrow(/must be completed/i); + await svc.submitReport('insp-sub-2', TENANT); + + const row = await readStatuses(testDb, 'insp-sub-2'); + expect(row.reportStatus).toBe(REPORT_STATUS.SUBMITTED); + expect(row.status).toBe(INSPECTION_STATUS.SCHEDULED); }); it('submitReport throws when reportStatus is not in_progress (e.g. already submitted)', async () => { From 48de222fa6c8a61c89bc053ec4de5168be3d9ef5 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 01:17:53 +0800 Subject: [PATCH 02/35] fix(hub): stop hiding the Report card behind the order lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reportActions() opened with `if (inspectionStatus !== COMPLETED) return []`, so the hub rendered no report actions at all until an inspection was marked completed — which no surface in the product could do. Every real inspection landed in the else branch, a single line of text ("Report is still in progress.") that named no blocker and offered no way forward. The rule it enforced is gone from the API, so it goes from here too, along with the now meaningless `inspectionStatus` parameter. canPublish() encoded the same rule, was covered by assertions in two test files, and had zero production callers — a gate that was built, tested, and never installed. Narrow it to the report axis and make isReportShipped its exact complement, so the predicate has one implementation and that implementation is the one the page actually renders from. The remaining branch no longer needs the "any actions?" test to decide whether to render: the status line always applies, and the action row is what varies by role. Splitting them also fixes the case that made the old else branch reachable — a reviewer without publish rights looking at a submitted report was told it was "still in progress". Also folds together the duplicates this touched: reportShipped/reportPublished were two names for one derivation, the blocking-count condition was spelled out three times, and canPublish/isReportShipped were asserted in both hub-blocks.test.ts and inspection-hub.test.ts. --- app/lib/hub-blocks.test.ts | 33 +++++++------- app/lib/hub-blocks.ts | 19 +++++--- app/lib/inspection-hub.test.ts | 55 +---------------------- app/routes/inspection-hub-actions.test.ts | 33 +++++++------- app/routes/inspection-hub.tsx | 50 ++++++++++----------- messages/en/inspections.json | 1 - 6 files changed, 71 insertions(+), 120 deletions(-) diff --git a/app/lib/hub-blocks.test.ts b/app/lib/hub-blocks.test.ts index daa9e6e5..51da6752 100644 --- a/app/lib/hub-blocks.test.ts +++ b/app/lib/hub-blocks.test.ts @@ -31,23 +31,20 @@ describe('isReportShipped', () => { }); }); -describe('canPublish', () => { - it('completed + in_progress → true', () => { - expect(canPublish(hub({ inspection: { status: 'completed', reportStatus: 'in_progress' } }))).toBe(true); - }); - it('completed + submitted → true (can still publish bypassing review)', () => { - expect(canPublish(hub({ inspection: { status: 'completed', reportStatus: 'submitted' } }))).toBe(true); - }); - it('completed + published → false (already published)', () => { - expect(canPublish(hub({ inspection: { status: 'completed', reportStatus: 'published' } }))).toBe(false); - }); - it('requested + in_progress → false (not completed)', () => { - expect(canPublish(hub({ inspection: { status: 'requested', reportStatus: 'in_progress' } }))).toBe(false); - }); - it('cancelled + in_progress → false (cancelled)', () => { - expect(canPublish(hub({ inspection: { status: 'cancelled', reportStatus: 'in_progress' } }))).toBe(false); - }); - it('scheduled + in_progress → false (not completed)', () => { - expect(canPublish(hub({ inspection: { status: 'scheduled', reportStatus: 'in_progress' } }))).toBe(false); +// canPublish reads the report axis and nothing else. The order lifecycle +// (requested → … → completed) tracks the job, not the report, and the server +// stopped gating publication on it — a UI gate here would only re-create the +// mismatch where the hub hides an action the API would have accepted. +describe('canPublish — report axis only', () => { + it('allows publishing from every point in the order lifecycle', () => { + for (const status of ['requested', 'scheduled', 'confirmed', 'completed', 'cancelled'] as const) { + expect(canPublish(hub({ inspection: { status, reportStatus: 'in_progress' } }))).toBe(true); + } + }); + it('allows publishing a submitted report, bypassing review', () => { + expect(canPublish(hub({ inspection: { status: 'confirmed', reportStatus: 'submitted' } }))).toBe(true); + }); + it('does not offer publishing for an already published report', () => { + expect(canPublish(hub({ inspection: { status: 'confirmed', reportStatus: 'published' } }))).toBe(false); }); }); diff --git a/app/lib/hub-blocks.ts b/app/lib/hub-blocks.ts index 56251c18..f509e3a8 100644 --- a/app/lib/hub-blocks.ts +++ b/app/lib/hub-blocks.ts @@ -11,7 +11,7 @@ * en-US/USD (behavior-preserving) and callers thread the viewer values when known. */ -import { INSPECTION_STATUS, isReportPublished } from '~/lib/status'; +import { isReportPublished } from '~/lib/status'; import { formatCurrency } from '~/lib/format'; import { m } from '~/paraglide/messages'; @@ -168,17 +168,22 @@ export function deriveBlockStates(hub: HubPayload): BlockStates { /** * Whether the hub Report card should offer an active "Publish report" button. - * True only when the inspection is `completed` AND the report is not already - * published. The `completed` gate excludes cancelled / requested / in-progress - * regardless of report status. + * Reads the report axis only: anything not yet published can be published. + * The order lifecycle (requested → … → completed) tracks the job rather than + * the report and the API does not gate publication on it, so gating here would + * only hide an action the server would have accepted. */ export function canPublish(hub: HubPayload): boolean { - return hub.inspection.status === INSPECTION_STATUS.COMPLETED && !isReportPublished(hub.inspection.reportStatus); + return !isReportPublished(hub.inspection.reportStatus); } -/** Whether the report has already been shipped to the client (read-only state). */ +/** + * Whether the report has already been shipped to the client (read-only state). + * The exact complement of `canPublish` — stated as such so the two can never + * drift into disagreeing about the same report. + */ export function isReportShipped(hub: HubPayload): boolean { - return isReportPublished(hub.inspection.reportStatus); + return !canPublish(hub); } /* ------------------------------------------------------------------ */ diff --git a/app/lib/inspection-hub.test.ts b/app/lib/inspection-hub.test.ts index 9dcc89ca..bc450f4a 100644 --- a/app/lib/inspection-hub.test.ts +++ b/app/lib/inspection-hub.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { deriveBlockStates, formatCents, canPublish, isReportShipped, type HubPayload } from '~/lib/hub-blocks'; +// canPublish / isReportShipped are covered in hub-blocks.test.ts — one home per rule. +import { deriveBlockStates, formatCents, type HubPayload } from '~/lib/hub-blocks'; /** * Issue #111 — pure block-state derivation for the `/inspections/:id` hub page. @@ -150,58 +151,6 @@ describe('deriveBlockStates — report block (reportStatus axis)', () => { }); }); -describe('canPublish — report publish affordance', () => { - it('completed + in_progress → true (active publish CTA)', () => { - expect(canPublish(hub({ - inspection: { status: 'completed', reportStatus: 'in_progress' }, - }))).toBe(true); - }); - - it('completed + submitted → true (can still publish bypassing review)', () => { - expect(canPublish(hub({ - inspection: { status: 'completed', reportStatus: 'submitted' }, - }))).toBe(true); - }); - - it('completed + published → false (already published)', () => { - expect(canPublish(hub({ - inspection: { status: 'completed', reportStatus: 'published' }, - }))).toBe(false); - }); - - it('requested + in_progress → false (status gate: not completed yet)', () => { - expect(canPublish(hub({ - inspection: { status: 'requested', reportStatus: 'in_progress' }, - }))).toBe(false); - }); - - it('cancelled + in_progress → false (status gate excludes cancelled)', () => { - expect(canPublish(hub({ - inspection: { status: 'cancelled', reportStatus: 'in_progress' }, - }))).toBe(false); - }); - - it('scheduled + in_progress → false (not completed)', () => { - expect(canPublish(hub({ - inspection: { status: 'scheduled', reportStatus: 'in_progress' }, - }))).toBe(false); - }); -}); - -describe('isReportShipped', () => { - it('published reportStatus → true', () => { - expect(isReportShipped(hub({ inspection: { reportStatus: 'published' } }))).toBe(true); - }); - - it('in_progress reportStatus → false', () => { - expect(isReportShipped(hub({ inspection: { reportStatus: 'in_progress' } }))).toBe(false); - }); - - it('submitted reportStatus → false', () => { - expect(isReportShipped(hub({ inspection: { reportStatus: 'submitted' } }))).toBe(false); - }); -}); - describe('formatCents', () => { it('formats whole dollars with the currency symbol', () => { expect(formatCents(50000)).toBe('$500.00'); diff --git a/app/routes/inspection-hub-actions.test.ts b/app/routes/inspection-hub-actions.test.ts index 22678856..bac31233 100644 --- a/app/routes/inspection-hub-actions.test.ts +++ b/app/routes/inspection-hub-actions.test.ts @@ -5,33 +5,34 @@ const publishCaps = { publish: true }; const noCaps = { publish: false }; describe('reportActions', () => { - it('non-completed → []', () => { - expect(reportActions(publishCaps, 'in_progress', 'requested')).toEqual([]); - expect(reportActions(publishCaps, 'in_progress', 'scheduled')).toEqual([]); - expect(reportActions(publishCaps, 'in_progress', 'cancelled')).toEqual([]); + // The order lifecycle no longer gates report actions: the hub used to render + // nothing at all until the inspection was marked completed, which no surface + // could do, so the Report card was a dead end for every real inspection. + it('offers publish from every point in the order lifecycle', () => { + expect(reportActions(publishCaps, 'in_progress')).toEqual(['publish']); }); - it('completed + published + publish cap → unpublish', () => { - expect(reportActions(publishCaps, 'published', 'completed')).toEqual(['unpublish']); + it('published + publish cap → unpublish', () => { + expect(reportActions(publishCaps, 'published')).toEqual(['unpublish']); }); - it('completed + published + no cap → []', () => { - expect(reportActions(noCaps, 'published', 'completed')).toEqual([]); + it('published + no cap → []', () => { + expect(reportActions(noCaps, 'published')).toEqual([]); }); - it('completed + submitted + publish cap → publish, return', () => { - expect(reportActions(publishCaps, 'submitted', 'completed')).toEqual(['publish', 'return']); + it('submitted + publish cap → publish, return', () => { + expect(reportActions(publishCaps, 'submitted')).toEqual(['publish', 'return']); }); - it('completed + submitted + no cap → []', () => { - expect(reportActions(noCaps, 'submitted', 'completed')).toEqual([]); + it('submitted + no cap → []', () => { + expect(reportActions(noCaps, 'submitted')).toEqual([]); }); - it('completed + in_progress + publish cap → publish', () => { - expect(reportActions(publishCaps, 'in_progress', 'completed')).toEqual(['publish']); + it('in_progress + publish cap → publish', () => { + expect(reportActions(publishCaps, 'in_progress')).toEqual(['publish']); }); - it('completed + in_progress + no cap → submit', () => { - expect(reportActions(noCaps, 'in_progress', 'completed')).toEqual(['submit']); + it('in_progress + no cap → submit', () => { + expect(reportActions(noCaps, 'in_progress')).toEqual(['submit']); }); }); diff --git a/app/routes/inspection-hub.tsx b/app/routes/inspection-hub.tsx index 8bee7495..f5fcc30e 100644 --- a/app/routes/inspection-hub.tsx +++ b/app/routes/inspection-hub.tsx @@ -6,7 +6,7 @@ import { createApi } from "~/lib/api-client.server"; import { formatInspectionDateTime } from "~/lib/format-date"; import { useDisplayTimeZone } from "~/hooks/useSessionContext"; import { deriveBlockStates, formatCents, isReportShipped, type HubPayload } from "~/lib/hub-blocks"; -import { INSPECTION_STATUS, REPORT_STATUS, isReportPublished, humanizeStatus, statusTone } from "~/lib/status"; +import { REPORT_STATUS, isReportPublished, humanizeStatus, statusTone } from "~/lib/status"; import { getEffectivePriceCents } from "~/lib/effective-price"; import { Breadcrumb } from "~/components/Breadcrumb"; import { PageHeader, Card, Pill, Button, EmptyState } from "@core/shared-ui"; @@ -333,20 +333,18 @@ export async function action({ request, params, context }: Route.ActionArgs) { /* ------------------------------------------------------------------ */ /** - * Derive which report action buttons to render given the current user's - * capabilities, the report status, and the inspection lifecycle status. - * Returns an ordered array of action identifiers for the Report card. + * Report action buttons for the current user's capabilities and report status. + * The order lifecycle is deliberately not an input: it tracks the job, not the + * report, and gating on it here left the card empty for every inspection that + * was never marked completed. Returns an ordered array of action identifiers. */ export function reportActions( caps: { publish: boolean }, reportStatus: string, - inspectionStatus: string, ): Array<'submit' | 'publish' | 'return' | 'unpublish'> { - if (inspectionStatus !== INSPECTION_STATUS.COMPLETED) return []; if (reportStatus === REPORT_STATUS.PUBLISHED) return caps.publish ? ['unpublish'] : []; if (reportStatus === REPORT_STATUS.SUBMITTED) return caps.publish ? ['publish', 'return'] : []; - // in_progress (or unknown) - return caps.publish ? ['publish'] : ['submit']; + return caps.publish ? ['publish'] : ['submit']; // in_progress (or unknown) } /* ------------------------------------------------------------------ */ @@ -471,18 +469,19 @@ export default function InspectionHubPage() { } }, [createReinspection.state, createReinspection.data, navigate]); - // "View report" only makes sense once the report is shipped to the client. - const reportShipped = isReportPublished(inspection.reportStatus); + // Shipped-to-client: gates the header "View report" link and the card body. + const reportShipped = isReportShipped(hub); - // Report card affordance: active publish CTA vs read-only-shipped. - const reportPublished = isReportShipped(hub); + // `publish` is the user's permission; the report's own eligibility lives in + // canPublish, which reportShipped above reads. + const reportActionList = reportActions({ publish: canPublishCap }, inspection.reportStatus); - // Report action matrix — what buttons to show in the Report card. - const reportActionList = reportActions( - { publish: canPublishCap }, - inspection.reportStatus, - inspection.status, - ); + // Incomplete content: drives the count line, the "resolve" link, and whether + // the action row has anything to hold. + const reportBlockersPending = + inspection.reportStatus === REPORT_STATUS.IN_PROGRESS && + !hub.publishReadiness.ready && + hub.publishReadiness.blockingCount > 0; const servicesTotalCents = services.reduce((sum, s) => sum + s.priceCents, 0); @@ -674,7 +673,7 @@ export default function InspectionHubPage() { {/* 6. Report ------------------------------------------------ */} - {reportPublished ? ( + {reportShipped ? ( // Already shipped — read-only for publishing. The header "View report" // link covers viewing. #119: a published baseline can spawn a // re-inspection that carries forward its still-open flagged items. @@ -713,7 +712,9 @@ export default function InspectionHubPage() { )} - ) : reportActionList.length > 0 ? ( + ) : ( + // Not shipped yet: the status line always applies, the action row + // only for roles that have an action to take. <> {inspection.reportStatus === 'submitted' && (

@@ -725,11 +726,12 @@ export default function InspectionHubPage() { {m.inspections_hub_report_ready()}

)} - {inspection.reportStatus === 'in_progress' && !hub.publishReadiness.ready && hub.publishReadiness.blockingCount > 0 && ( + {reportBlockersPending && (

{m.inspections_hub_report_blockers({ count: hub.publishReadiness.blockingCount })}

)} + {(reportActionList.length > 0 || reportBlockersPending) && (
{reportActionList.includes('publish') && (
+ )} - ) : ( - // Pre-completion (in progress) — nothing to publish yet. -

{m.inspections_hub_report_in_progress()}

)} diff --git a/messages/en/inspections.json b/messages/en/inspections.json index cef9a51b..4b8d5328 100644 --- a/messages/en/inspections.json +++ b/messages/en/inspections.json @@ -103,7 +103,6 @@ "inspections_hub_report_returning": "Returning…", "inspections_hub_report_return": "Return to inspector", "inspections_hub_report_resolve": "Resolve in editor", - "inspections_hub_report_in_progress": "Report is still in progress.", "inspections_hub_doc_upload_failed": "Upload failed. Please try again.", "inspections_hub_doc_delete_failed": "Could not delete the document. Please try again.", "inspections_hub_sms_granted": "granted", From 6faf092baed04db2ad4d4d1ab9da2a4ef5d3669b Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 01:41:35 +0800 Subject: [PATCH 03/35] fix(complete): stop firing report.published from order completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once publishing was decoupled from order completion, the /complete handler firing report.published became a defect rather than the delivery path: an inspector who marks fieldwork complete before publishing (now possible, and common) would trigger report delivery for a report that was never published — clients would receive a link to an unpublished report. report.published is a report event; it now fires only from publish. /complete is a pure order-lifecycle action plus its in-app admin notification. Also clears the ghost 'delivered' inspection status (IA-74). It is not a member of INSPECTION_STATUSES, so every `status === 'delivered'` comparison was dead: - the idempotency short-circuit's `|| status === 'delivered'` half never matched — now `=== COMPLETED` via the constant (IA-75's first bare-literal cleanup); - getInspectionStatusIcons keyed its `sent` icon solely off 'delivered', so the icon could never light. The helper had zero production callers and its test locked in impossible states (`{status:'delivered'} → sent:true`), so both are deleted; - inspection-core.service.ts cast status to `'draft'|'completed'|'delivered'`, a union disjoint from the real enum — now cast to InspectionStatus. The audit's IA-74 claim of a single occurrence undercounted: the ghost value had spread to two more sites, exactly as that item predicted. --- server/api/inspections/publish.ts | 35 +++------ server/lib/inspection-status.ts | 55 ------------- .../inspection/inspection-core.service.ts | 4 +- .../complete-route-primary-client.spec.ts | 68 +++++----------- .../inspections/inspection-status.spec.ts | 78 ------------------- 5 files changed, 29 insertions(+), 211 deletions(-) delete mode 100644 server/lib/inspection-status.ts delete mode 100644 tests/unit/inspections/inspection-status.spec.ts diff --git a/server/api/inspections/publish.ts b/server/api/inspections/publish.ts index 0102ee83..03173dda 100644 --- a/server/api/inspections/publish.ts +++ b/server/api/inspections/publish.ts @@ -18,6 +18,7 @@ import { createApiResponseSchema, SuccessResponseSchema } from '../../lib/valida import { PublishInspectionSchema, CreateReinspectionSchema, CancelInspectionSchema } from '../../lib/validations/inspection.schema'; import { drizzle } from 'drizzle-orm/d1'; import { inspections as inspectionTable } from '../../lib/db/schema'; +import { INSPECTION_STATUS } from '../../lib/status/inspection-status'; import { eq, and } from 'drizzle-orm'; import { getTenantId } from '../../lib/route-helpers'; import { withMcpMetadata } from '../../lib/route-metadata-standards'; @@ -286,42 +287,24 @@ const publishRoutes = createApiRouter() const service = c.var.services.inspection; const { inspection } = await service.getInspection(id, tenantId); - // Idempotency: if already completed, short-circuit to prevent accidental - // email storms when the client retries on network errors or double-clicks. - if (inspection.status === 'completed' || inspection.status === 'delivered') { + // Idempotency: if already completed, short-circuit so a retry on a + // network error or double-click does not re-run the admin notification. + if (inspection.status === INSPECTION_STATUS.COMPLETED) { return c.json({ success: true }, 200); } const db = drizzle(c.env.DB); - await db.update(inspectionTable).set({ status: 'completed' }).where(and(eq(inspectionTable.id, id), eq(inspectionTable.tenantId, tenantId))); + await db.update(inspectionTable).set({ status: INSPECTION_STATUS.COMPLETED }).where(and(eq(inspectionTable.id, id), eq(inspectionTable.tenantId, tenantId))); // Task 9a (people-role-profiles) — resolve the recipient via the // inspection_people join (PeopleService) instead of the legacy // inspection.clientEmail/.clientName column, which is being dropped. - // Resolved here only for the admin notification's metadata below — the - // actual report delivery is the automation trigger fired next. + // Used for the admin notification's metadata below. Report delivery is + // NOT fired here: report.published is a report event, produced solely by + // the publish path. Firing it on order completion would deliver a report + // that may never have been published (see IA-30). const primaryClient = await c.var.services.people.getPrimaryClient(tenantId, id); - // Route report delivery through the single automation engine (report.published) - // instead of an inline per-route send. The seeded report.published rules - // (client active, buyer_agent active, listing_agent inactive) fan out one - // per-recipient automation_log; the cron flush renders the PDF once and sends - // each recipient their role-keyed tokenized link. Idempotent per inspection - // (see the auto:report.published: dedup key), so a later /publish that - // fires the same event does not double-send. - try { - await c.var.services.automation.trigger({ - tenantId, - inspectionId: id, - triggerEvent: 'report.published', - companyName: '', - reportBaseUrl: '', - }); - } catch (err) { - logger.error('[complete] report.published automation trigger failed', - { inspectionId: id }, err instanceof Error ? err : undefined); - } - // B3: in-app notification for report ready c.executionCtx.waitUntil( c.var.services.notification.createForAllAdmins(tenantId, { diff --git a/server/lib/inspection-status.ts b/server/lib/inspection-status.ts deleted file mode 100644 index 0e82a73b..00000000 --- a/server/lib/inspection-status.ts +++ /dev/null @@ -1,55 +0,0 @@ -/** - * Round-2 F2 — Inspection list status-icon helper. - * - * Translates the dashboard-bucket statusFlags shape into the four visual - * indicators rendered next to each inspection row: - * - * - 📄 report ready → status is `completed` or `delivered` - * - 📋 agreement signed → at least one signed agreement_requests envelope - * - ✈️ sent → status is `delivered` (publish workflow ran) - * - 🚩 flag → row is in the "Needs Attention" bucket - * (e.g. agreement unsigned past threshold, - * invoice overdue, report unpublished too long) - * - * Pure function: input is the minimal subset of inspection-row fields the - * dashboard already loads, output is a 4-key boolean record. Easy to unit - * test with a state matrix and reuse anywhere a row needs the icons. - */ - -export interface InspectionStatusInput { - /** Inspection lifecycle status (`draft`, `scheduled`, `confirmed`, `in_progress`, `completed`, `delivered`, `cancelled`). */ - status?: string | null; - /** Has at least one signed agreement record. */ - agreementSigned?: boolean; - /** Row currently sits in the Needs Attention bucket. */ - flagged?: boolean; -} - -export interface InspectionStatusIcons { - /** Report has been built (rated + published). */ - reportReady: boolean; - /** Inspection agreement was signed by the client. */ - agreementSigned: boolean; - /** Publish workflow completed — report has been delivered to recipients. */ - sent: boolean; - /** Row needs attention (overdue or stale). */ - flagged: boolean; -} - -/** - * Computes the four status-icon states for one inspection row. - * - * Defensive: missing fields collapse to `false` so partial data (e.g. a row - * pulled before signed-agreements join lands) never throws. - */ -export function getInspectionStatusIcons(input: InspectionStatusInput): InspectionStatusIcons { - const status = (input.status ?? '').toLowerCase(); - const completedOrDelivered = status === 'completed' || status === 'delivered'; - const delivered = status === 'delivered'; - return { - reportReady: completedOrDelivered, - agreementSigned: input.agreementSigned === true, - sent: delivered, - flagged: input.flagged === true, - }; -} diff --git a/server/services/inspection/inspection-core.service.ts b/server/services/inspection/inspection-core.service.ts index 30693c28..ad9a45f2 100644 --- a/server/services/inspection/inspection-core.service.ts +++ b/server/services/inspection/inspection-core.service.ts @@ -13,7 +13,7 @@ import { computePreflightFromData } from '../../lib/preflight'; import { syncInspectionAssignments } from '../../lib/db/assignment-links'; import { findingKey, DEFAULT_UNIT } from '../../lib/finding-key'; import { parseReinspectionStatuses, isOpenStatus } from '../../lib/reinspection-status'; -import { INSPECTION_STATUS } from '../../lib/status/inspection-status'; +import { INSPECTION_STATUS, type InspectionStatus } from '../../lib/status/inspection-status'; import { REPORT_STATUS } from '../../lib/status/report-status'; import { fireAutomation, type Inspection, type InspectionListParams, type CreateInspectionData } from './shared'; import { InspectionSubService } from './base'; @@ -300,7 +300,7 @@ export class InspectionCoreService extends InspectionSubService { clientName: primaryClient?.name ?? null, clientEmail: primaryClient?.email ?? null, clientPhone: primaryClient?.phone ?? null, - status: result.status as 'draft' | 'completed' | 'delivered', + status: result.status as InspectionStatus, date: result.date as string, inspectorId: result.inspectorId as string | null, templateId: result.templateId as string | null, diff --git a/tests/unit/inspections/complete-route-primary-client.spec.ts b/tests/unit/inspections/complete-route-primary-client.spec.ts index 678e8734..f8a30b03 100644 --- a/tests/unit/inspections/complete-route-primary-client.spec.ts +++ b/tests/unit/inspections/complete-route-primary-client.spec.ts @@ -7,12 +7,14 @@ * inspection_people populated, so it fails against the old implementation * (which reads only inspection.clientEmail and skips the send entirely). * - * Spec 2 Task 4 — /complete no longer sends the report inline. It fires the - * `report.published` automation trigger (the same engine path the live - * /publish route already uses), so this spec now spies on - * `c.var.services.automation.trigger` instead of asserting inline send-method - * call counts. The in-app admin notification (createForAllAdmins) is - * unchanged — still fired directly from the route, not by the engine. + * IA-30 Task 3 — /complete no longer fires the `report.published` automation + * trigger at all. Once report publishing was decoupled from order completion + * (Task 1/2), marking fieldwork complete could fire report delivery for a + * report that was never published — clients would receive a link to an + * unpublished report. `report.published` is a report event and now fires from + * publish only; /complete is a pure order-lifecycle action. This spec asserts + * the trigger is NOT called and no inline send happens; the in-app admin + * notification (createForAllAdmins) still fires directly from the route. */ import { describe, it, expect, beforeEach, vi } from 'vitest'; import * as schema from '../../../server/lib/db/schema'; @@ -95,7 +97,7 @@ function buildApp(inspectionStub: { status: string; clientEmail: string | null; const ENV = { DB: {}, APP_BASE_URL: 'https://acme.example.com', JWT_SECRET: 'test-secret' } as never; -describe('POST /api/inspections/:id/complete — primary-client resolution (Task 9a) + engine-routed delivery (Spec 2 Task 4)', () => { +describe('POST /api/inspections/:id/complete — primary-client resolution (Task 9a) + no report delivery (IA-30 Task 3)', () => { beforeEach(async () => { const fixture = createTestDb(); db = fixture.db; @@ -130,7 +132,7 @@ describe('POST /api/inspections/:id/complete — primary-client resolution (Task return req; } - it('fires report.published through the automation engine, not the inline send methods, for a primary client resolved via PeopleService', async () => { + it('does not fire report.published (delivery is publish\'s job) but still records the admin notification with the resolved primary-client email', async () => { const { ctx, settle } = makeExecCtx(); const app = buildApp({ status: 'in_progress', clientEmail: null, clientName: null, @@ -140,23 +142,15 @@ describe('POST /api/inspections/:id/complete — primary-client resolution (Task expect(res.status).toBe(200); await settle(); - // The engine trigger is fired (awaited inline, not via waitUntil) with - // the right tenant/inspection/event. - expect(automationTrigger).toHaveBeenCalledTimes(1); - expect(automationTrigger.mock.calls[0][0]).toMatchObject({ - tenantId: TENANT, - inspectionId: INSP_ID, - triggerEvent: 'report.published', - }); - - // The route itself no longer performs an inline send — delivery is the - // engine's job now (per-recipient PDF + role-keyed link, cron-flushed). + // report.published belongs to publish. Marking fieldwork complete must + // not deliver a report — it may not even be published yet. + expect(automationTrigger).not.toHaveBeenCalled(); expect(issueToken).not.toHaveBeenCalled(); expect(sendInspectionReportPdf).not.toHaveBeenCalled(); expect(sendReportReady).not.toHaveBeenCalled(); - // The in-app admin notification's metadata still carries the resolved - // primary-client email (unrelated to the automation engine). + // The in-app admin notification still fires, and its metadata carries the + // primary-client email resolved via PeopleService (Task 9a). expect(createForAllAdmins).toHaveBeenCalledTimes(1); expect(createForAllAdmins.mock.calls[0][1]).toMatchObject({ type: 'report.published', @@ -164,7 +158,7 @@ describe('POST /api/inspections/:id/complete — primary-client resolution (Task }); }); - it('no primary client at all — still fires the trigger (engine resolves zero recipients) and the notification carries clientEmail:null', async () => { + it('no primary client at all — no delivery, notification carries clientEmail:null', async () => { // Remove the seeded inspection_people row so getPrimaryClient resolves null. const { eq } = await import('drizzle-orm'); await db.delete(schema.inspectionPeople).where(eq(schema.inspectionPeople.inspectionId, INSP_ID)); @@ -178,16 +172,7 @@ describe('POST /api/inspections/:id/complete — primary-client resolution (Task expect(res.status).toBe(200); await settle(); - // report.published still fires unconditionally — the engine (not this - // route) is responsible for resolving recipients and no-op'ing when - // there are none. - expect(automationTrigger).toHaveBeenCalledTimes(1); - expect(automationTrigger.mock.calls[0][0]).toMatchObject({ - tenantId: TENANT, - inspectionId: INSP_ID, - triggerEvent: 'report.published', - }); - + expect(automationTrigger).not.toHaveBeenCalled(); expect(issueToken).not.toHaveBeenCalled(); expect(sendReportReady).not.toHaveBeenCalled(); expect(sendInspectionReportPdf).not.toHaveBeenCalled(); @@ -198,24 +183,7 @@ describe('POST /api/inspections/:id/complete — primary-client resolution (Task expect(createForAllAdmins.mock.calls[0][1]).toMatchObject({ metadata: { clientEmail: null } }); }); - it('a failed automation trigger does not 500 the completion (log + continue)', async () => { - const { ctx, settle } = makeExecCtx(); - const app = buildApp({ - status: 'in_progress', clientEmail: null, clientName: null, - propertyAddress: '1 Main St', inspectorId: null, id: INSP_ID, - }); - automationTrigger.mockRejectedValueOnce(new Error('enqueue failed')); - - const res = await app.fetch(post(), ENV, ctx); - expect(res.status).toBe(200); - await settle(); - - expect(automationTrigger).toHaveBeenCalledTimes(1); - // Completion + admin notification still happen even though the trigger failed. - expect(createForAllAdmins).toHaveBeenCalledTimes(1); - }); - - it('already-completed inspection short-circuits — no trigger, no notification', async () => { + it('already-completed inspection short-circuits — no notification', async () => { const { ctx, settle } = makeExecCtx(); const app = buildApp({ status: 'completed', clientEmail: null, clientName: null, diff --git a/tests/unit/inspections/inspection-status.spec.ts b/tests/unit/inspections/inspection-status.spec.ts deleted file mode 100644 index 4e6479ee..00000000 --- a/tests/unit/inspections/inspection-status.spec.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { getInspectionStatusIcons } from '../../../server/lib/inspection-status'; - -describe('Round-2 F2 — getInspectionStatusIcons', () => { - it('returns all-false for a draft inspection with no flags', () => { - const icons = getInspectionStatusIcons({ status: 'draft' }); - expect(icons).toEqual({ - reportReady: false, - agreementSigned: false, - sent: false, - flagged: false, - }); - }); - - it('marks reportReady when status is completed (not yet sent)', () => { - const icons = getInspectionStatusIcons({ status: 'completed' }); - expect(icons.reportReady).toBe(true); - expect(icons.sent).toBe(false); - }); - - it('marks both reportReady and sent when status is delivered', () => { - const icons = getInspectionStatusIcons({ status: 'delivered' }); - expect(icons.reportReady).toBe(true); - expect(icons.sent).toBe(true); - }); - - it('agreementSigned + flagged independent of status', () => { - const icons = getInspectionStatusIcons({ - status: 'in_progress', - agreementSigned: true, - flagged: true, - }); - expect(icons.agreementSigned).toBe(true); - expect(icons.flagged).toBe(true); - expect(icons.reportReady).toBe(false); - expect(icons.sent).toBe(false); - }); - - it('treats missing fields as false (defensive)', () => { - const icons = getInspectionStatusIcons({}); - expect(icons).toEqual({ - reportReady: false, - agreementSigned: false, - sent: false, - flagged: false, - }); - }); - - it('handles cancelled status — no green flags', () => { - const icons = getInspectionStatusIcons({ status: 'cancelled' }); - expect(icons.reportReady).toBe(false); - expect(icons.sent).toBe(false); - }); - - it('case-insensitive status matching', () => { - const icons = getInspectionStatusIcons({ status: 'DELIVERED' }); - expect(icons.reportReady).toBe(true); - expect(icons.sent).toBe(true); - }); - - it('matrix — every status combination', () => { - const cases: Array<[string, boolean, boolean]> = [ - // status, expected reportReady, expected sent - ['draft', false, false], - ['scheduled', false, false], - ['confirmed', false, false], - ['in_progress', false, false], - ['completed', true, false], - ['delivered', true, true ], - ['cancelled', false, false], - ]; - for (const [status, ready, sent] of cases) { - const icons = getInspectionStatusIcons({ status }); - expect(icons.reportReady, `${status} reportReady`).toBe(ready); - expect(icons.sent, `${status} sent`).toBe(sent); - } - }); -}); From 6814b11f3e8bb65ebb08b66e84fb7ac0214cde3c Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 02:46:30 +0800 Subject: [PATCH 04/35] feat: let inspectors mark fieldwork complete, from either surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The order lifecycle's `completed` value had no producer — every other order state was written by some flow (booking, scheduling, concierge, cancel), but nothing marked an inspection completed, so the axis could never advance past confirmed. This adds that producer as an advisory action on the two surfaces an inspector actually uses, never as a gate. Editor (the primary surface — where the inspector finishes on-site): - a "Finish fieldwork" toolbar button, shown until the work is marked complete, POSTing the complete intent; - clicking Publish while not completed opens the existing PublishModal with two paths — "Just publish" and "Mark complete and publish" — both of which publish. Advisory, never blocking, matching the scheduling-conflict philosophy (IA-6). The editor's inspection copy is frozen at mount, so both paths patch status locally (the setInspection pattern structural edits use) to keep the badge live without a reload. Hub: an "Inspection status" card showing the current state and a "Mark fieldwork complete" button while it is still open. Server: a `complete` intent on both the hub and editor actions; the editor's publish intent takes an optional markComplete that fires complete first — idempotent and non-blocking, so it never stops the publish that follows. The Finish-fieldwork button shows from lg up (one step more prominent than the xl-only Sign button, reflecting that it is a more central action). The three touched route files are already grandfathered in the file-size baseline; this bumps their caps for the genuine feature growth. --- app/components/editor/EditorHeader.tsx | 22 ++++++ app/components/editor/PublishModal.tsx | 30 +++++++- app/routes/inspection-edit.tsx | 37 +++++++++- .../inspection-edit/action.server.test.ts | 73 +++++++++++++++++++ app/routes/inspection-edit/action.server.ts | 14 ++++ app/routes/inspection-hub.tsx | 40 +++++++++- messages/en/editor-2.json | 6 ++ messages/en/inspections.json | 5 ++ scripts/file-size-baseline.json | 6 +- 9 files changed, 224 insertions(+), 9 deletions(-) create mode 100644 app/routes/inspection-edit/action.server.test.ts diff --git a/app/components/editor/EditorHeader.tsx b/app/components/editor/EditorHeader.tsx index 6c9a04e6..e520117e 100644 --- a/app/components/editor/EditorHeader.tsx +++ b/app/components/editor/EditorHeader.tsx @@ -18,6 +18,10 @@ export interface EditorHeaderProps { setSignModalOpen: (open: boolean) => void; /** Publish button click handler. */ handlePublishClick: () => void; + /** Marks the on-site work complete (advisory order-lifecycle move). */ + handleFinishFieldwork: () => void; + /** Whether the finish-fieldwork request is in flight. */ + finishingFieldwork: boolean; /** #181 — whether collab (and thus version history) is available. */ collabEditing?: boolean; /** Opens the version-history panel. */ @@ -44,6 +48,8 @@ export function EditorHeader({ tenantSlug, setSignModalOpen, handlePublishClick, + handleFinishFieldwork, + finishingFieldwork, collabEditing, onOpenVersionHistory, perUnitControls, @@ -296,6 +302,22 @@ export function EditorHeader({ {m.editor_header_sign()} + {/* Finish fieldwork — advisory order-lifecycle move, shown until the + on-site work is marked complete. Publishing does not require it. A more + central action than Sign (xl-only), so it appears from lg up. */} + {(state.inspection.status as string) !== "completed" && ( + + )} + {/* Publish button */} + + + + ) : ( <> + ) } >

@@ -45,6 +66,11 @@ export function PublishModal({ open, progress, status, publishError, isSubmittin

{m.editor_publish_stat_completion()}{progress.pct}%
{m.editor_publish_stat_status()}{status}
+ {notCompleted && ( +

+ {m.editor_publish_not_completed_prompt()} +

+ )} {publishError && (
{publishError} diff --git a/app/routes/inspection-edit.tsx b/app/routes/inspection-edit.tsx index e4880ef4..02aba4bf 100644 --- a/app/routes/inspection-edit.tsx +++ b/app/routes/inspection-edit.tsx @@ -1,5 +1,6 @@ import { useState, useCallback, useMemo, useEffect, useRef } from "react"; import { useLoaderData, useFetcher, useNavigate, useRevalidator } from "react-router"; +import { INSPECTION_STATUS } from "~/lib/status"; import { findRatingLevel, ratingAdvanceDecision } from "~/lib/rating-levels"; import { makeCustomDefect } from "~/lib/custom-defects"; import { useInspectionState, type InspectionSchema, type ItemFilter } from "~/hooks/useInspection"; @@ -159,6 +160,9 @@ export default function InspectionEditPage() { // message is surfaced inline in the publish modal instead. const publishFetcher = useFetcher<{ ok: boolean; intent?: string; error?: string }>(); const [publishError, setPublishError] = useState(null); + // Order-lifecycle "Finish fieldwork" — its own fetcher so marking the on-site + // work complete never contends with an in-flight autosave or publish. + const completeFetcher = useFetcher<{ ok: boolean; intent?: string }>(); // Commercial PCA Phase S — narrative editor panel. Own fetcher (mirrors the // notes/upload isolation reasoning above) so a per-block blur save cannot be // aborted by an unrelated in-flight mutation. Dispatches the "save-pca-narrative" @@ -668,6 +672,11 @@ export default function InspectionEditPage() { state.setShowPublishModal(true); }, [state.inspection.id, state.setShowPublishModal]); + /* Finish fieldwork — advisory order-lifecycle move (never a publish gate). */ + const handleFinishFieldwork = useCallback(() => { + completeFetcher.submit({ intent: "complete" }, { method: "post" }); + }, [completeFetcher]); + /* ---------------------------------------------------------------- */ /* Item attribute handler */ /* ---------------------------------------------------------------- */ @@ -885,6 +894,18 @@ export default function InspectionEditPage() { } }, [publishFetcher.state, publishFetcher.data, state.setShowPublishModal, revalidator]); + /* Finish-fieldwork result — the editor's inspection copy is frozen at mount + * (see useInspectionState), so revalidation alone leaves the header badge and + * the toolbar button stale. Patch status locally the same way structural edits + * do, then revalidate for anything loader-derived. */ + useEffect(() => { + if (completeFetcher.state !== "idle" || !completeFetcher.data) return; + if (completeFetcher.data.ok) { + state.setInspection((prev) => ({ ...prev, status: INSPECTION_STATUS.COMPLETED })); + revalidator.revalidate(); + } + }, [completeFetcher.state, completeFetcher.data, revalidator, state]); + /* ---------------------------------------------------------------- */ /* Rating handler with auto-advance */ /* ---------------------------------------------------------------- */ @@ -2015,11 +2036,19 @@ export default function InspectionEditPage() { publishError={publishError} isSubmitting={publishFetcher.state !== "idle"} onClose={() => { setPublishError(null); state.setShowPublishModal(false); }} - onPublish={() => { + onPublish={(markComplete: boolean) => { // Keep the modal open: the publish-result effect closes it on success - // and shows the real server reason inline on failure. + // and shows the real server reason inline on failure. `markComplete` + // also closes the order axis first (advisory — never blocks publish); + // patch status locally so the badge reflects it without a reload. setPublishError(null); - publishFetcher.submit({ intent: "publish" }, { method: "post" }); + if (markComplete) { + state.setInspection((prev) => ({ ...prev, status: INSPECTION_STATUS.COMPLETED })); + } + publishFetcher.submit( + markComplete ? { intent: "publish", markComplete: "true" } : { intent: "publish" }, + { method: "post" }, + ); }} autoSign={autoSign} onAutoSignToggle={handleAutoSignToggle} @@ -2129,6 +2158,8 @@ export default function InspectionEditPage() { tenantSlug={loaderData.tenantSlug} setSignModalOpen={setSignModalOpen} handlePublishClick={handlePublishClick} + handleFinishFieldwork={handleFinishFieldwork} + finishingFieldwork={completeFetcher.state !== "idle"} collabEditing={loaderData.collabEditing} onOpenVersionHistory={() => setVersionHistoryOpen(true)} onChangeTemplate={() => state.setSettingsOpen(true)} diff --git a/app/routes/inspection-edit/action.server.test.ts b/app/routes/inspection-edit/action.server.test.ts new file mode 100644 index 00000000..90f020a9 --- /dev/null +++ b/app/routes/inspection-edit/action.server.test.ts @@ -0,0 +1,73 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// The editor action mints an API client via createApi and reads the token via +// requireToken. Both are mocked so the test exercises only the action's own +// branching — which endpoints it calls, in what order, for a given intent. +const completePost = vi.fn(); +const publishPost = vi.fn(); + +vi.mock('~/lib/session.server', () => ({ + requireToken: vi.fn().mockResolvedValue('tok'), +})); +vi.mock('~/lib/api-client.server', () => ({ + createApi: vi.fn(() => ({ + inspections: { + ':id': { + complete: { $post: completePost }, + publish: { $post: publishPost }, + }, + }, + })), +})); + +import { action } from './action.server'; + +function post(fields: Record) { + const body = new URLSearchParams(fields); + const request = new Request('https://acme.example.com/inspections/insp-1/edit', { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: body.toString(), + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return action({ request, params: { id: 'insp-1' }, context: {} as any }); +} + +describe('editor action — order-lifecycle intents (IA-30 Task 4)', () => { + beforeEach(() => { + completePost.mockReset().mockResolvedValue(new Response(null, { status: 200 })); + publishPost.mockReset().mockResolvedValue(new Response(null, { status: 200 })); + }); + + it('intent=complete hits the complete endpoint only', async () => { + const res = await post({ intent: 'complete' }); + expect(completePost).toHaveBeenCalledTimes(1); + expect(completePost).toHaveBeenCalledWith({ param: { id: 'insp-1' } }); + expect(publishPost).not.toHaveBeenCalled(); + expect(res).toMatchObject({ ok: true, intent: 'complete' }); + }); + + it('intent=publish without markComplete publishes and never marks complete', async () => { + const res = await post({ intent: 'publish' }); + expect(publishPost).toHaveBeenCalledTimes(1); + expect(completePost).not.toHaveBeenCalled(); + expect(res).toMatchObject({ ok: true, intent: 'publish' }); + }); + + it('intent=publish with markComplete fires complete first, then publishes', async () => { + const order: string[] = []; + completePost.mockImplementation(() => { order.push('complete'); return Promise.resolve(new Response(null, { status: 200 })); }); + publishPost.mockImplementation(() => { order.push('publish'); return Promise.resolve(new Response(null, { status: 200 })); }); + + const res = await post({ intent: 'publish', markComplete: 'true' }); + expect(order).toEqual(['complete', 'publish']); + expect(res).toMatchObject({ ok: true, intent: 'publish' }); + }); + + it('a failing complete never blocks the publish it precedes (advisory)', async () => { + completePost.mockRejectedValue(new Error('complete boom')); + const res = await post({ intent: 'publish', markComplete: 'true' }); + expect(publishPost).toHaveBeenCalledTimes(1); + expect(res).toMatchObject({ ok: true, intent: 'publish' }); + }); +}); diff --git a/app/routes/inspection-edit/action.server.ts b/app/routes/inspection-edit/action.server.ts index 18dec810..037b5b5e 100644 --- a/app/routes/inspection-edit/action.server.ts +++ b/app/routes/inspection-edit/action.server.ts @@ -15,7 +15,21 @@ export async function action({ request, params, context }: Route.ActionArgs) { // data loss (the save pill said "Saved" either way). let ok = true; + // `complete` is not on the typed client (like submit/return/unpublish). + const completeEndpoint = () => (api.inspections[":id"] as unknown as { + complete: { $post: (args: { param: { id: string } }) => Promise }; + }).complete.$post({ param: { id: params.id } }); + + if (intent === "complete") { + // Advisory order-lifecycle move; decoupled from publishing, never a gate. + const res = await completeEndpoint(); + return { ok: res.ok, intent: "complete" as const }; + } + if (intent === "publish") { + // "Mark complete and publish": fire complete first — idempotent and + // non-blocking, so a failure there never stops the publish that follows. + if (formData.get("markComplete") === "true") await completeEndpoint().catch(() => undefined); const res = await api.inspections[":id"].publish.$post({ param: { id: params.id }, json: {} }); // Publish has meaningful precondition failures (e.g. "Inspection must be // completed before publishing the report.") that the inspector MUST see — diff --git a/app/routes/inspection-hub.tsx b/app/routes/inspection-hub.tsx index f5fcc30e..fc4f9aa4 100644 --- a/app/routes/inspection-hub.tsx +++ b/app/routes/inspection-hub.tsx @@ -6,7 +6,7 @@ import { createApi } from "~/lib/api-client.server"; import { formatInspectionDateTime } from "~/lib/format-date"; import { useDisplayTimeZone } from "~/hooks/useSessionContext"; import { deriveBlockStates, formatCents, isReportShipped, type HubPayload } from "~/lib/hub-blocks"; -import { REPORT_STATUS, isReportPublished, humanizeStatus, statusTone } from "~/lib/status"; +import { INSPECTION_STATUS, REPORT_STATUS, isReportPublished, humanizeStatus, statusTone } from "~/lib/status"; import { getEffectivePriceCents } from "~/lib/effective-price"; import { Breadcrumb } from "~/components/Breadcrumb"; import { PageHeader, Card, Pill, Button, EmptyState } from "@core/shared-ui"; @@ -266,6 +266,14 @@ export async function action({ request, params, context }: Route.ActionArgs) { return toActionResult(res, "unpublish", m.inspections_hub_error_unpublish()); } + if (intent === "complete") { + const completeApi = api.inspections[":id"] as unknown as { + complete: { $post: (args: { param: { id: string } }) => Promise }; + }; + const res = await completeApi.complete.$post({ param: { id } }); + return toActionResult(res, "complete", m.inspections_hub_lifecycle_error()); + } + if (intent === "create-reinspection") { // #119 Task 6 — carry the checked baseline items forward into a new // re-inspection. The form submits one `selectedItemIds` value per checked @@ -438,6 +446,8 @@ export default function InspectionHubPage() { const submitReport = useFetcher(); const returnReport = useFetcher(); const unpublishReport = useFetcher(); + const completeInspection = useFetcher(); + const markingComplete = completeInspection.state !== "idle"; const submittingReport = submitReport.state !== "idle"; const returningReport = returnReport.state !== "idle"; const unpublishingReport = unpublishReport.state !== "idle"; @@ -587,6 +597,34 @@ export default function InspectionHubPage() { + {/* 2b. Order lifecycle — independent of report publishing. "Mark + fieldwork complete" is the only producer of `completed`; advisory, + never a publish precondition. */} + + + {inspection.status !== INSPECTION_STATUS.COMPLETED && + inspection.status !== INSPECTION_STATUS.CANCELLED && ( + <> +

+ {m.inspections_hub_lifecycle_hint()} +

+ + + + + + )} + + {/* 3. Services ---------------------------------------------- */} diff --git a/messages/en/editor-2.json b/messages/en/editor-2.json index 786cc0fd..c3983ea6 100644 --- a/messages/en/editor-2.json +++ b/messages/en/editor-2.json @@ -187,6 +187,12 @@ "editor_publish_stat_completion": "Completion", "editor_publish_stat_status": "Status", "editor_publish_autosign": "Auto-sign this report on publish", + "editor_finish_fieldwork": "Finish fieldwork", + "editor_finish_fieldwork_pending": "Finishing…", + "editor_finish_fieldwork_error": "Could not mark fieldwork complete.", + "editor_publish_not_completed_prompt": "The on-site work is not marked complete. Publishing does not require it — you can do both, or just publish.", + "editor_publish_mark_complete_and_publish": "Mark complete and publish", + "editor_publish_just_publish": "Just publish", "editor_rating_aria": "Rating", "editor_recrop_title": "Re-crop this photo?", "editor_recrop_confirm": "Crop & clear", diff --git a/messages/en/inspections.json b/messages/en/inspections.json index 4b8d5328..7c7efedd 100644 --- a/messages/en/inspections.json +++ b/messages/en/inspections.json @@ -103,6 +103,11 @@ "inspections_hub_report_returning": "Returning…", "inspections_hub_report_return": "Return to inspector", "inspections_hub_report_resolve": "Resolve in editor", + "inspections_hub_lifecycle_title": "Inspection status", + "inspections_hub_lifecycle_mark_complete": "Mark fieldwork complete", + "inspections_hub_lifecycle_marking": "Marking…", + "inspections_hub_lifecycle_hint": "Marks the on-site work as finished. Publishing a report does not require it.", + "inspections_hub_lifecycle_error": "Could not mark the inspection complete.", "inspections_hub_doc_upload_failed": "Upload failed. Please try again.", "inspections_hub_doc_delete_failed": "Could not delete the document. Please try again.", "inspections_hub_sms_granted": "granted", diff --git a/scripts/file-size-baseline.json b/scripts/file-size-baseline.json index 48a5977e..4a55cdde 100644 --- a/scripts/file-size-baseline.json +++ b/scripts/file-size-baseline.json @@ -1,7 +1,7 @@ { - "app/routes/inspection-edit.tsx": 2443, + "app/routes/inspection-edit.tsx": 2474, "server/services/inspection/inspection-core.service.ts": 1083, - "app/routes/inspection-hub.tsx": 1010, + "app/routes/inspection-hub.tsx": 1048, "server/services/booking.service.ts": 967, "server/services/inspection/inspection-report.service.ts": 903, "server/durable-objects/inspection-doc.ts": 896, @@ -43,7 +43,7 @@ "server/services/inspection-request.service.ts": 501, "server/services/report-export-consumer.ts": 499, "server/api/repair-builder.ts": 496, - "app/routes/inspection-edit/action.server.ts": 482, + "app/routes/inspection-edit/action.server.ts": 496, "app/components/collab/VersionHistoryPanel.tsx": 478, "app/routes/settings-workspace.tsx": 477, "server/api/bookings.ts": 477, From 3518acc6198d1ef4b30d4c0012cc514e1feba5f6 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 03:09:18 +0800 Subject: [PATCH 05/35] test(e2e): cover publishing with and without the completion step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The publish chain had no end-to-end coverage — `rg "/complete" tests/` was empty — which is precisely how the "publish requires completed, but nothing can mark completed" deadlock reached v1.0.0. These specs pin the decoupling: publishing is offered while the order is still open, and marking fieldwork complete advances the order axis without changing whether publishing is offered. Self-seeds its own template + inspection rather than reusing the shared editor-seed, because it mutates order state (marks complete) and the subsystem-a specs read that seed in parallel. Registered as the lifecycle-publish project, depending on api for the admin workspace. --- playwright.config.ts | 8 + .../e2e/inspection-lifecycle-publish.spec.ts | 142 ++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 tests/e2e/inspection-lifecycle-publish.spec.ts diff --git a/playwright.config.ts b/playwright.config.ts index 54b7cdbd..e574c9ea 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -66,6 +66,14 @@ export default defineConfig({ name: 'inspection-hub', testMatch: 'inspection-hub.spec.ts', }, + { + // IA-29 / IA-30 — publishing is decoupled from order completion. + // Self-seeds its own inspection (mutates order state), so it depends + // on `api` for the admin workspace, not the shared editor-seed. + name: 'lifecycle-publish', + testMatch: 'inspection-lifecycle-publish.spec.ts', + dependencies: ['api'], + }, // ...all projects previously in playwright.api.config.ts, verbatim // (the `api` initializer project is declared first, above): { diff --git a/tests/e2e/inspection-lifecycle-publish.spec.ts b/tests/e2e/inspection-lifecycle-publish.spec.ts new file mode 100644 index 00000000..5eaef90e --- /dev/null +++ b/tests/e2e/inspection-lifecycle-publish.spec.ts @@ -0,0 +1,142 @@ +/** + * IA-29 / IA-30 regression guard — the order lifecycle does not gate report + * publishing. Before this, `POST /publish` required inspections.status === + * 'completed' and the hub hid the publish affordance until then, but no surface + * could mark an inspection completed, so publishing was unreachable — and no + * test exercised the chain, which is exactly how the break survived to v1.0.0 + * (`rg "/complete" tests/` was empty). + * + * These two specs pin the decoupling: publishing is offered while the order is + * still open, and marking fieldwork complete changes the order axis without + * changing whether publishing is offered. + * + * Self-seeded (own template + inspection) rather than reusing the shared + * editor-seed: this spec mutates order state (marks complete), and the + * editor-seed inspection is read in parallel by the subsystem-a specs. + */ +import { execFileSync } from 'node:child_process'; +import { writeFileSync, rmSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { test, expect } from '@playwright/test'; +import type { APIRequestContext, Page } from '@playwright/test'; + +const BASE_URL = process.env.FRONTEND_URL || 'http://127.0.0.1:8789'; +const NAV_TIMEOUT = 30_000; +const ADMIN_EMAIL = process.env.TEST_EMAIL || 'admin@autotest.com'; +const ADMIN_PASSWORD = process.env.TEST_PASSWORD || 'Password123!'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const APP_DIR = path.resolve(__dirname, '..', '..'); + +/** PBKDF2-SHA256 (100k iters, 16-byte salt) — matches server/lib/password.ts. */ +async function hashPassword(password: string): Promise { + const toHex = (b: Uint8Array) => Array.from(b).map((x) => x.toString(16).padStart(2, '0')).join(''); + const salt = crypto.getRandomValues(new Uint8Array(16)); + const key = await crypto.subtle.importKey('raw', new TextEncoder().encode(password), { name: 'PBKDF2' }, false, ['deriveBits']); + const bits = await crypto.subtle.deriveBits({ name: 'PBKDF2', salt, iterations: 100_000, hash: 'SHA-256' }, key, 256); + return `pbkdf2:${toHex(salt)}:${toHex(new Uint8Array(bits))}`; +} + +/** Reset the api-seeded admin's password in the LOCAL D1 for a deterministic login. */ +async function seedAdminPassword(): Promise { + const hash = await hashPassword(ADMIN_PASSWORD); + const sql = `UPDATE users SET password_hash='${hash}' WHERE email='${ADMIN_EMAIL.replace(/'/g, "''")}';`; + const sqlFile = path.join(APP_DIR, '.lifecycle-e2e-seed.tmp.sql'); + writeFileSync(sqlFile, sql, 'utf8'); + try { + execFileSync( + process.execPath, + [path.join(APP_DIR, 'scripts', 'wrangler.mjs'), 'd1', 'execute', 'DB', '--local', '--file', sqlFile], + { cwd: APP_DIR, stdio: ['ignore', 'ignore', 'inherit'] }, + ); + } finally { + rmSync(sqlFile, { force: true }); + } +} + +async function loginApi(request: APIRequestContext, email: string, password: string): Promise { + const csrf = 'deadbeefdeadbeefdeadbeefdeadbeef'; + const res = await request.post(`${BASE_URL}/api/auth/login`, { + data: { email, password }, + headers: { 'Content-Type': 'application/json', 'X-CSRF-Token': csrf, Cookie: `__Host-csrf_token=${csrf}` }, + }); + expect(res.status(), `Login failed for ${email}: expected 200`).toBe(200); + const token = (res.headers()['set-cookie'] ?? '').match(/__Host-inspector_token=([^;]+)/)?.[1] ?? ''; + expect(token, `No session cookie returned for ${email}`).toBeTruthy(); + return token; +} + +async function apiPost(request: APIRequestContext, path: string, token: string, data: Record) { + return request.post(`${BASE_URL}${path}`, { + data, + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` }, + }); +} + +async function gotoHubAuthed(page: Page, inspectionId: string, token: string) { + await page.setExtraHTTPHeaders({ Cookie: `__Host-inspector_token=${token}` }); + await page.goto(`${BASE_URL}/inspections/${inspectionId}`, { timeout: NAV_TIMEOUT, waitUntil: 'networkidle' }); +} + +let token = ''; +let inspectionId = ''; + +test.describe.serial('Publish is decoupled from order completion (IA-29 / IA-30)', () => { + test.beforeAll(async ({ request }) => { + await seedAdminPassword(); + token = await loginApi(request, ADMIN_EMAIL, ADMIN_PASSWORD); + + const richItem = (id: string, label: string) => ({ + id, label, type: 'rich' as const, + ratingOptions: ['Inspected', 'Repair'], + tabs: { information: [], limitations: [], defects: [] }, + }); + const tpl = await apiPost(request, '/api/inspections/templates', token, { + name: 'Lifecycle Publish E2E Template', + schema: { schemaVersion: 2, sections: [{ id: 's_general', title: 'General', items: [richItem('roof', 'Roof')] }] }, + }); + expect(tpl.status(), 'template creation must return 201').toBe(201); + const templateId = (await tpl.json()).data?.template?.id as string | undefined; + expect(templateId, 'template id must be returned').toBeTruthy(); + + const insp = await apiPost(request, '/api/inspections', token, { + propertyAddress: '1 Lifecycle Publish Street, Testville', + clientName: 'Lifecycle Client', + clientEmail: 'lifecycle@example.com', + templateId, + }); + expect(insp.status(), 'inspection creation must return 201').toBe(201); + inspectionId = (await insp.json()).data?.inspection?.id as string; + expect(inspectionId, 'inspection id must be returned').toBeTruthy(); + }); + + test('offers publishing while the order is still open (never completed)', async ({ page }) => { + await gotoHubAuthed(page, inspectionId, token); + + // The order is fresh — still `requested`, never marked complete — yet the + // Report card offers publishing. That is the whole fix: the order axis does + // not gate report delivery. + await expect(page.getByRole('button', { name: /mark fieldwork complete/i })).toBeVisible({ timeout: NAV_TIMEOUT }); + await expect(page.getByRole('button', { name: /publish report/i })).toBeVisible(); + }); + + test('marking fieldwork complete advances the order without affecting publish', async ({ page }) => { + await gotoHubAuthed(page, inspectionId, token); + + // Baseline: publishable, and the order is still open. + const completeBtn = page.getByRole('button', { name: /mark fieldwork complete/i }); + await expect(completeBtn).toBeVisible({ timeout: NAV_TIMEOUT }); + await expect(page.getByRole('button', { name: /publish report/i })).toBeVisible(); + + // Advance the order axis. The button belongs to the lifecycle card and + // disappears once the work is complete — a single-element signal for the + // transition (the status label itself renders in both the header and the + // card, so it is not a strict-mode-safe locator). + await completeBtn.click(); + await expect(completeBtn).toBeHidden({ timeout: NAV_TIMEOUT }); + + // The invariant: completing the order changed nothing about publishing. + await expect(page.getByRole('button', { name: /publish report/i })).toBeVisible(); + }); +}); From c74d1c83108ab33d0882c27859885d86837a226a Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 03:28:19 +0800 Subject: [PATCH 06/35] fix(agent): read defect states by the canonical key so recommendations appear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit flattenInspectionToRecommendations read each item's field state by the bare itemId and pulled `.defects` off the top of the entry — but inspection_results stores entries under the composite findingKey (`_default:{section}:{item}`) with defects nested under `.tabs`. Both halves were wrong, so defectStates was always empty and `/agent-recommendations` came back empty for every agent, no matter how many defects a delivered inspection had. It is an agent account's only defect view. Extract the read the report service already did correctly into readItemEntry / readItemDefectStates (server/lib/read-item-defects.ts) and route both the report path and the agent path through it, so the key resolution has one home and cannot drift again. agent-referral-people.spec.ts was a false green over this exact bug: its fixture stored `{ item1: { defects: [...] } }` — the bare-key, no-tabs shape the UI never writes — which only the broken reader could parse. Corrected to the canonical storage shape, so it now guards the real path. Fixing the read unmasks IA-41 (custom defects and tenant-defined categories are still dropped by the cannedById / category guards) — tracked separately. --- server/lib/read-item-defects.ts | 32 +++++++++++ server/services/agent-recommendations.ts | 23 +++----- .../inspection/inspection-report.service.ts | 6 +- .../unit/agent/agent-recommendations.spec.ts | 57 +++++++++++++++++++ .../unit/agents/agent-referral-people.spec.ts | 6 +- 5 files changed, 105 insertions(+), 19 deletions(-) create mode 100644 server/lib/read-item-defects.ts create mode 100644 tests/unit/agent/agent-recommendations.spec.ts diff --git a/server/lib/read-item-defects.ts b/server/lib/read-item-defects.ts new file mode 100644 index 00000000..5c91d176 --- /dev/null +++ b/server/lib/read-item-defects.ts @@ -0,0 +1,32 @@ +/** + * Single source of truth for reading one template item's stored result entry + * out of `inspection_results.data`. + * + * Entries are keyed by the composite `findingKey(_default, sectionId, itemId)` + * (`"_default:{sectionId}:{itemId}"`), with a bare-`itemId` fallback for any + * pre-composite-key rows. Getting either half wrong reads nothing: the report + * service had this right, but the agent-recommendations path keyed by bare + * itemId AND skipped the `.tabs` layer, so `/agent-recommendations` came back + * empty for every agent regardless of how many defects existed (IA-31). Both + * sides now go through here so the read cannot drift apart again. + */ +import { findingKey, DEFAULT_UNIT } from './finding-key'; +import type { ItemEntry, DefectState, ResultsProjection } from './collab/results-doc.types'; + +/** The stored entry for one item, or an empty entry when nothing is recorded. */ +export function readItemEntry( + resultData: ResultsProjection, + sectionId: string, + itemId: string, +): ItemEntry { + return resultData[findingKey(DEFAULT_UNIT, sectionId, itemId)] || resultData[itemId] || {}; +} + +/** The per-defect field states for one item — the `.tabs.defects` list. */ +export function readItemDefectStates( + resultData: ResultsProjection, + sectionId: string, + itemId: string, +): DefectState[] { + return readItemEntry(resultData, sectionId, itemId).tabs?.defects ?? []; +} diff --git a/server/services/agent-recommendations.ts b/server/services/agent-recommendations.ts index 5127004e..c54ba888 100644 --- a/server/services/agent-recommendations.ts +++ b/server/services/agent-recommendations.ts @@ -9,6 +9,8 @@ // override the canned default; same for photos. import type { DefectCategory } from '../types/template-schema'; +import { readItemDefectStates } from '../lib/read-item-defects'; +import type { ResultsProjection, DefectState } from '../lib/collab/results-doc.types'; export interface AgentRecommendationRow { inspectionId: string; @@ -41,17 +43,6 @@ interface ItemShape { id: string; label: string; tabs?: { defects?: CannedDef interface SectionShape { id: string; title: string; items?: ItemShape[] } interface SnapshotShape { sections?: SectionShape[] } -interface DefectStateShape { - cannedId: string; - included?: boolean; - comment?: string | null; - category?: string; - location?: string | null; - photos?: Array<{ key?: string } | string> | null; -} -interface ResultsItemShape { defects?: DefectStateShape[] } -interface ResultsDataShape { [itemId: string]: ResultsItemShape | undefined } - export interface RawInspectionForRecommendations { id: string; propertyAddress: string; @@ -64,7 +55,7 @@ function isCategory(v: unknown): v is DefectCategory { return v === 'safety' || v === 'recommendation' || v === 'maintenance'; } -function flattenPhotos(photos: DefectStateShape['photos']): string[] { +function flattenPhotos(photos: DefectState['photos']): string[] { if (!photos || !Array.isArray(photos)) return []; const out: string[] = []; for (const p of photos) { @@ -88,15 +79,17 @@ export function flattenInspectionToRecommendations( // D1 sometimes hands back a JSON column as a raw string (depending on // how the row was originally written); coerce defensively. const snapshot = coerceJson(insp.templateSnapshot); - const results = coerceJson(insp.resultsData); + const results = coerceJson(insp.resultsData) ?? {}; if (!snapshot || !Array.isArray(snapshot.sections)) return []; const out: AgentRecommendationRow[] = []; for (const section of snapshot.sections) { for (const item of section.items ?? []) { const cannedDefects = item.tabs?.defects ?? []; - const itemResults = results?.[item.id]; - const defectStates = itemResults?.defects ?? []; + // Read via the shared resolver — the canonical findingKey with a + // bare-itemId fallback, then `.tabs.defects`. Keying by bare itemId + // or skipping `.tabs` (the prior bug) reads nothing (IA-31). + const defectStates = readItemDefectStates(results, section.id, item.id); // Index canned by id for fast lookup. const cannedById = new Map(); for (const c of cannedDefects) cannedById.set(c.id, c); diff --git a/server/services/inspection/inspection-report.service.ts b/server/services/inspection/inspection-report.service.ts index 3e041991..860e287e 100644 --- a/server/services/inspection/inspection-report.service.ts +++ b/server/services/inspection/inspection-report.service.ts @@ -10,13 +10,13 @@ import { mapRatingSystemLevels } from '../../lib/map-rating-levels'; import { renderTemplate } from '../../lib/mustache'; import { mapRepairItems } from '../../lib/report-repair-items'; import { selectReportMedia, type ReportMediaContext } from '../../lib/report-video'; -import { findingKey, DEFAULT_UNIT } from '../../lib/finding-key'; +import { readItemEntry } from '../../lib/read-item-defects'; import { sha256Hex } from '../signing-key.service'; import { RENDER_VERSION } from '../../lib/pdf'; import { resolvePdfSettings, type PdfSettings } from '../../lib/pdf-settings'; import { isReportPublished } from '../../lib/status/report-status'; import { resolveBuildingProfile } from '../../lib/building-profile'; -import { buildSystemsSummary } from '../../lib/pca-systems-summary'; +import type { buildSystemsSummary } from '../../lib/pca-systems-summary'; import { buildPcaReportBlock } from '../../lib/pca-report-block'; import { gatedSectionRegistry } from '../../lib/pca-section-registry'; import { buildReportOutline } from '../../lib/report-outline'; @@ -269,7 +269,7 @@ export class InspectionReportService extends InspectionSubService { : null, alwaysPageBreak: sec.alwaysPageBreak === true, items: sec.items.map((item: SchemaItem) => { - const res = resultData[findingKey(DEFAULT_UNIT, sec.id, item.id)] || resultData[item.id] || {}; + const res = readItemEntry(resultData, sec.id, item.id); const ratingId = res.rating ?? null; const bucket = getRatingBucket(ratingId, levels); const level = levels.find((l: RatingLevel) => l.id === ratingId); diff --git a/tests/unit/agent/agent-recommendations.spec.ts b/tests/unit/agent/agent-recommendations.spec.ts new file mode 100644 index 00000000..bb47de03 --- /dev/null +++ b/tests/unit/agent/agent-recommendations.spec.ts @@ -0,0 +1,57 @@ +/** + * IA-31 — `/agent-recommendations` returned empty for every agent because + * flattenInspectionToRecommendations read the stored defect states by the bare + * itemId (not the composite findingKey) AND skipped the `.tabs` layer, so + * defectStates was always []. These specs pin the storage-shaped read: the + * fixture is keyed exactly as inspection_results.data is written + * (`_default:{sectionId}:{itemId}` → `.tabs.defects[]`). + */ +import { describe, it, expect } from 'vitest'; +import { flattenInspectionToRecommendations, type RawInspectionForRecommendations } from '../../../server/services/agent-recommendations'; + +const snapshot = { + sections: [{ + id: 's_roof', + title: 'Roof', + items: [{ + id: 'i_shingles', + label: 'Shingles', + tabs: { defects: [{ id: 'd_missing', title: 'Missing shingles', category: 'safety', comment: 'Replace missing shingles.' }] }, + }], + }], +}; + +function raw(resultsData: unknown): RawInspectionForRecommendations { + return { id: 'insp-1', propertyAddress: '1 Main St', date: '2026-06-01', templateSnapshot: snapshot, resultsData }; +} + +describe('flattenInspectionToRecommendations — storage-shaped results', () => { + it('reads defect state from the canonical _default:section:item key with the tabs layer', () => { + const rows = flattenInspectionToRecommendations(raw({ + '_default:s_roof:i_shingles': { tabs: { defects: [{ cannedId: 'd_missing', included: true }] } }, + })); + expect(rows).toHaveLength(1); + expect(rows[0].defectTitle).toBe('Missing shingles'); + expect(rows[0].category).toBe('safety'); + expect(rows[0].itemLabel).toBe('Shingles'); + }); + + it('still reads a legacy bare-itemId row (pre-composite-key fallback), also under .tabs', () => { + const rows = flattenInspectionToRecommendations(raw({ + 'i_shingles': { tabs: { defects: [{ cannedId: 'd_missing', included: true }] } }, + })); + expect(rows).toHaveLength(1); + expect(rows[0].defectTitle).toBe('Missing shingles'); + }); + + it('excludes defects the field marked not included', () => { + const rows = flattenInspectionToRecommendations(raw({ + '_default:s_roof:i_shingles': { tabs: { defects: [{ cannedId: 'd_missing', included: false }] } }, + })); + expect(rows).toHaveLength(0); + }); + + it('returns nothing when there are no recorded results', () => { + expect(flattenInspectionToRecommendations(raw({}))).toHaveLength(0); + }); +}); diff --git a/tests/unit/agents/agent-referral-people.spec.ts b/tests/unit/agents/agent-referral-people.spec.ts index 734c24ec..31632577 100644 --- a/tests/unit/agents/agent-referral-people.spec.ts +++ b/tests/unit/agents/agent-referral-people.spec.ts @@ -105,7 +105,11 @@ describe('listRecommendationsForAgent — buyer_agent via inspection_people (Tas }], }], }; - const resultsData = { item1: { defects: [{ cannedId: 'd1', included: true }] } }; + // Canonical storage shape: composite findingKey + `.tabs.defects` — exactly + // what the editor writes. The prior fixture used a bare itemId with a + // top-level `.defects`, a shape the UI never produces; that made this test a + // false green over the IA-31 read bug (page empty for every real agent). + const resultsData = { '_default:sec1:item1': { tabs: { defects: [{ cannedId: 'd1', included: true }] } } }; it('legacy referredByAgentId NULL, buyer_agent inspection_people row present — surfaces the recommendation', async () => { await db.insert(schema.inspections).values({ From acfe1723830cb2d77f04ab28f052264f2d5d8676 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 03:46:42 +0800 Subject: [PATCH 07/35] fix(pca): map the produced bucket domain in the Systems Summary severity roll-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildSystemsSummary rolls up each system's worst condition from the item `severityBucket`, but the report pipeline only ever writes the getRatingBucket domain (`satisfactory | monitor | defect | other`). asSeverity accepted the OTHER axis (`marginal | significant | minor`), so every real bucket fell through to 'good' and worstSeverity was always 'good' — the commercial PCA summary printed "Good" for a system even while the same row counted four safety defects. Map the buckets to the severity axis (the inverse of getRatingBucket) so the roll-up reflects what was actually rated. This feeds both the web report and the DOCX export (via pca-report-block → report-export-consumer); there is no duplicate asSeverity to change. Two report specs fed the fix its own blind spot: pca-systems-summary.spec.ts and pca-report-block.spec.ts both set severityBucket to severity-axis values ('marginal'/'significant'/'good') the pipeline never emits, so the always-good bug stayed green. Both now feed the bucket domain. IA-43 tracks unifying the two domains so this local inverse can be retired. --- server/lib/pca-systems-summary.ts | 18 ++++++++-- tests/unit/reports/pca-report-block.spec.ts | 5 ++- .../unit/reports/pca-systems-summary.spec.ts | 36 +++++++++++++++---- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/server/lib/pca-systems-summary.ts b/server/lib/pca-systems-summary.ts index f8332c43..88a1ff08 100644 --- a/server/lib/pca-systems-summary.ts +++ b/server/lib/pca-systems-summary.ts @@ -38,10 +38,22 @@ const SEVERITY_RANK: Record = { significant: 3, }; +/** + * Normalize an item's `severityBucket` (the getRatingBucket domain + * `satisfactory | monitor | defect | other`) to the severity axis this table + * ranks. It is the inverse of getRatingBucket — the summary reads bucket + * values, so accepting the severity domain directly (the prior bug) made every + * real bucket fall through to 'good'. IA-43 tracks unifying the two domains so + * this local inverse can be retired. + */ function asSeverity(bucket: string | undefined): SeverityRank { - return bucket === 'marginal' || bucket === 'significant' || bucket === 'minor' - ? bucket - : 'good'; + switch (bucket) { + case 'defect': return 'significant'; + case 'monitor': return 'marginal'; + case 'other': return 'minor'; + case 'satisfactory': return 'good'; + default: return 'good'; + } } export function buildSystemsSummary(sections: SystemsSummaryInput[]): SystemsSummaryRow[] { diff --git a/tests/unit/reports/pca-report-block.spec.ts b/tests/unit/reports/pca-report-block.spec.ts index 25c55bda..d3c51012 100644 --- a/tests/unit/reports/pca-report-block.spec.ts +++ b/tests/unit/reports/pca-report-block.spec.ts @@ -7,8 +7,11 @@ import { describe, it, expect } from 'vitest'; import { buildPcaReportBlock } from '../../../server/lib/pca-report-block'; import { PCA_NARRATIVE_SEED } from '../../../server/lib/pca-narrative'; +// severityBucket carries the getRatingBucket domain the report pipeline emits +// (`monitor` → marginal severity); the prior 'marginal' here was a value that +// domain never produces (IA-32). const sections = [ - { id: 'site', title: 'Site', items: [{ severityBucket: 'marginal', resolvedTabs: { defects: [{ included: true, effectiveCategory: 'safety' }] } }] }, + { id: 'site', title: 'Site', items: [{ severityBucket: 'monitor', resolvedTabs: { defects: [{ included: true, effectiveCategory: 'safety' }] } }] }, ]; describe('buildPcaReportBlock — commercial gate', () => { diff --git a/tests/unit/reports/pca-systems-summary.spec.ts b/tests/unit/reports/pca-systems-summary.spec.ts index 3766be02..8ecd8b89 100644 --- a/tests/unit/reports/pca-systems-summary.spec.ts +++ b/tests/unit/reports/pca-systems-summary.spec.ts @@ -1,19 +1,29 @@ -// tests/unit/pca-systems-summary.spec.ts +// tests/unit/reports/pca-systems-summary.spec.ts import { describe, it, expect } from 'vitest'; import { buildSystemsSummary } from '../../../server/lib/pca-systems-summary'; +/** + * IA-32 — the summary reads each item's `severityBucket`, and the report + * pipeline only ever writes the getRatingBucket domain + * (`satisfactory | monitor | defect | other`). asSeverity used to accept the + * OTHER domain (`marginal | significant | minor`), so every real bucket fell + * through to 'good' and worstSeverity was always 'good' — a system with four + * safety defects still printed "Good". These fixtures feed the bucket values + * the pipeline actually produces; the prior fixtures fed severity values it + * never emits, which is how the correctness bug stayed green. + */ describe('buildSystemsSummary', () => { - it('returns one row per system with worst severity + included-defect category counts', () => { + it('rolls up the worst severity across items using the produced bucket domain', () => { const sections = [ { id: 'mep', title: 'Mechanical, Electrical & Plumbing', items: [ - { rating: 'd', severityBucket: 'marginal', resolvedTabs: { defects: [ + { rating: 'd', severityBucket: 'monitor', resolvedTabs: { defects: [ { included: true, effectiveCategory: 'safety' }, { included: true, effectiveCategory: 'maintenance' }, { included: false, effectiveCategory: 'safety' }, // excluded -> not counted ] } }, - { rating: 'd', severityBucket: 'significant', resolvedTabs: { defects: [ + { rating: 'd', severityBucket: 'defect', resolvedTabs: { defects: [ { included: true }, // no category -> recommendation default ] } }, ], @@ -24,13 +34,25 @@ describe('buildSystemsSummary', () => { expect(rows[0]).toMatchObject({ systemId: 'mep', systemTitle: 'Mechanical, Electrical & Plumbing', - worstSeverity: 'significant', // significant beats marginal + worstSeverity: 'significant', // defect (→significant) beats monitor (→marginal) counts: { safety: 1, recommendation: 1, maintenance: 1 }, }); }); - it('defaults worstSeverity to good for a system with no rated defects', () => { - const sections = [{ id: 'site', title: 'Site', items: [{ rating: 'g', severityBucket: 'good' }] }]; + it('maps a single defect item to significant (the core bug: it used to read good)', () => { + const sections = [{ id: 'roof', title: 'Roof', items: [{ rating: 'd', severityBucket: 'defect' }] }]; + expect(buildSystemsSummary(sections as never)[0].worstSeverity).toBe('significant'); + }); + + it('maps monitor to marginal and other to minor', () => { + const monitor = buildSystemsSummary([{ id: 's', title: 'S', items: [{ severityBucket: 'monitor' }] }] as never); + expect(monitor[0].worstSeverity).toBe('marginal'); + const other = buildSystemsSummary([{ id: 's', title: 'S', items: [{ severityBucket: 'other' }] }] as never); + expect(other[0].worstSeverity).toBe('minor'); + }); + + it('defaults worstSeverity to good for a system of only satisfactory items', () => { + const sections = [{ id: 'site', title: 'Site', items: [{ rating: 'g', severityBucket: 'satisfactory' }] }]; const rows = buildSystemsSummary(sections as never); expect(rows[0].worstSeverity).toBe('good'); expect(rows[0].counts).toEqual({ safety: 0, recommendation: 0, maintenance: 0 }); From 5884898e2d48a730414ad5d3d1126dd0130efd09 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 04:03:42 +0800 Subject: [PATCH 08/35] fix(report): keep internal_notes and price out of the public report payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getReportData spread the whole inspections row into data.inspection, and GET /api/public/report/:tenant/:id returns that data verbatim to any link holder — including agent-kind tokens. The row carries internal_notes (the inspector's private notes) and price_cents (commercial pricing), which the account track already withholds from agents at the schema layer (agent.schema.ts). The public track only avoided leaking them because the SSR page happens not to render them — page-doesn't-render is not a boundary. Delete both from the object at the point it becomes the public payload. The internal `inspection` row keeps every column for getReportData's own logic, so nothing downstream of the read changes. The audit defers severity and any broader projection to a dedicated security review; this is the boundary it names. --- scripts/file-size-baseline.json | 2 +- .../inspection/inspection-report.service.ts | 9 ++- .../public-report-field-projection.spec.ts | 56 +++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 tests/unit/reports/public-report-field-projection.spec.ts diff --git a/scripts/file-size-baseline.json b/scripts/file-size-baseline.json index 4a55cdde..e8055b9e 100644 --- a/scripts/file-size-baseline.json +++ b/scripts/file-size-baseline.json @@ -3,7 +3,7 @@ "server/services/inspection/inspection-core.service.ts": 1083, "app/routes/inspection-hub.tsx": 1048, "server/services/booking.service.ts": 967, - "server/services/inspection/inspection-report.service.ts": 903, + "server/services/inspection/inspection-report.service.ts": 910, "server/durable-objects/inspection-doc.ts": 896, "app/routes/inspections.tsx": 892, "server/lib/collab/results-doc.ts": 874, diff --git a/server/services/inspection/inspection-report.service.ts b/server/services/inspection/inspection-report.service.ts index 860e287e..13f97bbd 100644 --- a/server/services/inspection/inspection-report.service.ts +++ b/server/services/inspection/inspection-report.service.ts @@ -765,8 +765,15 @@ export class InspectionReportService extends InspectionSubService { ? defectCountsByUnit(matrixUnits, resultData as Record, levels) : {}; + // IA-33 (boundary A) — this object is returned verbatim by the public + // report endpoint to any link holder (incl. agent-kind tokens); strip + // the columns the account track withholds from agents (agent.schema.ts): + // private `internalNotes` and commercial `price`. + const publicInspection = { ...inspection, inspectorName }; + delete (publicInspection as Partial).internalNotes; + delete (publicInspection as Partial).price; return { - inspection: { ...inspection, inspectorName }, + inspection: publicInspection, styleProfile: { ...styleProfile, tokens: styleProfile.tokens as Record }, inspectorCredentials: credentialSnapshot, amendmentTrail, diff --git a/tests/unit/reports/public-report-field-projection.spec.ts b/tests/unit/reports/public-report-field-projection.spec.ts new file mode 100644 index 00000000..781ad840 --- /dev/null +++ b/tests/unit/reports/public-report-field-projection.spec.ts @@ -0,0 +1,56 @@ +/** + * IA-33 (boundary A) — getReportData spreads the whole inspections row into + * data.inspection, and GET /api/public/report/:tenant/:id returns that data + * verbatim to any holder of the link (incl. agent-kind tokens). The row + * carries `internal_notes` (inspector's private notes) and `price_cents` + * (commercial pricing), which the account track explicitly withholds from + * agents at the schema layer. The public report track must not expose them + * just because the current SSR page happens not to render them. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { InspectionService } from '../../../server/services/inspection.service'; +import { createTestDb, setupSchema } from '../db'; +import * as schema from '../../../server/lib/db/schema'; +import type { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; + +vi.mock('drizzle-orm/d1', () => ({ drizzle: vi.fn() })); +import { drizzle as mockDrizzle } from 'drizzle-orm/d1'; + +const TENANT = 'aa000000-0000-0000-0000-000000000001'; +const INSPECTION = 'cc000000-0000-0000-0000-000000000003'; + +describe('getReportData — public field projection (IA-33 boundary A)', () => { + let svc: InspectionService; + let testDb: BetterSQLite3Database; + + beforeEach(async () => { + const fix = createTestDb(); + testDb = fix.db; + await setupSchema(fix.sqlite); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (mockDrizzle as any).mockReturnValue(testDb); + svc = new InspectionService({} as D1Database); + await testDb.insert(schema.tenants).values({ + id: TENANT, name: 'Acme', slug: 'acme', status: 'active', + deploymentMode: 'shared', tier: 'free', createdAt: new Date(), + }); + await testDb.insert(schema.inspections).values({ + id: INSPECTION, tenantId: TENANT, + propertyAddress: '1 Main St', date: '2026-06-01', + status: 'completed', reportStatus: 'published', paymentStatus: 'unpaid', + price: 45000, internalNotes: 'Client was rude; do not rebook.', + paymentRequired: false, agreementRequired: false, createdAt: new Date(), + }); + }); + + it('omits internal_notes and price_cents from data.inspection', async () => { + const data = await svc.getReportData(INSPECTION, TENANT) as unknown as { + inspection: Record; + }; + expect(data.inspection).not.toHaveProperty('internalNotes'); + expect(data.inspection).not.toHaveProperty('price'); + // Non-sensitive fields the report needs still come through. + expect(data.inspection.propertyAddress).toBe('1 Main St'); + expect(data.inspection).toHaveProperty('date', '2026-06-01'); + }); +}); From 91ebac9fe2344cc674c4bd8cc04881e9a6ece3eb Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 04:30:55 +0800 Subject: [PATCH 09/35] fix(repair): resolve the actor kind instead of assuming client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveBuilderAccess Path 1 assigned creator.kind='client' to any resolvable portal token, so an agent-kind token (buyer_agent / listing_agent) acted as the client on the repair builder's five write endpoints, and an other-kind token (attorney, title company, …) acted as one too. The sibling client-actor resolver already refuses non-client kinds; Path 1 did not. Resolve the grant's role kind via a new PortalAccessService.getRoleKind (role KEY → contact_role_profiles.kind, so tenant-custom profiles are honored, not just the seed keys): client/co_client → client, agent → agent, anything else → no builder actor (rejected). An agent now reaches the builder as an agent, and the two agent paths (portal token vs agent-portal session) resolve to the same kind — asserted directly so they cannot drift apart again. This is IA-73's unconditional half (stop the identity impersonation). The tenant-level agentRepairAccess switch (off/read/readwrite) that gates whether agents may act at all lands next, on top of this. --- server/lib/repair-access.ts | 19 +++- server/services/portal-access.service.ts | 20 +++++ .../helpers/repair-builder-routes-harness.ts | 4 + .../repair/repair-builder-actor-kind.spec.ts | 87 +++++++++++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 tests/unit/repair/repair-builder-actor-kind.spec.ts diff --git a/server/lib/repair-access.ts b/server/lib/repair-access.ts index 74901345..1698f75e 100644 --- a/server/lib/repair-access.ts +++ b/server/lib/repair-access.ts @@ -30,11 +30,24 @@ export async function resolveBuilderAccess( ): Promise<{ tenantId: string; creator: Creator; ownerPreview: boolean } | null> { const token = c.req.query('token'); - // Path 1: persistent portal token (client / co_client / agent role). + // Path 1: persistent portal token. The grant's role kind decides the actor — + // assuming 'client' for every resolvable token let an agent-kind token + // (buyer_agent / listing_agent) act as the client on the builder's five + // write endpoints (IA-35), exactly what the sibling client-actor resolver + // guards against. client/co_client → client; agent → agent; anything else + // (attorney, title company, …) has no builder role → reject. const grant = await resolvePortalAccess(c.var.services.portalAccess, token, id); if (grant) { - const creator: Creator = { kind: 'client', ref: grant.recipientEmail }; - return { tenantId: grant.tenantId, creator, ownerPreview: false }; + const kind = await c.var.services.portalAccess.getRoleKind(grant.tenantId, grant.role); + if (kind === 'client') { + const creator: Creator = { kind: 'client', ref: grant.recipientEmail }; + return { tenantId: grant.tenantId, creator, ownerPreview: false }; + } + if (kind === 'agent') { + const creator: Creator = { kind: 'agent', ref: grant.recipientEmail }; + return { tenantId: grant.tenantId, creator, ownerPreview: false }; + } + return null; } // Path 2: legacy KV agent-view token (existing share links). diff --git a/server/services/portal-access.service.ts b/server/services/portal-access.service.ts index 593b4473..0fdd88d7 100644 --- a/server/services/portal-access.service.ts +++ b/server/services/portal-access.service.ts @@ -2,6 +2,7 @@ import { drizzle } from 'drizzle-orm/d1'; import { eq, and } from 'drizzle-orm'; import { inspectionAccessTokens } from '../lib/db/schema/portal-access'; import { contactRoleProfiles } from '../lib/db/schema'; +import type { RoleKind } from '../lib/people/capabilities'; import type { PortalAccessRow, PortalRole } from '../lib/public-access'; import { mintToken, hashToken, deadTokenSentinel, resolveTokenRow } from '../lib/token-hash'; import { sealToken, openToken } from '../lib/config-crypto'; @@ -51,6 +52,25 @@ export class PortalAccessService { return openToken(row.tokenEnc, row.tenantId, s.jwtSecret, s.jwtSecretPrevious); } + /** + * Resolve a portal grant's role KEY to its role-profile KIND + * (client / agent / other). The `role` on a token is a free-form key, so a + * caller that needs to know "is this an agent?" must map key → kind here + * rather than hardcode the seed keys (which misses tenant-custom profiles). + * Returns null when the key has no active profile for the tenant. + */ + async getRoleKind(tenantId: string, roleKey: string): Promise { + const db = this.getDrizzle(); + const row = await db.select({ kind: contactRoleProfiles.kind }).from(contactRoleProfiles) + .where(and( + eq(contactRoleProfiles.tenantId, tenantId), + eq(contactRoleProfiles.key, roleKey), + eq(contactRoleProfiles.active, true), + )) + .get(); + return (row?.kind as RoleKind | undefined) ?? null; + } + /** * The `role` column is a free-form role-profile KEY, not a fixed enum — * validate it against the tenant's active `contact_role_profiles` so a diff --git a/tests/unit/helpers/repair-builder-routes-harness.ts b/tests/unit/helpers/repair-builder-routes-harness.ts index c55225f9..d2098df4 100644 --- a/tests/unit/helpers/repair-builder-routes-harness.ts +++ b/tests/unit/helpers/repair-builder-routes-harness.ts @@ -84,6 +84,7 @@ export function makeServices(overrides: { creditTotal?: ReturnType; assertCanEdit?: ReturnType; accessToInspection?: ReturnType; + portalAccessGetRoleKind?: ReturnType; } = {}) { const defaultPortalToken = vi.fn().mockResolvedValue(null); const defaultAgent = vi.fn().mockResolvedValue(null); @@ -93,6 +94,9 @@ export function makeServices(overrides: { return { portalAccess: { resolveToken: overrides.portalAccessResolveToken ?? defaultPortalToken, + // Default matches VALID_TOKEN_ROW.role ('client') so existing specs + // keep resolving a client creator. + getRoleKind: overrides.portalAccessGetRoleKind ?? vi.fn().mockResolvedValue('client'), }, inspection: { resolveAgentViewToken: overrides.resolveAgentViewToken ?? defaultAgent, diff --git a/tests/unit/repair/repair-builder-actor-kind.spec.ts b/tests/unit/repair/repair-builder-actor-kind.spec.ts new file mode 100644 index 00000000..fdc48287 --- /dev/null +++ b/tests/unit/repair/repair-builder-actor-kind.spec.ts @@ -0,0 +1,87 @@ +/** + * IA-35 (boundary C) — resolveBuilderAccess Path 1 assigned creator.kind='client' + * to ANY resolvable portal token, so an agent-kind token (buyer_agent / + * listing_agent) acted as the CLIENT on the repair-builder's write endpoints. + * The actor's kind must come from the grant's role kind, matching the sibling + * client-actor resolver — client/co_client → client, agent → agent, anything + * else → rejected. + * + * Parity: an agent reaching the builder via a portal token (Path 1) and via an + * agent-portal session (Path 4) must resolve to the SAME actor kind — the whole + * point of the fix is that the two tracks stop disagreeing. + */ +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('drizzle-orm/d1', () => ({ drizzle: vi.fn() })); +vi.mock('../../../server/lib/public-access', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + resolveOwnerPreviewFull: vi.fn().mockResolvedValue(null), + resolveAgentSession: vi.fn().mockResolvedValue(null), + }; +}); + +import { resolveAgentSession } from '../../../server/lib/public-access'; +import { makeServices, buildApp, VALID_TOKEN_ROW } from '../helpers/repair-builder-routes-harness'; + +/** The GET /source handler passes the resolved creator to listMineWithItems. */ +async function creatorFromSource(services: ReturnType): Promise { + const { app } = buildApp({ services, reportStatus: 'published', enableCustomerRepairExport: true }); + const res = await app.request('/api/public/repair-builder/t1/insp1/source?token=tok1'); + expect(res.status).toBe(200); + const call = (services.repairRequest.listMineWithItems as ReturnType).mock.calls[0]; + return call?.[2]; +} + +describe('resolveBuilderAccess — actor kind from the grant role (IA-35)', () => { + it('a client-role token resolves a client actor (unchanged)', async () => { + const services = makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue(VALID_TOKEN_ROW), + portalAccessGetRoleKind: vi.fn().mockResolvedValue('client'), + }); + expect(await creatorFromSource(services)).toEqual({ kind: 'client', ref: 'buyer@example.com' }); + }); + + it('an agent-role token resolves an AGENT actor, not a client', async () => { + const services = makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue({ ...VALID_TOKEN_ROW, role: 'buyer_agent', recipientEmail: 'agent@example.com' }), + portalAccessGetRoleKind: vi.fn().mockResolvedValue('agent'), + }); + expect(await creatorFromSource(services)).toEqual({ kind: 'agent', ref: 'agent@example.com' }); + }); + + it('an other-kind token (attorney, title company, …) is rejected — no builder actor', async () => { + const services = makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue({ ...VALID_TOKEN_ROW, role: 'attorney' }), + portalAccessGetRoleKind: vi.fn().mockResolvedValue('other'), + }); + const { app } = buildApp({ services, reportStatus: 'published' }); + const res = await app.request('/api/public/repair-builder/t1/insp1/source?token=tok1'); + expect(res.status).toBe(401); // no builder actor → auth gate rejects + }); + + it('parity: an agent via portal token (Path 1) and via agent session (Path 4) resolve the same kind', async () => { + // Path 1 — agent portal token. + const viaToken = makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue({ ...VALID_TOKEN_ROW, role: 'buyer_agent', recipientEmail: 'a@x.com' }), + portalAccessGetRoleKind: vi.fn().mockResolvedValue('agent'), + }); + const tokenCreator = await creatorFromSource(viaToken) as { kind: string }; + + // Path 4 — agent-portal session (no portal token; session resolves the inspection). + vi.mocked(resolveAgentSession).mockResolvedValueOnce({ userId: 'agent-user-1' } as never); + const viaSession = makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue(null), + accessToInspection: vi.fn().mockResolvedValue({ tenantId: 't1' }), + }); + const { app } = buildApp({ services: viaSession, reportStatus: 'published' }); + const res = await app.request('/api/public/repair-builder/t1/insp1/source'); + expect(res.status).toBe(200); + const sessionCreator = (viaSession.repairRequest.listMineWithItems as ReturnType).mock.calls[0]?.[2] as { kind: string }; + + expect(tokenCreator.kind).toBe('agent'); + expect(sessionCreator.kind).toBe('agent'); + expect(tokenCreator.kind).toBe(sessionCreator.kind); + }); +}); From 6d8857ae9a947ba8723fde88024cd1f20c1c1386 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 05:09:50 +0800 Subject: [PATCH 10/35] feat(repair): tenant switch for agent access to the repair list (IA-73) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes IA-35: after the identity fix labeled agents correctly, this lets a tenant decide whether agents may act on the repair request list at all — `off` (no access), `read` (view only), `readwrite` (full, the default). Default readwrite: account-track agents already receive a builder link (dashboard), so defaulting off would remove a shipped ability. The setting lives in the inspectionPrefs JSON (no migration) and is read by InspectionCoreService.getAgentRepairAccess. resolveBuilderAccess now carries an accessLevel and consumes the switch in every agent branch — portal token (Path 1), legacy KV token (Path 2), and agent session (Path 4) — so the two agent tracks can never diverge. The shared write guard (runAssertCanEdit) and the create-list handler refuse a read-only actor with 403; client and inspector actors are always readwrite. Settings → Inspection exposes the three levels as a RadioGroup (matching the sibling controls on that page rather than the plan's RadioCardGroup, for visual consistency). Tests cover all three states plus a direct assertion that the token track and the session track resolve to the same action set. --- app/hooks/useInspectionPrefs.ts | 4 ++ app/routes/settings-inspection.tsx | 15 +++++ messages/en/settings-catalog.json | 5 ++ scripts/file-size-baseline.json | 6 +- server/api/repair-builder.ts | 12 ++-- server/lib/db/schema/tenant/core.ts | 2 +- server/lib/repair-access.ts | 40 ++++++++---- server/lib/repair-gates.ts | 7 ++ .../validations/inspection-prefs.schema.ts | 7 ++ server/services/inspection.service.ts | 17 +++-- .../inspection/inspection-core.service.ts | 14 ++++ .../helpers/repair-builder-routes-harness.ts | 3 + .../repair/repair-builder-actor-kind.spec.ts | 65 +++++++++++++++++++ 13 files changed, 170 insertions(+), 27 deletions(-) diff --git a/app/hooks/useInspectionPrefs.ts b/app/hooks/useInspectionPrefs.ts index 2fdba981..dbcbfb55 100644 --- a/app/hooks/useInspectionPrefs.ts +++ b/app/hooks/useInspectionPrefs.ts @@ -16,6 +16,9 @@ export interface InspectionPrefs { /** Track H (IA-7) — which defect fields the publish gate REQUIRES * tenant-wide. 'none' (default) = gaps warn but never block. */ requireDefectFields: RequireDefectFields; + /** IA-35 / IA-73 — tenant policy for agent access to the repair list. + * 'readwrite' (default) = agents may view and edit. */ + agentRepairAccess: 'off' | 'read' | 'readwrite'; } const DEFAULTS: InspectionPrefs = { @@ -24,6 +27,7 @@ const DEFAULTS: InspectionPrefs = { autoAdvanceDelayMs: 200, pinnedTagIds: [], requireDefectFields: 'none', + agentRepairAccess: 'readwrite', }; /** diff --git a/app/routes/settings-inspection.tsx b/app/routes/settings-inspection.tsx index 85b07865..23b2f795 100644 --- a/app/routes/settings-inspection.tsx +++ b/app/routes/settings-inspection.tsx @@ -96,6 +96,21 @@ export default function SettingsInspectionPage() { /> +
+

{m.settings_inspection_agent_repair_heading()}

+

{m.settings_inspection_agent_repair_hint()}

+ patch({ agentRepairAccess: v as 'off' | 'read' | 'readwrite' })} + options={[ + { value: 'off', label: m.settings_inspection_agent_repair_off() }, + { value: 'read', label: m.settings_inspection_agent_repair_read() }, + { value: 'readwrite', label: m.settings_inspection_agent_repair_readwrite() }, + ]} + /> +
+

{m.settings_inspection_pinned_heading({ count: prefs.pinnedTagIds.length })}

{m.settings_inspection_pinned_help()}

diff --git a/messages/en/settings-catalog.json b/messages/en/settings-catalog.json index c2cc7ce7..a7ef41cb 100644 --- a/messages/en/settings-catalog.json +++ b/messages/en/settings-catalog.json @@ -34,6 +34,11 @@ "settings_inspection_required_location": "Location required", "settings_inspection_required_trade": "Recommended trade required", "settings_inspection_required_both": "Location + trade required", + "settings_inspection_agent_repair_heading": "Agent access to repair requests", + "settings_inspection_agent_repair_hint": "Agents can always view the report itself. This controls the repair request list.", + "settings_inspection_agent_repair_off": "No access", + "settings_inspection_agent_repair_read": "View only", + "settings_inspection_agent_repair_readwrite": "View and edit", "settings_inspection_pinned_heading": "Pinned tags ({count}/5)", "settings_inspection_pinned_help": "Up to 5 tags shown as 1-click chips below the Notes field.", "settings_inspection_manage_tags": "Manage tag library", diff --git a/scripts/file-size-baseline.json b/scripts/file-size-baseline.json index e8055b9e..b3912c24 100644 --- a/scripts/file-size-baseline.json +++ b/scripts/file-size-baseline.json @@ -1,6 +1,6 @@ { "app/routes/inspection-edit.tsx": 2474, - "server/services/inspection/inspection-core.service.ts": 1083, + "server/services/inspection/inspection-core.service.ts": 1097, "app/routes/inspection-hub.tsx": 1048, "server/services/booking.service.ts": 967, "server/services/inspection/inspection-report.service.ts": 910, @@ -11,7 +11,7 @@ "server/api/sms.ts": 824, "app/components/portal/sections/ReportView.tsx": 780, "app/routes/settings-communication.tsx": 776, - "server/services/inspection.service.ts": 747, + "server/services/inspection.service.ts": 752, "app/routes/settings-communication-templates.tsx": 724, "app/routes/template-edit.tsx": 719, "app/components/media-studio/PhotoAnnotator.tsx": 692, @@ -42,7 +42,7 @@ "app/routes/settings-profile.tsx": 505, "server/services/inspection-request.service.ts": 501, "server/services/report-export-consumer.ts": 499, - "server/api/repair-builder.ts": 496, + "server/api/repair-builder.ts": 500, "app/routes/inspection-edit/action.server.ts": 496, "app/components/collab/VersionHistoryPanel.tsx": 478, "app/routes/settings-workspace.tsx": 477, diff --git a/server/api/repair-builder.ts b/server/api/repair-builder.ts index e66a0850..c2948083 100644 --- a/server/api/repair-builder.ts +++ b/server/api/repair-builder.ts @@ -273,6 +273,10 @@ const repairBuilderRoutes = createApiRouter() const access = await resolveBuilderAccess(c, id); if (!access) return c.json({ success: false as const, error: { code: 'UNAUTHORIZED', message: 'No access.' } }, 401); const { tenantId, creator } = access; + // IA-35 / IA-73 — creating a list is a write; a read-only agent is refused. + if (access.accessLevel !== 'readwrite') { + return c.json({ success: false as const, error: { code: 'FORBIDDEN', message: 'Read-only access to the repair list.' } }, 403); + } const gateResult = await runBuilderGate(c, id, tenantId); if (gateResult) return gateResult; @@ -315,7 +319,7 @@ const repairBuilderRoutes = createApiRouter() if (gateResult) return gateResult; // I1: pass inspectionId so assertCanEdit rejects RRs from a different inspection. - const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator); + const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator, access.accessLevel); if (guardResult) return guardResult; // Map Zod-output (undefined optional) to service ItemInput (null optional) @@ -344,7 +348,7 @@ const repairBuilderRoutes = createApiRouter() if (gateResult) return gateResult; // I1: pass inspectionId so assertCanEdit rejects RRs from a different inspection. - const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator); + const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator, access.accessLevel); if (guardResult) return guardResult; // Map Zod-output optional fields to service patch type (null not undefined). @@ -369,7 +373,7 @@ const repairBuilderRoutes = createApiRouter() if (gateResult) return gateResult; // I1: pass inspectionId so assertCanEdit rejects RRs from a different inspection. - const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator); + const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator, access.accessLevel); if (guardResult) return guardResult; // I1: pass inspectionId so the service guards against cross-inspection deletes. @@ -390,7 +394,7 @@ const repairBuilderRoutes = createApiRouter() if (gateResult) return gateResult; // I1: pass inspectionId so assertCanEdit rejects RRs from a different inspection. - const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator); + const guardResult = await runAssertCanEdit(c, tenantId, id, rrId, creator, access.accessLevel); if (guardResult) return guardResult; // I1: pass inspectionId so the service guards against cross-inspection writes. diff --git a/server/lib/db/schema/tenant/core.ts b/server/lib/db/schema/tenant/core.ts index b86e32db..c88de7ee 100644 --- a/server/lib/db/schema/tenant/core.ts +++ b/server/lib/db/schema/tenant/core.ts @@ -96,7 +96,7 @@ export const tenantConfigs = sqliteTable('tenant_configs', { // Workflow shortcuts PR — { cloneDefault, autoAdvanceDelayMs, pinnedTagIds } // Nullable; server applies hard-coded defaults when NULL. inspectionPrefs: text('inspection_prefs', { mode: 'json' }) - .$type<{ cloneDefault: 'rating' | 'rating_notes' | 'all'; autoAdvanceDelayMs: number; pinnedTagIds: string[] }>(), + .$type<{ cloneDefault: 'rating' | 'rating_notes' | 'all'; autoAdvanceDelayMs: number; pinnedTagIds: string[]; agentRepairAccess?: 'off' | 'read' | 'readwrite' }>(), // Sprint 2 S2-4 — when true, published reports render the per-defect // "Estimated cost: $X – $Y" badge. showEstimates: integer('is_estimates_shown', { mode: 'boolean' }).notNull().default(false), diff --git a/server/lib/repair-access.ts b/server/lib/repair-access.ts index 1698f75e..3daf1723 100644 --- a/server/lib/repair-access.ts +++ b/server/lib/repair-access.ts @@ -24,12 +24,25 @@ import type { HonoConfig } from '../types/hono'; * authenticated via a logged-in agent-portal session JWT * inspector → userId from the verified owner-preview JWT */ +/** Whether the resolved actor may read only, or read and write. */ +export type BuilderAccessLevel = 'read' | 'readwrite'; + export async function resolveBuilderAccess( c: Context, id: string, -): Promise<{ tenantId: string; creator: Creator; ownerPreview: boolean } | null> { +): Promise<{ tenantId: string; creator: Creator; ownerPreview: boolean; accessLevel: BuilderAccessLevel } | null> { const token = c.req.query('token'); + // Whether/how an agent may act on the repair list is a tenant policy + // (IA-35 / IA-73): off → no access at all, read → view only (writes 403), + // readwrite → full. Both agent tracks (portal token and agent session) go + // through here, so they can never disagree. client / inspector actors are + // always readwrite. + const agentLevel = async (tenantId: string): Promise => { + const setting = await c.var.services.inspection.getAgentRepairAccess(tenantId); + return setting === 'off' ? null : setting; + }; + // Path 1: persistent portal token. The grant's role kind decides the actor — // assuming 'client' for every resolvable token let an agent-kind token // (buyer_agent / listing_agent) act as the client on the builder's five @@ -40,12 +53,12 @@ export async function resolveBuilderAccess( if (grant) { const kind = await c.var.services.portalAccess.getRoleKind(grant.tenantId, grant.role); if (kind === 'client') { - const creator: Creator = { kind: 'client', ref: grant.recipientEmail }; - return { tenantId: grant.tenantId, creator, ownerPreview: false }; + return { tenantId: grant.tenantId, creator: { kind: 'client', ref: grant.recipientEmail }, ownerPreview: false, accessLevel: 'readwrite' }; } if (kind === 'agent') { - const creator: Creator = { kind: 'agent', ref: grant.recipientEmail }; - return { tenantId: grant.tenantId, creator, ownerPreview: false }; + const accessLevel = await agentLevel(grant.tenantId); + if (!accessLevel) return null; + return { tenantId: grant.tenantId, creator: { kind: 'agent', ref: grant.recipientEmail }, ownerPreview: false, accessLevel }; } return null; } @@ -54,16 +67,16 @@ export async function resolveBuilderAccess( if (token) { const legacy = await c.var.services.inspection.resolveAgentViewToken(token); if (legacy && legacy.inspectionId === id) { - const creator: Creator = { kind: 'agent', ref: token }; - return { tenantId: legacy.tenantId, creator, ownerPreview: false }; + const accessLevel = await agentLevel(legacy.tenantId); + if (!accessLevel) return null; + return { tenantId: legacy.tenantId, creator: { kind: 'agent', ref: token }, ownerPreview: false, accessLevel }; } } // Path 3: owner-preview via session Bearer JWT (tenant user / inspector). const ownerFull = await resolveOwnerPreviewFull(c); if (ownerFull) { - const creator: Creator = { kind: 'inspector', ref: ownerFull.userId }; - return { tenantId: ownerFull.tenantId, creator, ownerPreview: true }; + return { tenantId: ownerFull.tenantId, creator: { kind: 'inspector', ref: ownerFull.userId }, ownerPreview: true, accessLevel: 'readwrite' }; } // Path 4: logged-in agent-portal session JWT (tokenless dashboard link). @@ -73,10 +86,11 @@ export async function resolveBuilderAccess( // inspection row, never from the URL `:tenant` segment. const agentSession = await resolveAgentSession(c); if (agentSession) { - const access = await c.var.services.agent.accessToInspection(agentSession.userId, id); - if (access) { - const creator: Creator = { kind: 'agent', ref: agentSession.userId }; - return { tenantId: access.tenantId, creator, ownerPreview: false }; + const assoc = await c.var.services.agent.accessToInspection(agentSession.userId, id); + if (assoc) { + const accessLevel = await agentLevel(assoc.tenantId); + if (!accessLevel) return null; + return { tenantId: assoc.tenantId, creator: { kind: 'agent', ref: agentSession.userId }, ownerPreview: false, accessLevel }; } } diff --git a/server/lib/repair-gates.ts b/server/lib/repair-gates.ts index f21958a3..7e467bf0 100644 --- a/server/lib/repair-gates.ts +++ b/server/lib/repair-gates.ts @@ -67,7 +67,14 @@ export async function runAssertCanEdit( inspectionId: string, rrId: string, creator: import('../services/repair-request.service').Creator, + accessLevel: 'read' | 'readwrite' = 'readwrite', ): Promise { + // IA-35 / IA-73 — a read-only actor (agent under the tenant's `read` + // policy) may see the list but not mutate it. Refuse before the ownership + // check so a read-only agent gets a clean 403, not a "not the creator" one. + if (accessLevel !== 'readwrite') { + return c.json({ success: false as const, error: { code: 'FORBIDDEN', message: 'Read-only access to the repair list.' } }, 403); + } try { await c.var.services.repairRequest.assertCanEdit(tenantId, inspectionId, rrId, creator); return null; diff --git a/server/lib/validations/inspection-prefs.schema.ts b/server/lib/validations/inspection-prefs.schema.ts index 866aa33f..a6a0caf7 100644 --- a/server/lib/validations/inspection-prefs.schema.ts +++ b/server/lib/validations/inspection-prefs.schema.ts @@ -20,6 +20,12 @@ export const InspectionPrefsSchema = z.object({ * `tenant_configs.require_defect_fields` column (the readiness service * reads it directly). Default LOOSE — gaps warn, not block. */ requireDefectFields: z.enum(['none', 'location', 'trade', 'both']).default('none'), + /** IA-35 / IA-73 — whether agents may act on the repair request list. + * `off` = no access, `read` = view only (write endpoints refuse), + * `readwrite` = full. Default `readwrite`: account-track agents already + * get a builder link, so defaulting off would remove a shipped ability — + * the fix here is stopping the identity impersonation, not the access. */ + agentRepairAccess: z.enum(['off', 'read', 'readwrite']).default('readwrite'), }).openapi('InspectionPrefs'); export type InspectionPrefs = z.infer; @@ -33,6 +39,7 @@ export const DEFAULT_INSPECTION_PREFS: InspectionPrefs = { autoAdvanceDelayMs: 200, pinnedTagIds: [], requireDefectFields: 'none', + agentRepairAccess: 'readwrite', }; /** Merge a possibly-partial DB row with the defaults. */ diff --git a/server/services/inspection.service.ts b/server/services/inspection.service.ts index b51a489f..53578dc7 100644 --- a/server/services/inspection.service.ts +++ b/server/services/inspection.service.ts @@ -1,10 +1,10 @@ -import { type CoverCrop, type PhotoCrop } from '../lib/validations/inspection.schema'; -import { ScopedDB } from '../lib/db/scoped'; +import type { CoverCrop, PhotoCrop } from '../lib/validations/inspection.schema'; +import type { ScopedDB } from '../lib/db/scoped'; import type { AgreementService } from './agreement.service'; -import { type ReportMediaContext } from '../lib/report-video'; -import { type ImagesBinding } from '../lib/media/strip-exif'; -import { type PdfSettings } from '../lib/pdf-settings'; -import { type DeviationInput } from '../lib/pca-deviations'; +import type { ReportMediaContext } from '../lib/report-video'; +import type { ImagesBinding } from '../lib/media/strip-exif'; +import type { PdfSettings } from '../lib/pdf-settings'; +import type { DeviationInput } from '../lib/pca-deviations'; // Module-level types, constants, and pure helpers now live in // ./inspection/shared.ts (single source of truth shared by the facade + every @@ -554,6 +554,11 @@ export class InspectionService { return this.analytics.getRepairList(inspectionId, tenantId); } + /** IA-35 / IA-73 — tenant policy for agent access to the repair list. */ + async getAgentRepairAccess(tenantId: string): Promise<'off' | 'read' | 'readwrite'> { + return this.core.getAgentRepairAccess(tenantId); + } + /** * Returns tab counts for the inspection list UI. * Single query with 6 conditional aggregates to avoid N+1. diff --git a/server/services/inspection/inspection-core.service.ts b/server/services/inspection/inspection-core.service.ts index ad9a45f2..964002f7 100644 --- a/server/services/inspection/inspection-core.service.ts +++ b/server/services/inspection/inspection-core.service.ts @@ -82,6 +82,20 @@ export class InspectionCoreService extends InspectionSubService { * to `tenantId`. Throws BadRequest for the first id that fails — preventing * cross-tenant contact/agent references from being persisted (D1 does not * enforce FK constraints at runtime, so this is the application-layer gate). + * IA-35 / IA-73 — the tenant's policy for whether agents may act on the + * repair request list (`off` / `read` / `readwrite`). Stored in the + * inspectionPrefs JSON; absent → `readwrite` (see the schema default). + */ + async getAgentRepairAccess(tenantId: string): Promise<'off' | 'read' | 'readwrite'> { + const db = this.getDrizzle(); + const row = await db.select({ prefs: tenantConfigs.inspectionPrefs }) + .from(tenantConfigs) + .where(eq(tenantConfigs.tenantId, tenantId)) + .get(); + return row?.prefs?.agentRepairAccess ?? 'readwrite'; + } + + /** * Called in createInspection before the inspections insert. */ private async assertContactsBelongToTenant(tenantId: string, ids: Array) { diff --git a/tests/unit/helpers/repair-builder-routes-harness.ts b/tests/unit/helpers/repair-builder-routes-harness.ts index d2098df4..eebb777a 100644 --- a/tests/unit/helpers/repair-builder-routes-harness.ts +++ b/tests/unit/helpers/repair-builder-routes-harness.ts @@ -85,6 +85,7 @@ export function makeServices(overrides: { assertCanEdit?: ReturnType; accessToInspection?: ReturnType; portalAccessGetRoleKind?: ReturnType; + getAgentRepairAccess?: ReturnType; } = {}) { const defaultPortalToken = vi.fn().mockResolvedValue(null); const defaultAgent = vi.fn().mockResolvedValue(null); @@ -101,6 +102,8 @@ export function makeServices(overrides: { inspection: { resolveAgentViewToken: overrides.resolveAgentViewToken ?? defaultAgent, getRepairList: overrides.getRepairList ?? defaultRepairList, + // IA-35 / IA-73 — tenant agent-repair policy; default readwrite. + getAgentRepairAccess: overrides.getAgentRepairAccess ?? vi.fn().mockResolvedValue('readwrite'), }, agent: { accessToInspection: overrides.accessToInspection ?? vi.fn().mockResolvedValue(null), diff --git a/tests/unit/repair/repair-builder-actor-kind.spec.ts b/tests/unit/repair/repair-builder-actor-kind.spec.ts index fdc48287..09e295ac 100644 --- a/tests/unit/repair/repair-builder-actor-kind.spec.ts +++ b/tests/unit/repair/repair-builder-actor-kind.spec.ts @@ -85,3 +85,68 @@ describe('resolveBuilderAccess — actor kind from the grant role (IA-35)', () = expect(tokenCreator.kind).toBe(sessionCreator.kind); }); }); + +describe('agentRepairAccess switch (IA-73)', () => { + const agentToken = { ...VALID_TOKEN_ROW, role: 'buyer_agent', recipientEmail: 'agent@example.com' }; + + function agentServices(setting: 'off' | 'read' | 'readwrite') { + return makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue(agentToken), + portalAccessGetRoleKind: vi.fn().mockResolvedValue('agent'), + getAgentRepairAccess: vi.fn().mockResolvedValue(setting), + }); + } + + it('off — the agent is refused entirely (read AND write)', async () => { + const { app } = buildApp({ services: agentServices('off'), reportStatus: 'published' }); + const read = await app.request('/api/public/repair-builder/t1/insp1/source?token=tok1'); + expect(read.status).toBe(401); + const write = await app.request('/api/public/repair-builder/t1/insp1', { method: 'POST' }); + expect(write.status).toBe(401); + }); + + it('read — the agent may read but every write endpoint is 403', async () => { + const { app } = buildApp({ services: agentServices('read'), reportStatus: 'published' }); + expect((await app.request('/api/public/repair-builder/t1/insp1/source?token=tok1')).status).toBe(200); + // create list + expect((await app.request('/api/public/repair-builder/t1/insp1?token=tok1', { method: 'POST' })).status).toBe(403); + // add item + const addItem = await app.request('/api/public/repair-builder/t1/insp1/lists/rr1/items?token=tok1', { + method: 'POST', headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ findingKey: 'canned:s1:item1:x', sectionTitle: 'Roof', itemLabel: 'Shingles' }), + }); + expect(addItem.status).toBe(403); + }); + + it('readwrite — the agent may read and write', async () => { + // Fresh app per request: makeTwoQueryDb keys off call-count, so a second + // request on the same app would read the wrong gate row. + const { app: readApp } = buildApp({ services: agentServices('readwrite'), reportStatus: 'published' }); + expect((await readApp.request('/api/public/repair-builder/t1/insp1/source?token=tok1')).status).toBe(200); + const { app: writeApp } = buildApp({ services: agentServices('readwrite'), reportStatus: 'published' }); + expect((await writeApp.request('/api/public/repair-builder/t1/insp1?token=tok1', { method: 'POST' })).status).toBe(200); + }); + + it('parity — the token track and the session track get the same action set for the same setting', async () => { + for (const setting of ['off', 'read', 'readwrite'] as const) { + // Token track. + const { app: tokenApp } = buildApp({ services: agentServices(setting), reportStatus: 'published' }); + const tokenRead = (await tokenApp.request('/api/public/repair-builder/t1/insp1/source?token=tok1')).status; + const tokenWrite = (await tokenApp.request('/api/public/repair-builder/t1/insp1?token=tok1', { method: 'POST' })).status; + + // Session track (no portal token; agent session resolves the inspection). + vi.mocked(resolveAgentSession).mockResolvedValue({ userId: 'agent-user-1' } as never); + const sessionSvc = makeServices({ + portalAccessResolveToken: vi.fn().mockResolvedValue(null), + accessToInspection: vi.fn().mockResolvedValue({ tenantId: 't1' }), + getAgentRepairAccess: vi.fn().mockResolvedValue(setting), + }); + const { app: sessionApp } = buildApp({ services: sessionSvc, reportStatus: 'published' }); + const sessionRead = (await sessionApp.request('/api/public/repair-builder/t1/insp1/source')).status; + const sessionWrite = (await sessionApp.request('/api/public/repair-builder/t1/insp1', { method: 'POST' })).status; + + expect({ read: tokenRead, write: tokenWrite }).toEqual({ read: sessionRead, write: sessionWrite }); + } + vi.mocked(resolveAgentSession).mockResolvedValue(null as never); + }); +}); From 6a68ce5f70c1e50a5d64342695a5053526705835 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 05:49:19 +0800 Subject: [PATCH 11/35] feat(portal): revoke report access on removal, expire links on unpublish (IA-36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit revokeForRecipient and setExpiryForInspection were implemented and tested but had no API caller, so a delivered report link stayed live forever and unpublishing retracted the report while leaving its links working. Wire both cascades at the route layer (services stay decoupled): - Removing a person from an inspection now revokes that recipient's report-access token. removePerson returns the removed email so the handler can revoke; revokedAt already beats any expiry in both resolution paths, so the link is dead regardless of the report-link TTL. - Unpublishing expires every access token for the inspection immediately — no live URL should point at a withdrawn report. The People-card Remove button gains a confirmation (no window.confirm) that names the side effect — "Their report link stops working immediately" — and is relabeled "Remove from inspection", because a silent access revocation on an already-ambiguous button is exactly what the audit flagged. This is the security core of IA-36 (the registry's "most-worth-fixing half": existing capability, only the call sites + the confirming UI were missing). The fuller lifecycle — Reset/rotate, Make-primary, per-recipient status, a tenant report-link TTL, an expiry landing page — remains as follow-up. --- .../inspection/PeopleEditor.test.tsx | 3 +- app/components/inspection/PeopleEditor.tsx | 30 +++++- messages/en/inspections.json | 5 +- server/api/inspections/people.ts | 6 +- server/api/inspections/publish.ts | 5 + server/services/people.service.ts | 17 +++- .../inspections/report-access-revoke.spec.ts | 94 +++++++++++++++++++ .../unit/people/inspection-people-api.spec.ts | 6 +- tests/unit/people/people.service.spec.ts | 10 ++ .../reports/report-review-endpoints.spec.ts | 2 + 10 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 tests/unit/inspections/report-access-revoke.spec.ts diff --git a/app/components/inspection/PeopleEditor.test.tsx b/app/components/inspection/PeopleEditor.test.tsx index 61670b95..dcfc117c 100644 --- a/app/components/inspection/PeopleEditor.test.tsx +++ b/app/components/inspection/PeopleEditor.test.tsx @@ -116,7 +116,8 @@ describe("PeopleEditor", () => { ); expect(getByText("Primary")).toBeTruthy(); // Exactly one Remove control — the agent row, not the primary client. - const removeButtons = queryAllByText("Remove"); + // (IA-36 renamed the row button to "Remove from inspection".) + const removeButtons = queryAllByText("Remove from inspection"); expect(removeButtons).toHaveLength(1); expect(removeButtons[0].closest("div")?.textContent).toContain("Amy Agent"); }); diff --git a/app/components/inspection/PeopleEditor.tsx b/app/components/inspection/PeopleEditor.tsx index 01cb0c22..19147293 100644 --- a/app/components/inspection/PeopleEditor.tsx +++ b/app/components/inspection/PeopleEditor.tsx @@ -1,6 +1,6 @@ import { useEffect, useState } from "react"; import { Link, useFetcher } from "react-router"; -import { Card, Pill, Button, EmptyState } from "@core/shared-ui"; +import { Card, Pill, Button, EmptyState, Modal } from "@core/shared-ui"; import type { action } from "~/routes/inspection-hub"; import type { RoleProfile } from "~/components/contacts/contacts-helpers"; import { AddPersonModal } from "./AddPersonModal"; @@ -78,8 +78,14 @@ export function PeopleEditor({ if (modalOpen && addSucceeded) setModalOpen(false); }, [modalOpen, addSucceeded]); + // Removing a person also revokes their report-access link (IA-36), so the + // action is confirmed — a silent side effect on an already-ambiguous button + // is exactly what the audit flagged. + const [removeTarget, setRemoveTarget] = useState(null); + function handleRemove(personId: string) { removeFetcher.submit({ intent: "person-remove", personId }, { method: "post" }); + setRemoveTarget(null); } const groups = GROUP_ORDER.map((kind) => ({ @@ -147,7 +153,7 @@ export function PeopleEditor({ {!isPrimary && ( + + + } + > +

+ {m.inspections_hub_people_remove_confirm({ name: removeTarget?.name ?? "" })} +

+ ); } diff --git a/messages/en/inspections.json b/messages/en/inspections.json index 7c7efedd..cc4d0659 100644 --- a/messages/en/inspections.json +++ b/messages/en/inspections.json @@ -54,7 +54,10 @@ "inspections_hub_people_submit": "Add", "inspections_hub_people_adding": "Adding…", "inspections_hub_people_primary": "Primary", - "inspections_hub_people_remove": "Remove", + "inspections_hub_people_remove": "Remove from inspection", + "inspections_hub_people_remove_title": "Remove from inspection?", + "inspections_hub_people_remove_confirm": "Remove {name} from this inspection? Their report link stops working immediately.", + "inspections_hub_people_remove_cta": "Remove", "inspections_hub_people_empty_title": "No people yet", "inspections_hub_people_empty_desc": "Add a client, agent, or other contact to this inspection.", "inspections_hub_people_modal_title": "Add a person", diff --git a/server/api/inspections/people.ts b/server/api/inspections/people.ts index 862ce362..531a9989 100644 --- a/server/api/inspections/people.ts +++ b/server/api/inspections/people.ts @@ -147,7 +147,11 @@ const peopleRoutes = createApiRouter() const tenantId = getTenantId(c); const { id, personId } = c.req.valid('param'); await assertInspectionOwned(getDrizzle(c), id, tenantId); - await c.var.services.people.removePerson(tenantId, id, personId); + const { email } = await c.var.services.people.removePerson(tenantId, id, personId); + // IA-36 — leaving the inspection stops the person's report link. Their + // access token is revoked (revokedAt beats any expiry, so the link is + // dead regardless of the report-link TTL). + if (email) await c.var.services.portalAccess.revokeForRecipient(tenantId, id, email); return c.json({ success: true as const }, 200); }); diff --git a/server/api/inspections/publish.ts b/server/api/inspections/publish.ts index 03173dda..60b568e6 100644 --- a/server/api/inspections/publish.ts +++ b/server/api/inspections/publish.ts @@ -508,6 +508,11 @@ const publishRoutes = createApiRouter() const { id } = c.req.valid('param'); const result = await runReportTransition(() => c.var.services.inspection.unpublishReport(id, tenantId), 'Failed to unpublish report'); if (!result.ok) return c.json({ success: false as const, error: { code: 'BAD_REQUEST', message: result.message } }, 400); + // IA-36 — unpublishing retracts the report, so the links that point at + // it must stop working too; otherwise a recipient keeps a live URL to a + // report the inspector has withdrawn. Expire all of this inspection's + // access tokens immediately. + await c.var.services.portalAccess.setExpiryForInspection(tenantId, id, Date.now()); return c.json({ success: true as const, data: { reportStatus: 'in_progress' } }, 200); }); diff --git a/server/services/people.service.ts b/server/services/people.service.ts index fe4283c6..574dadcb 100644 --- a/server/services/people.service.ts +++ b/server/services/people.service.ts @@ -61,7 +61,21 @@ export class PeopleService { }).onConflictDoNothing(); } - async removePerson(tenantId: string, inspectionId: string, inspectionPersonId: string): Promise { + async removePerson(tenantId: string, inspectionId: string, inspectionPersonId: string): Promise<{ email: string | null }> { + // Resolve the recipient email BEFORE the delete so the caller can revoke + // this person's report-access token (IA-36): removing someone from an + // inspection must stop their report link, which otherwise stays live + // forever. Returned rather than revoked here to keep PeopleService free + // of the portal-access dependency. + const row = await this.db.select({ email: contacts.email }) + .from(inspectionPeople) + .innerJoin(contacts, eq(inspectionPeople.contactId, contacts.id)) + .where(and( + eq(inspectionPeople.tenantId, tenantId), + eq(inspectionPeople.inspectionId, inspectionId), + eq(inspectionPeople.id, inspectionPersonId), + )) + .get(); // Scope the delete to the URL's inspection as well as the tenant — the // personId path segment is asserted to belong to `inspectionId`, so a // person row from a DIFFERENT inspection (same tenant) must not be @@ -72,6 +86,7 @@ export class PeopleService { eq(inspectionPeople.inspectionId, inspectionId), eq(inspectionPeople.id, inspectionPersonId), )); + return { email: row?.email ?? null }; } async listPeople(tenantId: string, inspectionId: string): Promise { diff --git a/tests/unit/inspections/report-access-revoke.spec.ts b/tests/unit/inspections/report-access-revoke.spec.ts new file mode 100644 index 00000000..cc025c23 --- /dev/null +++ b/tests/unit/inspections/report-access-revoke.spec.ts @@ -0,0 +1,94 @@ +/** + * IA-36 — report-access lifecycle cascades: + * - removing a person from an inspection revokes their report-access token + * (revokeForRecipient), so a removed recipient's link stops working; + * - unpublishing a report expires every token for the inspection + * (setExpiryForInspection), so no live link points at a withdrawn report. + * + * Both are wired at the route layer (the services stay decoupled), so these + * spy on the portal-access calls made by the DELETE-person and unpublish + * handlers. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { OpenAPIHono } from '@hono/zod-openapi'; +import { inspectionsRoutes } from '../../../server/api/inspections'; +import { AppError } from '../../../server/lib/errors'; +import type { HonoConfig } from '../../../server/types/hono'; + +vi.mock('drizzle-orm/d1', () => ({ drizzle: vi.fn() })); +import { drizzle as mockDrizzle } from 'drizzle-orm/d1'; + +const TENANT = '00000000-0000-0000-0000-000000000001'; +const INSP = '550e8400-e29b-41d4-a716-446655440000'; +const PERSON = 'ip-1'; + +let revokeForRecipient: ReturnType; +let setExpiryForInspection: ReturnType; +let removePerson: ReturnType; +let unpublishReport: ReturnType; + +function buildApp() { + revokeForRecipient = vi.fn().mockResolvedValue(undefined); + setExpiryForInspection = vi.fn().mockResolvedValue(undefined); + removePerson = vi.fn().mockResolvedValue({ email: 'buyer@example.com' }); + unpublishReport = vi.fn().mockResolvedValue(undefined); + + // assertInspectionOwned selects the inspection row — return one so the + // route treats it as owned by this tenant. + (mockDrizzle as unknown as ReturnType).mockReturnValue({ + select: () => ({ from: () => ({ where: () => ({ get: async () => ({ id: INSP }) }) }) }), + }); + + const app = new OpenAPIHono(); + app.use('*', async (c, next) => { + c.set('userRole', 'owner' as never); + c.set('tenantId', TENANT); + c.set('user', { sub: 'user-1' } as never); + c.set('services', { + people: { removePerson }, + portalAccess: { revokeForRecipient, setExpiryForInspection }, + inspection: { unpublishReport }, + } as never); + await next(); + }); + app.route('/api/inspections', inspectionsRoutes); + app.onError((err, c) => { + if (err instanceof AppError) return c.json({ success: false, error: { code: err.code, message: err.message } }, err.status as never); + throw err; + }); + return app; +} + +const ENV = { DB: {} } as never; + +describe('IA-36 report-access cascades', () => { + beforeEach(() => buildApp()); + + it('removing a person revokes their report-access token', async () => { + const app = buildApp(); + const res = await app.fetch(new Request(`https://x/api/inspections/${INSP}/people/${PERSON}`, { method: 'DELETE' }), ENV); + expect(res.status).toBe(200); + expect(removePerson).toHaveBeenCalledWith(TENANT, INSP, PERSON); + expect(revokeForRecipient).toHaveBeenCalledWith(TENANT, INSP, 'buyer@example.com'); + }); + + it('does not revoke when the removed row had no email (already-gone / no contact)', async () => { + const app = buildApp(); + removePerson.mockResolvedValueOnce({ email: null }); + const res = await app.fetch(new Request(`https://x/api/inspections/${INSP}/people/${PERSON}`, { method: 'DELETE' }), ENV); + expect(res.status).toBe(200); + expect(revokeForRecipient).not.toHaveBeenCalled(); + }); + + it('unpublishing expires every access token for the inspection', async () => { + const app = buildApp(); + const res = await app.fetch(new Request(`https://x/api/inspections/${INSP}/unpublish`, { method: 'POST' }), ENV); + expect(res.status).toBe(200); + expect(unpublishReport).toHaveBeenCalledWith(INSP, TENANT); + expect(setExpiryForInspection).toHaveBeenCalledTimes(1); + const [t, i, expiry] = setExpiryForInspection.mock.calls[0]; + expect(t).toBe(TENANT); + expect(i).toBe(INSP); + expect(typeof expiry).toBe('number'); // immediate expiry timestamp + }); +}); diff --git a/tests/unit/people/inspection-people-api.spec.ts b/tests/unit/people/inspection-people-api.spec.ts index db24a3ee..41d78554 100644 --- a/tests/unit/people/inspection-people-api.spec.ts +++ b/tests/unit/people/inspection-people-api.spec.ts @@ -37,7 +37,11 @@ function buildApp(userRole: string = 'owner') { app.use('*', async (c, next) => { c.set('userRole', userRole as HonoConfig['Variables']['userRole']); c.set('tenantId', TENANT_ID); - c.set('services', { people: new PeopleService({ DB: {} as D1Database } as unknown as { DB: D1Database }) } as unknown as HonoConfig['Variables']['services']); + c.set('services', { + people: new PeopleService({ DB: {} as D1Database } as unknown as { DB: D1Database }), + // IA-36 — removePerson now cascades to revoke the report link. + portalAccess: { revokeForRecipient: async () => undefined }, + } as unknown as HonoConfig['Variables']['services']); await next(); }); app.route('/api/inspections', peopleRoutes); diff --git a/tests/unit/people/people.service.spec.ts b/tests/unit/people/people.service.spec.ts index 5471979a..3f8f94ba 100644 --- a/tests/unit/people/people.service.spec.ts +++ b/tests/unit/people/people.service.spec.ts @@ -62,4 +62,14 @@ describe('PeopleService', () => { const id = await svc.contactIdForRole('t1', 'i1', 'buyer_agent'); expect(id).toBeNull(); }); + + // IA-36 — removePerson returns the removed recipient's email so the caller + // can revoke their report-access token. + it('removePerson returns the removed recipient email', async () => { + await svc.addPerson('t1', 'i1', 'c1', rp('client')); + const [row] = await svc.listPeople('t1', 'i1'); + const res = await svc.removePerson('t1', 'i1', row.id); + expect(res).toEqual({ email: 'b1@x.com' }); + expect(await svc.listPeople('t1', 'i1')).toHaveLength(0); + }); }); diff --git a/tests/unit/reports/report-review-endpoints.spec.ts b/tests/unit/reports/report-review-endpoints.spec.ts index 6d396666..f1494ac0 100644 --- a/tests/unit/reports/report-review-endpoints.spec.ts +++ b/tests/unit/reports/report-review-endpoints.spec.ts @@ -77,6 +77,8 @@ function buildApp( // Stubs for other services the router may touch: reportVersion: { snapshotOnPublish: vi.fn().mockResolvedValue({ versionNumber: 1 }) }, reportPdf: { isPipelineEnabled: vi.fn().mockResolvedValue(false) }, + // IA-36 — unpublish expires the inspection's access tokens. + portalAccess: { setExpiryForInspection: vi.fn().mockResolvedValue(undefined) }, } as unknown as HonoConfig['Variables']['services']); await next(); }); From 9fab550027462f84388587c77706f8eb4f5ca87b Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 06:26:05 +0800 Subject: [PATCH 12/35] fix(command-palette): mount it workspace-wide so Cmd/Ctrl+K works everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CommandPalette (and its global Cmd/Ctrl+K listener) was mounted only on /inspections, so the shortcut did nothing on /calendar, /settings/*, or anywhere else in the authenticated workspace — the product's strongest discoverability surface existed on one page (IA-49). Hoist the single mount to auth-layout, the workspace shell that wraps every non-fullscreen page. The own-chrome routes (the hub, the editors) sit outside auth-layout and keep their own key handling. --- app/routes/auth-layout.tsx | 8 +++++++- app/routes/inspections.tsx | 7 +++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/routes/auth-layout.tsx b/app/routes/auth-layout.tsx index d940a1af..c8caae55 100644 --- a/app/routes/auth-layout.tsx +++ b/app/routes/auth-layout.tsx @@ -1,8 +1,9 @@ import { useEffect, useState } from "react"; -import { Outlet, useLoaderData, useLocation, useNavigation } from "react-router"; +import { Outlet, useLoaderData, useLocation, useNavigate, useNavigation } from "react-router"; import type { Route } from "./+types/auth-layout"; import { requireToken } from "~/lib/session.server"; import { createApi } from "~/lib/api-client.server"; +import { CommandPalette } from "~/components/CommandPalette"; import { Sidebar, MobileHeader } from "~/components/Sidebar"; import { RouteSkeleton } from "~/components/RouteSkeleton"; import type { SessionContext } from "~/hooks/useSessionContext"; @@ -49,6 +50,7 @@ export default function AuthLayout() { const { context } = useLoaderData(); const navigation = useNavigation(); const location = useLocation(); + const navigate = useNavigate(); // Show a content-pane skeleton only during a *real* page navigation: // - navigation.state === "loading" (loader in flight, not a form submission) @@ -90,6 +92,10 @@ export default function AuthLayout() {
+ + {/* IA-49 — mounted at the workspace layout (not just /inspections) so the + global Cmd/Ctrl+K command palette works on every authenticated page. */} + navigate("/inspections/new")} /> ); } diff --git a/app/routes/inspections.tsx b/app/routes/inspections.tsx index 610e5fe3..eee10089 100644 --- a/app/routes/inspections.tsx +++ b/app/routes/inspections.tsx @@ -5,9 +5,8 @@ import type { Route } from "./+types/inspections"; import { requireToken } from "~/lib/session.server"; import { createApi } from "~/lib/api-client.server"; import { buildCreateInspectionJson } from "~/lib/inspection-create"; -import { type WizardTeamMember } from "~/components/NewInspectionWizard"; +import type { WizardTeamMember } from "~/components/NewInspectionWizard"; import { OnboardingChecklist } from "~/components/dashboard/OnboardingChecklist"; -import { CommandPalette } from "~/components/CommandPalette"; import { SeatBanner } from "~/components/SeatBanner"; import { QuotaBanner } from "~/components/QuotaBanner"; import { useSessionContext } from "~/hooks/useSessionContext"; @@ -867,8 +866,8 @@ export default function InspectionsPage() { })() )} - {/* Command Palette */} - navigate("/inspections/new")} /> + {/* Command Palette is mounted once at auth-layout (IA-49) so Cmd/Ctrl+K + works workspace-wide, not only on this page. */} {/* Filters drawer */} Date: Fri, 24 Jul 2026 06:49:02 +0800 Subject: [PATCH 13/35] fix(sidebar): wire the search button to open the command palette (IA-38) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sidebar's search button carried an "open command palette" aria-label but had no onClick, so its most visible entry point did nothing — a dead control is worse for trust than no control. Lift the palette's open state to auth-layout, expose it through a CommandPaletteContext, and point the button at it. CommandPalette becomes controlled (open / onOpenChange); its Cmd/Ctrl+K listener reads the current open through a ref so the once-registered handler still toggles correctly. Only IA-49's hoist to auth-layout made this reachable from every page in the first place. --- app/components/CommandPalette.tsx | 34 ++++++++++++++++++++++++++----- app/components/Sidebar.tsx | 3 +++ app/routes/auth-layout.tsx | 24 ++++++++++++++++------ 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/app/components/CommandPalette.tsx b/app/components/CommandPalette.tsx index 7cd4935f..c66019eb 100644 --- a/app/components/CommandPalette.tsx +++ b/app/components/CommandPalette.tsx @@ -1,8 +1,20 @@ -import { useState, useEffect, useRef, useCallback, useMemo } from "react"; +import { useState, useEffect, useRef, useCallback, useMemo, createContext, useContext } from "react"; import { useNavigate, useFetcher } from "react-router"; import { useSessionContext } from "~/hooks/useSessionContext"; import { m } from "~/paraglide/messages"; +/** + * Lets any workspace surface (the sidebar search button, MobileHeader) open the + * command palette. The provider (auth-layout) owns the open state; consumers + * call `openPalette()`. Default is a no-op so a stray consumer outside the + * provider fails silently rather than throwing. + */ +const CommandPaletteContext = createContext<{ openPalette: () => void }>({ openPalette: () => {} }); +export function useCommandPalette() { + return useContext(CommandPaletteContext); +} +export const CommandPaletteProvider = CommandPaletteContext.Provider; + /* ------------------------------------------------------------------ */ /* Types */ /* ------------------------------------------------------------------ */ @@ -135,8 +147,20 @@ function PaletteIcon({ type }: { type: string }) { /* Component */ /* ------------------------------------------------------------------ */ -export function CommandPalette({ onNewInspection }: { onNewInspection?: () => void }) { - const [open, setOpen] = useState(false); +export function CommandPalette({ + open, + onOpenChange, + onNewInspection, +}: { + open: boolean; + onOpenChange: (open: boolean) => void; + onNewInspection?: () => void; +}) { + const setOpen = onOpenChange; + // The Cmd/Ctrl+K listener registers once (empty deps) but must toggle the + // CURRENT open state — read it through a ref so the closure never goes stale. + const openRef = useRef(open); + openRef.current = open; const [query, setQuery] = useState(""); const [activeIdx, setActiveIdx] = useState(0); const inputRef = useRef(null); @@ -171,7 +195,7 @@ export function CommandPalette({ onNewInspection }: { onNewInspection?: () => vo function handleKeyDown(e: KeyboardEvent) { if ((e.metaKey || e.ctrlKey) && e.key === "k") { e.preventDefault(); - setOpen((prev) => !prev); + setOpen(!openRef.current); setQuery(""); setActiveIdx(0); } @@ -179,7 +203,7 @@ export function CommandPalette({ onNewInspection }: { onNewInspection?: () => vo } window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); - }, []); + }, [setOpen]); // Focus input when opened; lazy-load recent inspections via BFF resource route useEffect(() => { diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index 50f6bf24..6ca276fb 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -6,6 +6,7 @@ import { IC, WORKSPACE_ITEMS } from "~/components/sidebar/nav-items"; import { SidebarGroup } from "~/components/sidebar/SidebarGroup"; import { UserMenuPopover } from "~/components/sidebar/UserMenuPopover"; import { MobileHeader } from "~/components/sidebar/MobileHeader"; +import { useCommandPalette } from "~/components/CommandPalette"; import { Avatar } from "@core/shared-ui"; import { m } from "~/paraglide/messages"; @@ -22,6 +23,7 @@ export function Sidebar() { const [userMenuOpen, setUserMenuOpen] = useState(false); const userMenuRef = useRef(null); const ctx = useSessionContext(); + const { openPalette } = useCommandPalette(); const companyName = ctx?.branding?.companyName || "OpenInspection"; const logoUrl = ctx?.branding?.logoUrl || "/logo.svg"; @@ -70,6 +72,7 @@ export function Sidebar() {
+
); diff --git a/app/routes/resources/marketplace-install.test.tsx b/app/routes/resources/marketplace-install.test.tsx new file mode 100644 index 00000000..3caddfb0 --- /dev/null +++ b/app/routes/resources/marketplace-install.test.tsx @@ -0,0 +1,51 @@ +/** + * IA-39 — the marketplace-install resource route action forwards to the + * import endpoint via the token-relay API client and surfaces the new local + * template id (or a clean error) to the caller. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const importPost = vi.fn(); + +vi.mock("~/lib/session.server", () => ({ + requireToken: vi.fn().mockResolvedValue("tok"), +})); +vi.mock("~/lib/api-client.server", () => ({ + createApi: vi.fn(() => ({ marketplace: { ":id": { import: { $post: importPost } } } })), +})); + +import { action } from "./marketplace-install"; + +function post(fields: Record) { + const body = new URLSearchParams(fields); + const request = new Request("https://x/resources/marketplace-install", { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: body.toString(), + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return action({ request, params: {}, context: {} as any }); +} + +describe("marketplace-install action", () => { + beforeEach(() => importPost.mockReset()); + + it("imports the template and returns the new local id", async () => { + importPost.mockResolvedValue(new Response(JSON.stringify({ data: { localTemplateId: "tpl-local-1" } }), { status: 201 })); + const res = await post({ templateId: "mkt-1" }); + expect(importPost).toHaveBeenCalledWith({ param: { id: "mkt-1" } }); + expect(res).toEqual({ ok: true, localTemplateId: "tpl-local-1" }); + }); + + it("returns an error when the import endpoint fails", async () => { + importPost.mockResolvedValue(new Response("nope", { status: 500 })); + const res = await post({ templateId: "mkt-1" }); + expect(res).toMatchObject({ ok: false }); + }); + + it("rejects a missing template id without calling the endpoint", async () => { + const res = await post({}); + expect(importPost).not.toHaveBeenCalled(); + expect(res).toMatchObject({ ok: false }); + }); +}); diff --git a/app/routes/resources/marketplace-install.tsx b/app/routes/resources/marketplace-install.tsx new file mode 100644 index 00000000..51819b69 --- /dev/null +++ b/app/routes/resources/marketplace-install.tsx @@ -0,0 +1,31 @@ +/** + * IA-39 — BFF resource route for installing a marketplace template. + * + * action: POST /api/templates/marketplace/:id/import via the token-relay API + * client (never a raw client fetch — that would 401). Returns the new local + * template id so the caller can jump to it in the library. + */ +import type { Route } from "./+types/marketplace-install"; +import { requireToken } from "~/lib/session.server"; +import { createApi } from "~/lib/api-client.server"; + +export async function action({ request, context }: Route.ActionArgs) { + const token = await requireToken(context, request); + const form = await request.formData(); + const raw = form.get("templateId"); + const id = typeof raw === "string" ? raw : ""; + if (!id) return { ok: false as const, error: "Missing template id." }; + + const api = createApi(context, { token }); + // `import` lives at /:id/import on the marketplace mount; it isn't on the + // typed client surface, so reach it the same way the other action routes do. + const marketplace = api.marketplace as unknown as { + [":id"]: { import: { $post: (args: { param: { id: string } }) => Promise } }; + }; + const res = await marketplace[":id"].import.$post({ param: { id } }); + if (!res.ok) { + return { ok: false as const, error: "Couldn't install this template. Please try again." }; + } + const body = (await res.json()) as { data?: { localTemplateId?: string } }; + return { ok: true as const, localTemplateId: body.data?.localTemplateId ?? null }; +} diff --git a/messages/en/library.json b/messages/en/library.json index ed0b9443..a518b44f 100644 --- a/messages/en/library.json +++ b/messages/en/library.json @@ -124,6 +124,8 @@ "marketplace_empty_title": "Marketplace is empty", "marketplace_empty_desc": "Community templates and content packs will appear here.", "marketplace_install": "Install", + "marketplace_installing": "Installing…", + "marketplace_install_error": "Couldn't install this template. Please try again.", "notifications_meta_title": "Notifications - OpenInspection", "notifications_heading": "Notifications", "notifications_meta": "{count} notifications", From 6d906c7a474ae7fb2ab32e47e29ad203524eca06 Mon Sep 17 00:00:00 2001 From: important-new Date: Fri, 24 Jul 2026 08:14:46 +0800 Subject: [PATCH 15/35] feat(reports): surface report version history + fix the diff page (IA-40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The signed, immutable report-version system was fully built server-side but had no way in: the version-diff page had zero inbound links (only reachable by hand-typing the URL), the publish form never sent the `summary` the API already accepted, and — discovered during E2E — the diff page itself crashed on every real diff. - inspection-hub: the Report card now lists report versions (number, published date, Amendment badge, change summary), each amendment linking to a field-level diff against its predecessor via versionDiffHref(). The loader fetches the version list best-effort. - publish flow: an amendment (versionNumber > 1) now shows a "what changed" textarea in the publish modal and forwards it as `summary` to snapshotOnPublish, which already accepted it. - version-diff page: the loader/component expected a flat DiffEntry[] but the API returns { items, units }, so `data.map` threw on any non-empty diff — the page only ever "worked" when there were zero changes. flattenVersionDiff() now maps the real payload into the rendered rows (changed fields show before/after; added/removed items and units render a marker). Registry premise about the data layer being fully wired was wrong on the last point; corrected per code-over-docs. Verified in Chrome (light + dark). --- .../inspection-hub/PublishReportModal.tsx | 20 +++- .../inspection-hub-version-links.test.ts | 21 ++++ app/routes/inspection-hub.tsx | 97 ++++++++++++++++- app/routes/version-diff-flatten.test.ts | 47 ++++++++ app/routes/version-diff.tsx | 100 ++++++++++++++---- messages/en/inspections.json | 6 ++ messages/en/library.json | 2 + scripts/file-size-baseline.json | 8 +- 8 files changed, 271 insertions(+), 30 deletions(-) create mode 100644 app/routes/inspection-hub-version-links.test.ts create mode 100644 app/routes/version-diff-flatten.test.ts diff --git a/app/components/inspection-hub/PublishReportModal.tsx b/app/components/inspection-hub/PublishReportModal.tsx index bb55cb72..9f34f1f0 100644 --- a/app/components/inspection-hub/PublishReportModal.tsx +++ b/app/components/inspection-hub/PublishReportModal.tsx @@ -1,4 +1,4 @@ -import { useFetcher } from "react-router"; +import type { useFetcher } from "react-router"; import { Modal } from "@core/shared-ui"; import type { action } from "~/routes/inspection-hub"; import { m } from "~/paraglide/messages"; @@ -13,6 +13,7 @@ export function PublishReportModal({ open, agreementRequired, paymentRequired, + isAmendment, fetcher, submitting, error, @@ -21,6 +22,8 @@ export function PublishReportModal({ open: boolean; agreementRequired: boolean; paymentRequired: boolean; + /** IA-40 — this publish creates versionNumber > 1; ask what changed. */ + isAmendment: boolean; fetcher: ReturnType>; submitting: boolean; error: string | undefined; @@ -68,6 +71,21 @@ export function PublishReportModal({ defaultChecked={paymentRequired} /> + {isAmendment && ( +