fix(jobs)+feat(crm): job create/edit failures, restore dropped person edit/delete, and person archive

[corrin]

Four bodies of work on one branch. They're causally linked rather than merely bundled: fixing the E2E-masked job failures (1–2) meant running the suite, which surfaced a dropped feature (3), and restoring that properly turned into the person archive feature (4).

Suggested review order. §1–2 are small, self-contained, and already described below. §3 is a regression fix. §4 is the substantial one — start with the design spec, then the implementation plan.

---

1. Job create — create.vue swallowed create/navigate errors

handleSubmit's catch stashed form state to localStorage and called window.location.reload(), aborting the in-flight navigation and dropping the user on a blank create page with no error surfaced (the four statements after the reload were dead code). Split submit into two honest phases:

• create failure → toast.error(extractErrorMessage(error)), user can retry (no reload, no permanent-disable flag).
• navigate failure → the job already exists (201), so a router.push rejection reports "Job #N created but the page did not open", never as a creation failure.

Drops the localStorage round-trip and the hasCreationError flag (which permanently disabled the submit button with no reload left to reset it).

2. Job change-company — display-only person_name leaked into the delta

Changing a job's company cleared the linked person and the autosave delta included person_name, which the backend correctly rejects with 400 Unsupported field 'person_name' in delta payload — it's derived from job.person.name (read_only) and has no writable target. The rejected save meant waitForAutosave never resolved → 120s timeout in edit-job-settings › change company.

person_name legitimately blanks the displayed person via the optimistic patch; only the two wire-payload builders wrongly forwarded it. Fixed each at source:

• useJobHeaderAutosave saveAdapter — removed from allowedHeaderKeys and the company-change re-add; only person_id (the writable field) clears the person.
• JobSettingsTab saveAdapter — skips display-only keys when building the partial payload.

The backend's strict rejection (ADR 0004/0015/0017) remains the loud safety net; a regression test asserts a company-change save omits person_name from the wire.

3. Restore person Edit/Delete dropped by the people cutover (regression)

The "first-class people" cutover (5bf012f3) replaced ContactSelectionModal.vue with PersonSelectionModal.vue and silently dropped two of the three per-person hover actions. Production offers Select · Edit (pencil) · Delete (trash) plus a delete-confirmation overlay; this branch's modal offered Select only. Also lost: the edit-mode form, the primary-person warning, and the Update/Delete toasts.

All restored, with user-facing copy taken verbatim from production (only contact→person, client→company; no invented phrasing):

• usePersonManagement — startEditPerson / cancelEdit / updatePerson / deletePerson. updatePerson is change-detected and fans out across three endpoints, because the new model splits what production wrote in one call: identity (people_partial_update), company-link fields (people_company_links_update), and phone — now a ContactMethod row rather than a scalar (list → patch/create).
• PersonSelectionModal — Edit/Delete buttons, delete-confirm overlay, amber editing state, edit-mode title/submit label, stable automation ids.
• PersonSelector — wired emits, prod-aligned toasts.

All six production-editable fields (name, email, position, notes, is_primary, phone) are editable again — a full Feature Parity Inventory is in the plan doc.

4. Person archive (new feature)

Lets users retire departed people so they drop out of everyday views, reversibly, without hard-deleting. Person.is_active already existed as a soft-delete flag but no HTTP path could reach it.

The rule (is_active is the single source of truth — never computed from link counts):

• Removing a person's last active company link → archive. Removing one of several links leaves them active.
• Adding/restoring any company link → un-archive. This is the implicit un-archive; there's no separate button.
• Explicit Archive person on PersonDetail → deactivate all links + archive ("retire everywhere now").
• Active-but-company-less stays valid when it arises from creation without a link (unchanged). Only removal archives.

Backend — remove_company_link cascade; un-archive in put_company_link; the is_active=True gates relaxed on the person/link/contact-method lookups so archived people stay viewable and restorable; new POST /api/people/{id}/archive/; include_archived query on the directory; is_active exposed on PersonSummary.

Frontend — "Show archived" filter + Archived badge in /crm/people; Archived badge + "Archive person" button on PersonDetail; new e2e people-archive.spec.ts (archive → show-archived → restore).

For a single-company person, the job-settings Delete button now archives via the shared cascade; for a multi-company person it just unlinks. No change was needed there — it routes through the same service.

5. Guardrail: "No feature removal" (CLAUDE.md)

§3 happened because a rewrite dropped features and nobody checked. A memory-only note wouldn't fire — feature-drop happens exactly when nobody is thinking about the old thing. So this adds a repo-shared rule (loaded by every agent) requiring a Feature Parity Inventory before any component/page/model/endpoint rewrite: enumerate every capability the old version exposed, with a keep/drop/defer decision each; default keep, dropping needs sign-off. Plus docs/plans/_template.md making that inventory a required plan section.

---

Worth a reviewer's attention

