diff --git a/CLAUDE.md b/CLAUDE.md index 447efa8c..82529fb3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -161,7 +161,7 @@ Strict 3-tier architecture. Primitives + shared-composed live in `src/design-sys **The Reuse Gate (soft).** Before adding a new *workflow surface*, search for an existing surface that already owns that capability and extend it. A parallel surface is allowed only with a written "Reuse analysis" justifying why (genuinely different interaction model, or isolation requirement). A hook (`scripts/hooks/reuse_surface_reminder.sh`) warns on new files under `src/components/`. -**Size.** Keep feature components under ~200 LOC; decompose past that. ⚠️ **Known debt — do not imitate:** `ReconstructedResume.tsx` (1338), `SectionRewrite.tsx` (607), `ModelSelector.tsx` (556), `ReconstructedRole.tsx` (483) all violate this. If you are editing one, prefer extracting your change into a new sibling over growing the file further. +**Size.** Keep feature components under ~200 LOC; decompose past that. ⚠️ **Known debt — do not imitate:** `ReconstructedResume.tsx` (1489), `SectionRewrite.tsx` (607), `ModelSelector.tsx` (556), `ReconstructedRole.tsx` (490) all violate this. If you are editing one, prefer extracting your change into a new sibling over growing the file further. ## Styling & tokens diff --git a/src/components/features/ExperienceSection.other-bullets.test.tsx b/src/components/features/ExperienceSection.other-bullets.test.tsx index 50b9cf89..d7e394ea 100644 --- a/src/components/features/ExperienceSection.other-bullets.test.tsx +++ b/src/components/features/ExperienceSection.other-bullets.test.tsx @@ -224,6 +224,9 @@ function Harness() { addedBullets: edit.addedBullets, addedExperience: edit.addedEntries.filter((e) => e.section === "experience"), originalCount: 1, + // Identity: no parsed entry is deleted here, so a render position IS its + // parsed index (#856). + parsedIndices: [0], onAddEntry: () => edit.addEntry("experience"), onRemoveEntry: edit.removeEntry, onEntryField: edit.setEntryField, diff --git a/src/components/features/ExperienceSection.prune-hold.test.tsx b/src/components/features/ExperienceSection.prune-hold.test.tsx index 9fef336f..3b3bdd9c 100644 --- a/src/components/features/ExperienceSection.prune-hold.test.tsx +++ b/src/components/features/ExperienceSection.prune-hold.test.tsx @@ -166,6 +166,9 @@ function Harness() { addedExperience: edit.addedEntries.filter((e) => e.section === "experience"), // Index 0 is the parsed role; indices 1+ are the user-added ones. originalCount: 1, + // Identity: no parsed entry is deleted here, so a render position IS its + // parsed index (#856). + parsedIndices: [0], onAddEntry: () => edit.addEntry("experience"), onRemoveEntry: edit.removeEntry, onEntryField: edit.setEntryField, diff --git a/src/components/features/ExperienceSection.test.tsx b/src/components/features/ExperienceSection.test.tsx index 39c67f55..2962abd9 100644 --- a/src/components/features/ExperienceSection.test.tsx +++ b/src/components/features/ExperienceSection.test.tsx @@ -109,6 +109,9 @@ function Harness() { addedBullets: {}, addedExperience: [], originalCount: EXPERIENCES.length, + // Identity: nothing is deleted in this harness, so a render position IS its + // parsed index (#856). + parsedIndices: EXPERIENCES.map((_, i) => i), onAddEntry: () => {}, onRemoveEntry: () => {}, onEntryField: () => {}, diff --git a/src/components/features/ReconstructedEducationSkills.test.ts b/src/components/features/ReconstructedEducationSkills.test.ts index e28a6b78..c3945133 100644 --- a/src/components/features/ReconstructedEducationSkills.test.ts +++ b/src/components/features/ReconstructedEducationSkills.test.ts @@ -125,6 +125,9 @@ describe("EducationSection date-row symmetry (issue 376)", () => { onEducationFieldChange: () => {}, addedEducation: [], originalCount: 1, + // Identity: nothing is deleted in this harness, so a render position IS + // its parsed index (#856). + parsedIndices: [0], onAddEntry: () => {}, onRemoveEntry: () => {}, onEntryField: () => {}, diff --git a/src/components/features/ReconstructedEducationSkills.tsx b/src/components/features/ReconstructedEducationSkills.tsx index 78d1400b..ecb5ca50 100644 --- a/src/components/features/ReconstructedEducationSkills.tsx +++ b/src/components/features/ReconstructedEducationSkills.tsx @@ -22,6 +22,7 @@ import type { AddedEntry, AddedEntryField, } from "../../hooks/useEditableParse.ts"; +import { parsedEntryKey } from "../../hooks/useEditableParse.ts"; import { buildEducationDates } from "../../lib/score/entry-dates.ts"; import { EditableField, SectionHeading } from "@design-system"; import { validateDate } from "../../lib/edit/field-validators.ts"; @@ -105,7 +106,8 @@ function EducationEntry({ edu: ResumeEducation; overrides: EducationFieldOverrides | undefined; onFieldChange: (field: keyof EducationFieldOverrides, value: string) => void; - /** Remove this entry (only set for user-ADDED entries). */ + /** Remove this entry. Set for a PARSED entry too since #856 — "is this + * user-added?" is {@link isAdded}, never this prop's presence. */ onRemove?: () => void; /** User-added entries carry no `field` (major) slot, so the major affordance * renders on PARSED entries only. */ @@ -200,6 +202,7 @@ export function EducationSection({ onEducationFieldChange, addedEducation, originalCount, + parsedIndices, onAddEntry, onRemoveEntry, onEntryField, @@ -209,6 +212,8 @@ export function EducationSection({ heading?: string; education: ResumeEducation[]; educationOverrides: Record; + /** `index` is the entry's PARSED index — the key space `educationOverrides` + * uses — not its render position (#856). */ onEducationFieldChange: ( index: number, field: keyof EducationFieldOverrides, @@ -216,10 +221,14 @@ export function EducationSection({ ) => void; /** User-added education entries, append-aligned to indices ≥ originalCount. */ addedEducation: AddedEntry[]; - /** Count of PARSED education entries; indices at/above this are user-added. */ + /** Count of PARSED education entries still rendered; indices at/above this + * are user-added. */ originalCount: number; + /** Render position → PARSED index for the surviving parsed entries (#856), + * from `survivingParsedIndices`. Identity until one is deleted. */ + parsedIndices: readonly number[]; onAddEntry: () => void; - onRemoveEntry: (id: string) => void; + onRemoveEntry: (key: string) => void; onEntryField: (id: string, field: AddedEntryField, value: string) => void; /** Drop a blank added entry when focus leaves the section (#379). */ onPruneEmpty: () => void; @@ -239,14 +248,22 @@ export function EducationSection({ i >= originalCount ? addedEducation[i - originalCount] : undefined; + // PARSED index, not the render position — see `parsedIndices`. + const parsedIdx = parsedIndices[i] ?? i; + const entryKey = added + ? added.id + : parsedEntryKey("education", parsedIdx); return ( { if (!added) { - onEducationFieldChange(i, field, value); + onEducationFieldChange(parsedIdx, field, value); return; } // Added entries carry no major slot; the `field` edit can't @@ -255,7 +272,10 @@ export function EducationSection({ if (field !== "field") onEntryField(added.id, EDUCATION_FIELD_MAP[field], value); }} - onRemove={added ? () => onRemoveEntry(added.id) : undefined} + // Education carries no bullets, so this is the one section whose + // delete is the bare `removeEntry` (#856) rather than the + // bullets-first `removeEntryWithBullets`. + onRemove={() => onRemoveEntry(entryKey)} isAdded={Boolean(added)} /> ); diff --git a/src/components/features/ReconstructedResume.remove-parsed-entry.test.tsx b/src/components/features/ReconstructedResume.remove-parsed-entry.test.tsx new file mode 100644 index 00000000..6e77d49e --- /dev/null +++ b/src/components/features/ReconstructedResume.remove-parsed-entry.test.tsx @@ -0,0 +1,244 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 The offlinecv Authors + +// @vitest-environment jsdom + +/** + * #856 — the rendered delete affordance on a PARSED entry, and the index it + * writes with. + * + * Two things can only be caught here, above the hook and below the pipeline: + * + * 1. The affordance EXISTS on a parsed entry. Before #856 every section gated + * its `RemoveButton` on the entry being user-added, which is the whole + * report: a phantom achievement could be blanked field by field but never + * dropped, and still shipped into the Download PDF. + * 2. The key that crosses the component boundary is the PARSED index, not the + * render position. `applyOverrides` filters a deleted entry out of the array + * this section maps over, so from the first deletion on the two diverge — + * and `achievementOverrides`, `descriptionOverrides` and the tombstone set + * are all keyed by the parsed one. Writing a render position into any of them + * rebinds a survivor's edits to its neighbour's, silently and plausibly. + * + * Achievements is the section the report came from, and the one where all three + * of those channels meet, so it is the one rendered. Spies rather than a live + * hook: the claim is about which key crosses the boundary, which a spy states + * directly and a re-graded pipeline only implies. + * + * jsdom + raw `createRoot`, matching `ExperienceSection.test.tsx`. + */ + +import { describe, expect, it, afterEach, beforeEach, vi } from "vitest"; +import { createElement } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { act } from "react"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = + true; + +import { AchievementsSection } from "./ReconstructedResume.tsx"; +import { survivingParsedIndices } from "../../hooks/useEditableParse.ts"; +import type { AddedEntry } from "../../hooks/useEditableParse.ts"; +import { bulletId } from "../../lib/score/bullet-id.ts"; +import type { BulletGroup } from "../../lib/score/group-bullets.ts"; +import type { BulletObservation } from "../../lib/score/score.ts"; +import type { HeuristicAchievement } from "../../lib/score/types.ts"; + +function bullet(index: number, text: string): BulletObservation { + return { + text, + id: bulletId(text, 0), + index, + hasMetric: true, + startsWithActionVerb: true, + wellFormedLength: true, + wordCount: 9, + }; +} + +const PARSED: readonly HeuristicAchievement[] = [ + { type: "Patent", title: "Phantom method", year: "2019" }, + { type: "Award", title: "Best Paper", year: "2020" }, + { type: "Talk", title: "Scaling parsers", year: "2021" }, +]; + +const CITED = "Cited by 40 downstream filings."; + +/** Index-aligned groups, as `buildEntryGroups` hands them over. Only the middle + * entry carries bullets, so the cascade has something to be wrong about. */ +function groupsFor(achievements: readonly HeuristicAchievement[]): BulletGroup[] { + return achievements.map((a, i) => ({ + experienceIndex: i, + experience: { title: a.title }, + bullets: a.title === "Best Paper" ? [bullet(0, CITED)] : [], + })); +} + +let container: HTMLDivElement; +let root: Root; + +interface Spies { + onRemoveEntry: ReturnType; + onRemoveBullet: ReturnType; + onAchievementField: ReturnType; +} + +/** + * Render the section over `achievements` with `removedEntries` already applied + * — i.e. exactly the state the container is in on the render AFTER a deletion: + * the array is filtered, and `parsedIndices` is the map back. + */ +function render( + achievements: readonly HeuristicAchievement[], + removedEntries: ReadonlySet = new Set(), + added: AddedEntry[] = [], +): Spies { + const spies: Spies = { + onRemoveEntry: vi.fn(), + onRemoveBullet: vi.fn(() => true), + onAchievementField: vi.fn(), + }; + const originalCount = achievements.length - added.length; + act(() => + root.render( + createElement(AchievementsSection, { + achievements: [...achievements], + groups: groupsFor(achievements), + addedAchievements: added, + originalCount, + parsedIndices: survivingParsedIndices( + "achievements", + removedEntries, + originalCount, + ), + onAddEntry: () => {}, + onEntryField: () => {}, + onAddBullet: () => {}, + onPruneEmpty: () => {}, + ...spies, + }), + ), + ); + return spies; +} + +function removeButtons(): HTMLElement[] { + return [ + ...container.querySelectorAll('[aria-label="Remove achievement"]'), + ]; +} + +beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(() => { + act(() => root.unmount()); + container.remove(); +}); + +describe("the parsed-entry remove affordance (#856)", () => { + it("renders one on EVERY entry, parsed and added alike", () => { + render(PARSED); + expect(removeButtons()).toHaveLength(3); + }); + + it("deletes by the entry's parsedEntryKey, and takes its bullets", () => { + const spies = render(PARSED); + act(() => removeButtons()[1].click()); + + expect(spies.onRemoveEntry).toHaveBeenCalledExactlyOnceWith( + "achievements:1", + ); + // Bullets go through `removeBullet` — dropping the entry cannot take them + // out of `sections`, which is the pool the anonymous scorer grades. + expect(spies.onRemoveBullet).toHaveBeenCalledExactlyOnceWith( + bulletId(CITED, 0), + { entryKey: "achievements:1", text: CITED }, + ); + }); + + it("deletes a bullet-less entry with no bullet writes at all", () => { + const spies = render(PARSED); + act(() => removeButtons()[0].click()); + expect(spies.onRemoveEntry).toHaveBeenCalledExactlyOnceWith( + "achievements:0", + ); + expect(spies.onRemoveBullet).not.toHaveBeenCalled(); + }); + + it("still removes a user-ADDED entry by its own id", () => { + const added: AddedEntry = { + id: "added:7", + section: "achievements", + title: "Hand-typed award", + }; + const spies = render( + [...PARSED, { title: added.title }], + new Set(), + [added], + ); + act(() => removeButtons()[3].click()); + expect(spies.onRemoveEntry).toHaveBeenCalledExactlyOnceWith("added:7"); + }); +}); + +describe("index resolution after a deletion (#856)", () => { + // The render AFTER deleting parsed index 0: the array the section maps over + // has been filtered, so render position 0 is now parsed index 1. + const AFTER = PARSED.slice(1); + const REMOVED = new Set(["achievements:0"]); + + it("deletes the NEXT entry by its parsed index, not its render position", () => { + const spies = render(AFTER, REMOVED); + expect(removeButtons()).toHaveLength(2); + + act(() => removeButtons()[0].click()); + // Render position 0 — "achievements:0" here would be a no-op re-delete of + // the entry that is already gone, leaving this one un-deletable forever. + expect(spies.onRemoveEntry).toHaveBeenCalledExactlyOnceWith( + "achievements:1", + ); + expect(spies.onRemoveBullet).toHaveBeenCalledExactlyOnceWith( + bulletId(CITED, 0), + { entryKey: "achievements:1", text: CITED }, + ); + }); + + it("files a surviving entry's field edit under its parsed index", () => { + const spies = render(AFTER, REMOVED); + // The year cell of the SECOND surviving entry — parsed index 2. Read mode + // names the control "Edit