• The archive cascade was got wrong once, on purpose-preserved history. The first attempt (in 913cd927) flipped is_active but left every endpoint gating on is_active=True — archived people 404'd and their links couldn't be restored, breaking people.spec.ts. Reverted in 00b2bce8, then redone properly across cff80804…e31b662f. The revert is kept rather than squashed because the reason matters.

• is_active was a lying type. ModelSerializer inferred required=False from the model's BooleanField(default=True), so the response schema marked it optional and the generated zod produced is_active: z.boolean().optional() — meaning !person.is_active would badge every row as archived if the field were ever absent. Declared explicitly so the contract says always-present (4db39590, ADR 0028). These are response-only serializers, so it's inert on input.

• A final whole-branch review caught two Criticals every green test missed (d4e4d780), because the unit layer mocks the very endpoint that broke:

  1. PersonContactMethodsView was still is_active-gated. PersonDetail loads person + contact-methods + links in one Promise.all, so that 404 rejected the whole load → archived person rendered "Failed to load person" with no restore button — the same unreachable state the revert above existed to prevent.
  2. classify_phone_ownership skipped archived people, turning the phone-conflict "Restore company link" recovery path into a dead end that rendered "It belongs to and cannot be assigned to a person." — a regression of pre-branch behaviour.

  Both contradicted an out-of-scope note in my own spec; the note was a mistake, not a tradeoff, and the spec was corrected (9601515f).

Verification

• Backend: apps.company 143/143 pass (new tests cover archive-on-last-link, un-archive-on-restore, archived-person detail/contact-methods reachability, archived phone-owner recovery, and the include_archived filter). scripts/check_mypy.sh clean (pre-push gate passed).
• Frontend: unit suites for the modal, usePersonManagement, people directory and PersonDetail pass; npm run type-check clean.
• E2E: not yet run — this is the gate that matters. The unit layer structurally cannot catch the bug class the final review found. Needs XERO_READONLY=True:

  npm run test:e2e -- tests/crm/people.spec.ts tests/crm/people-archive.spec.ts
  

  people.spec.ts › manages link lifecycle is the key regression check (it's what the bad cascade broke); people-archive.spec.ts is new. Also still pending from §1–2: edit-job-settings › change company returning 2xx and, past the maxFailures=1 abort, reaching phone-call-job-link.

Known follow-ups (non-blocking, deliberately deferred)

• No select_for_update on Person in remove_company_link / put_company_link — a real but narrow race (concurrent link writes on the same person) that self-heals on the next link write; archive_person does lock.
• loadPerson() on link actions reverts unsaved identity-form edits (pre-existing pattern, inherited).
• frontend/CLAUDE.md rule 5 misdescribes the schema regen: npm run update-schema only prettifies schema.yml + regenerates the client; the real regen is scripts/regen_openapi_schema.sh (run by pre-commit).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Prevented display-only person names from being saved via job header autosave.
  • Improved job header updates when changing companies, ensuring the correct identifiers are preserved.
  • Fixed People directory results so stale, out-of-order responses no longer overwrite the latest search.
• Improvements
  • Job submit button state is now more reliable (submission status + validation + successful creation).
  • Job creation now resets to clean defaults when opened.
  • Creation failures and “opened after creation” navigation issues are reported separately via notifications.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes exclude person_name from job header autosave payloads, improve job creation error handling and form reset behavior, and prevent stale people-directory responses from overwriting newer search results.

Changes

Job header autosave filtering

Layer / File(s)	Summary
Filter display-only header fields and verify company changes frontend/src/components/job/JobSettingsTab.vue, frontend/src/composables/useJobHeaderAutosave.ts, frontend/src/components/job/__tests__/*	Job person fields are hydrated from job data, person_name is excluded from partial and delta payloads, and company-change tests verify the resulting request. Related store mocks now include patchHeader.

Job creation submission flow

Layer / File(s)	Summary
Handle creation and navigation errors frontend/src/pages/jobs/create.vue	Creation validates success and job_id, reports creation failures without reloading, handles navigation failures separately, and updates submit-button state.
Reset creation form state frontend/src/pages/jobs/create.vue	Mount initialization clears prior form values and restores default estimate and pricing-methodology values.

People request ordering

Layer / File(s)	Summary
Ignore stale people responses frontend/src/pages/crm/people/(index).vue, frontend/src/pages/crm/people/__tests__/people-directory.test.ts	Request sequencing prevents older responses from replacing newer search results, with coverage for out-of-order responses.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• corrin/docketworks#453: Updates the People-management and JobSettingsTab person-field shape used by these autosave changes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is detailed, but it does not follow the required template and is missing the Jira work item and checklist sections.	Add the template sections, include the Jira key (for example KAN-123), and finish the required checklist items.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is specific and mostly matches the job-create/edit and CRM updates, but it also mentions person edit/delete and archive work not shown here.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/job-create-edit-masked-timeouts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

frontend/src/components/job/JobSettingsTab.vue (1)

1286-1289: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Avoid allocating a Set on every save.

Since person_name is currently the only display-only field, you can avoid allocating a Set on every save by checking the key directly. If you prefer to keep the Set for future extensibility, consider defining it outside of saveAdapter so it is only allocated once.

♻️ Proposed refactor

-      const DISPLAY_ONLY_JOB_FIELDS = new Set(['person_name'])
       const partialPayload: Record<string, unknown> = {}
       for (const [k, v] of Object.entries(patch)) {
-        if (DISPLAY_ONLY_JOB_FIELDS.has(k)) continue
+        if (k === 'person_name') continue
         partialPayload[k] = normalise(k, v)
       }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/components/job/JobSettingsTab.vue` around lines 1286 - 1289,
Update saveAdapter’s display-only field filtering to avoid creating
DISPLAY_ONLY_JOB_FIELDS on every save: directly skip the person_name key, or
move the constant Set outside saveAdapter so it is allocated once while
preserving the existing filtering behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/pages/jobs/create.vue`:
- Around line 481-501: Prevent duplicate submissions after jobService.createJob
succeeds by introducing or reusing a creation-success guard in the create-job
submission flow. Update the navigation-failure handling around router.push so
resetting isSubmitting does not make canSubmit true or allow the untouched form
to be submitted again; preserve the existing error toast and navigation behavior
while blocking retries regardless of navigation outcome.
- Around line 488-501: Update the navigation flow around router.push in the job
creation handler to inspect its resolved result with isNavigationFailure().
Treat resolved navigation failures like caught errors by logging the failure,
showing the existing toast, and resetting isSubmitting; retain the success path
for successful navigation and the existing catch handling for unexpected errors.

---

Nitpick comments:
In `@frontend/src/components/job/JobSettingsTab.vue`:
- Around line 1286-1289: Update saveAdapter’s display-only field filtering to
avoid creating DISPLAY_ONLY_JOB_FIELDS on every save: directly skip the
person_name key, or move the constant Set outside saveAdapter so it is allocated
once while preserving the existing filtering behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d8a8496-bee1-484f-8f80-087513cebf03

📥 Commits

Reviewing files that changed from the base of the PR and between 5f4ad21 and eecbdb1.

📒 Files selected for processing (4)

• frontend/src/components/job/JobSettingsTab.vue
• frontend/src/components/job/__tests__/JobSettingsTab.companyChange.test.ts
• frontend/src/composables/useJobHeaderAutosave.ts
• frontend/src/pages/jobs/create.vue

💤 Files with no reviewable changes (1)

• frontend/src/composables/useJobHeaderAutosave.ts

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

frontend/src/components/job/JobSettingsTab.vue (1)

1255-1260: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reconcile person_name after removing it from the payload.

Filtering makes the later touchedKeys.includes('person_name') branches unreachable. Person clears and company-change responses can therefore leave serverBaseline and jobsStore with the old name.

When person_id or company_id is touched, reconcile both person fields from serverJobDetail and include both in headerPatch; when clearing without response data, explicitly clear the name too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/components/job/JobSettingsTab.vue` around lines 1255 - 1260,
Update the patch reconciliation flow around DISPLAY_ONLY_JOB_FIELDS and
touchedKeys so person_id or company_id changes derive both person_id and
person_name from serverJobDetail and add both fields to headerPatch. When
person_id is cleared without response data, explicitly clear person_name as
well, ensuring serverBaseline and jobsStore receive the reconciled values
despite person_name being excluded from partialPayload.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/components/job/JobSettingsTab.vue`:
- Around line 848-852: In the hydration logic around localJobData and
serverBaseline, replace direct property mutations with immutable object
replacements while preserving the existing person_id and person_name fallback
values; update each ref’s value by spreading its current object and overriding
those fields, leaving personDisplayValue unchanged.

---

Outside diff comments:
In `@frontend/src/components/job/JobSettingsTab.vue`:
- Around line 1255-1260: Update the patch reconciliation flow around
DISPLAY_ONLY_JOB_FIELDS and touchedKeys so person_id or company_id changes
derive both person_id and person_name from serverJobDetail and add both fields
to headerPatch. When person_id is cleared without response data, explicitly
clear person_name as well, ensuring serverBaseline and jobsStore receive the
reconciled values despite person_name being excluded from partialPayload.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 98590c6d-3bcc-462b-8be2-0cd4a62a63ba

📥 Commits

Reviewing files that changed from the base of the PR and between eecbdb1 and 3882ca5.

📒 Files selected for processing (6)

• frontend/src/components/job/JobSettingsTab.vue
• frontend/src/components/job/__tests__/JobSettingsTab.labourRate.test.ts
• frontend/src/components/job/__tests__/JobSettingsTab.urgent.test.ts
• frontend/src/pages/crm/people/(index).vue
• frontend/src/pages/crm/people/__tests__/people-directory.test.ts
• frontend/src/pages/jobs/create.vue

🚧 Files skipped from review as they are similar to previous changes (1)

• frontend/src/pages/jobs/create.vue