diff --git a/src/components/features/JobLetterIndicator.test.tsx b/src/components/features/JobLetterIndicator.test.tsx index 9f40060c..38851d9f 100644 --- a/src/components/features/JobLetterIndicator.test.tsx +++ b/src/components/features/JobLetterIndicator.test.tsx @@ -172,3 +172,196 @@ describe("JobLetterIndicator", () => { } }); }); + +/** #767: inheritance reaches the user through the DIALOGS, never through the + * glyph. These are the two halves of that: what the row still says, and what + * the customize path hands the editor. */ +describe("JobLetterIndicator inherited letters (#767)", () => { + const standard: LetterRecord = { + id: "standard-1", + createdAt: 1, + updatedAt: 9, + body: "My standard letter.", + }; + const inherited = { letter: standard, label: "your standard letter" }; + + it("a standard letter does NOT flip the row's glyph to has-letter", () => { + // The acceptance criterion with teeth: the row must keep offering to WRITE + // one. Flipping it would claim a letter the user never wrote for this + // employer, and the reveal would then show text they did not intend for it. + dom.render( + , + ); + expect( + dom.container.querySelector('button[aria-label="Write a cover letter"]'), + ).toBeTruthy(); + expect( + dom.container.querySelector('button[aria-label="View cover letter"]'), + ).toBeNull(); + }); + + it("offers the inherited letter as a starting point in the editor", () => { + dom.render( + , + ); + clickButton("Write a cover letter"); + + // Offered, capitalized for a standalone chip — and NOT seeded: the body is + // still empty until the user picks it. + const chip = [...dom.container.querySelectorAll("button")].find( + (b) => b.textContent === "Your standard letter", + ); + expect(chip).toBeTruthy(); + expect(dom.container.querySelector("textarea")!.value).toBe(""); + }); + + it("Customize seeds the editor from the inherited letter", () => { + dom.render( + , + ); + clickButton("View cover letter"); + clickButton("your standard letter"); + clickButton("Customize for this job"); + + // Seeded with the SOURCE's text, in an editor composing a new draft — the + // copy notice is what proves it is not revising the standard letter. + expect(dom.container.querySelector("textarea")!.value).toBe( + "My standard letter.", + ); + expect(dom.container.textContent).toContain("Started from"); + }); + + it("a plain write-one click never seeds, even with something to inherit", () => { + dom.render( + , + ); + clickButton("Write a cover letter"); + expect(dom.container.querySelector("textarea")!.value).toBe(""); + expect(dom.container.textContent).not.toContain("Started from"); + }); +}); + +/** #767 review, blocking 1: the egress acknowledgement must gate everything + * this component can put on screen, not just the job's OWN letters. Adding the + * inherited entry added two routes to an outside-produced body — the reveal's + * chip and the editor's picker — and neither read its `producer`. */ +describe("JobLetterIndicator egress gate over inherited letters (#767)", () => { + const outsideStandard: LetterRecord = { + id: "standard-1", + createdAt: 1, + updatedAt: 9, + body: "My standard letter.", + producer: { contract: 1, producer: "some-outside-producer" }, + }; + const inherited = { letter: outsideStandard, label: "your standard letter" }; + + it("warns before revealing, when only the INHERITED letter came from outside", () => { + // The job's own letter was hand-typed, so the pre-#767 test + // (`hasOutsideProducer(letters)`) says don't warn — but one click on the + // inherited chip would put outside-produced text on screen. + dom.render( + , + ); + clickButton("View cover letter"); + expect(dom.openDialogText()).toContain("Before you view this letter"); + }); + + it("warns before the EDITOR too, when the job has no letters of its own", () => { + // The worse path: no own letters means the glyph opens the editor directly, + // and the picker chip is one click from the same outside-produced body. + dom.render( + , + ); + clickButton("Write a cover letter"); + expect(dom.openDialogText()).toContain("Before you view this letter"); + }); + + it("lands on the right surface after acknowledging, per own-letter state", () => { + // The ack path used to hard-code `reveal`, which was correct only while the + // empty case could never warn. It can now. + dom.render( + , + ); + clickButton("Write a cover letter"); + clickButton("Got it"); + expect(dom.openDialogText()).toContain("Write a cover letter"); + expect(dom.container.querySelector("textarea")).toBeTruthy(); + }); + + it("does not warn when nothing reachable came from outside", () => { + // The other direction, still true: a hand-typed own letter and a hand-typed + // inherited one must not be gated behind a warning that would be false. + const { producer: _drop, ...cleanStandard } = outsideStandard; + void _drop; + dom.render( + , + ); + clickButton("View cover letter"); + expect(dom.openDialogText()).not.toContain("Before you view this letter"); + }); +}); + +/** #767 review, blocking 2: the company tier had no write path at all, so + * `scope: "company"` could only ever fire for a record an outside producer + * wrote. "Customize for this company" is that path. */ +describe("JobLetterIndicator company write path (#767)", () => { + it("offers to lift a letter to company scope when the job has a company key", () => { + dom.render( + , + ); + clickButton("View cover letter"); + expect( + [...dom.container.querySelectorAll("button")].some( + (b) => b.textContent === "Customize for this company", + ), + ).toBe(true); + }); + + it("offers nothing of the sort when the job has no company to key on", () => { + dom.render( + , + ); + clickButton("View cover letter"); + expect( + [...dom.container.querySelectorAll("button")].some( + (b) => b.textContent === "Customize for this company", + ), + ).toBe(false); + }); + + it("opens the editor in COMPANY scope, seeded, with no jobId", () => { + dom.render( + , + ); + clickButton("View cover letter"); + clickButton("Customize for this company"); + + // The title is what tells the user which letter they are about to write, + // and it is derived from the scope keys the save will carry. + expect(dom.openDialogText()).toContain("Write a company letter"); + expect(dom.container.querySelector("textarea")!.value).toBe("Own body."); + }); +}); diff --git a/src/components/features/JobLetterIndicator.tsx b/src/components/features/JobLetterIndicator.tsx index 62655793..60a9a459 100644 --- a/src/components/features/JobLetterIndicator.tsx +++ b/src/components/features/JobLetterIndicator.tsx @@ -62,8 +62,11 @@ import { hasAcknowledgedLetterEgress, recordLetterEgressAcknowledged, } from "../../lib/letter-egress-ack.ts"; -import { LetterRevealDialog } from "./LetterRevealDialog.tsx"; -import { LetterEditorDialog } from "./LetterEditorDialog.tsx"; +import { LetterRevealDialog, type InheritedLetter } from "./LetterRevealDialog.tsx"; +import { + LetterEditorDialog, + type LetterStartingPoint, +} from "./LetterEditorDialog.tsx"; import type { LetterRecord } from "../../lib/storage/index.ts"; /** Shared frame for both glyphs, so the two states differ only in the mark @@ -113,8 +116,24 @@ interface JobLetterIndicatorProps { /** The job these letters belong to — needed to write a new one. */ jobId: string; /** Every letter for this one job, most-recently-updated first. Empty (or - * omitted) renders the "write one" state, not nothing. */ + * omitted) renders the "write one" state, not nothing. + * + * THIS JOB'S OWN letters only, and that is what the glyph reports (#767). A + * company or standard letter existing must never flip a row to "has letter": + * the row would claim a letter the user never wrote for that employer, and + * the reveal would then show text they did not intend for it. Inheritance + * surfaces inside the dialogs, where there is room to say what it is. */ letters?: readonly LetterRecord[]; + /** The letter this job would inherit — its company's, or the standard one + * (#767). Drives the reveal's extra entry and the editor's "Start from…" + * picker. Omitted when the user has written nothing this job can reach. */ + inherited?: InheritedLetter; + /** This job's company as a DERIVED key (`deriveCompanyKey`), when it has one + * (#767). Present enables "Customize for this company", which is the only + * write path to the company tier — absent when the job's `company` is blank + * or all punctuation, which is exactly when a company letter would have no + * key to be found by. */ + companyKey?: string; /** Re-read the letter store after a write. Optional so a caller that only * displays letters (a test, a future read-only view) need not supply one; * without it a saved letter will not appear until the view remounts. */ @@ -129,40 +148,119 @@ function hasOutsideProducer(letters: readonly LetterRecord[]): boolean { return letters.some((letter) => letter.producer !== undefined); } +/** The scope phrase reads as a sentence fragment inside the reveal ("This is + * your standard letter") and as a standalone chip label in the editor's + * picker. One phrase, capitalized at the call site that needs it, rather than + * two strings the caller has to keep in step. + * + * ONLY the first character, deliberately: the phrase embeds a company name the + * user typed, echoed exactly as they typed it, so a lowercase "northwind" + * renders as "Your northwind letter". Title-casing free text is the worse + * option — it would mangle "eBay", "iRobot" and every deliberately-lowercase + * brand, and a name the user can see is theirs beats one this app restyled. */ +function capitalize(phrase: string): string { + return phrase.charAt(0).toUpperCase() + phrase.slice(1); +} + export function JobLetterIndicator({ jobId, letters = [], + inherited, + companyKey, onSaved = () => {}, }: JobLetterIndicatorProps) { const [stage, setStage] = useState("closed"); // Which letter the editor is revising. `undefined` composes a new draft, // which is also the empty-state path — one editor, both jobs. const [editing, setEditing] = useState(undefined); + // Set only by a Customize click — the user picking a letter to copy. Every + // other route into the editor clears it, which is what keeps a plain "Write a + // cover letter" click opening an empty draft. + const [seed, setSeed] = useState(undefined); + // Which scope the editor is composing FOR. "job" everywhere except + // "Customize for this company", which is the only write path to the company + // tier — see `openEditor`. + const [composeScope, setComposeScope] = useState<"job" | "company">("job"); const hasLetters = letters.length > 0; + + // What the editor may be started from. One entry, because `resolveLetterForJob` + // already picked the single most specific inherited letter — offering both a + // company and a standard letter here would ask the user to redo a decision the + // chain exists to make. Empty while revising is enforced by the editor itself. + const startFrom: readonly LetterStartingPoint[] = inherited + ? [{ letter: inherited.letter, label: capitalize(inherited.label) }] + : []; const label = !hasLetters ? "Write a cover letter" : letters.length === 1 ? "View cover letter" : `View cover letters (${letters.length})`; - function open() { - if (!hasLetters) { - setEditing(undefined); - setStage("edit"); - return; + /** What to call a letter being copied FROM, in the editor's copy notice. The + * inherited letter has a scope phrase; one of this job's own drafts has only + * its user-set label, and falls back to the same wording the reveal titles + * an unlabelled draft with. */ + function labelFor(source: LetterRecord): string { + if (inherited && source.id === inherited.letter.id) { + return capitalize(inherited.label); } + return source.label || "This job's letter"; + } + + /** + * Open into an editor composing a fresh draft for `scope`, optionally seeded + * from `from`. The ONE route into compose mode, so the three pieces of state + * that define it can never drift apart: no `editing` record (which is what + * makes a save an insert rather than an upsert over the source), the seed, + * and the scope key the save will carry. + */ + function openEditor( + scope: "job" | "company", + from?: LetterStartingPoint, + ): void { + setEditing(undefined); + setSeed(from); + setComposeScope(scope); + setStage("edit"); + } + + function open() { + // Gate on everything a click from here can put on screen, not just this + // job's own letters. Since #767 the reveal offers the inherited letter as + // an entry and the editor offers it as a starting point, so an + // outside-produced STANDARD letter reaches the screen through a job whose + // own letters are all hand-typed — and the warning is about egress that + // already happened to the text being shown, whichever scope holds it. + // // Read the acknowledgement fresh, not from a cached hook value: several // rows' indicators are mounted at once on this page, and it is meant to be // "once, ever" — not "once per row." See `letter-egress-ack.ts`. - const mustWarn = - hasOutsideProducer(letters) && !hasAcknowledgedLetterEgress(); - setStage(mustWarn ? "ack" : "reveal"); + const exposesOutsideProducer = + hasOutsideProducer(letters) || inherited?.letter.producer !== undefined; + if (exposesOutsideProducer && !hasAcknowledgedLetterEgress()) { + setStage("ack"); + return; + } + reveal(); + } + + /** Where `open` lands once the warning (if any) is out of the way — the + * reveal for a job with its own drafts, the editor for one without. Shared + * with `acknowledge` so the post-warning destination cannot diverge from the + * no-warning one; before #767 the ack path hard-coded `"reveal"`, which was + * right only while the empty case could never warn. */ + function reveal() { + if (!hasLetters) { + openEditor("job"); + return; + } + setStage("reveal"); } function acknowledge() { recordLetterEgressAcknowledged(); - setStage("reveal"); + reveal(); } return ( @@ -212,21 +310,52 @@ export function JobLetterIndicator({ open={stage === "reveal"} onClose={() => setStage("closed")} letters={letters} + inherited={inherited} onEdit={(letter) => { setEditing(letter); + setSeed(undefined); + setComposeScope("job"); setStage("edit"); }} - onCompose={() => { - setEditing(undefined); - setStage("edit"); - }} + onCompose={() => openEditor("job")} + // Compose, NOT revise — `openEditor` leaves `editing` undefined so the + // editor writes a new record with no id. Handing the source record to + // `editing` would make Save OVERWRITE the letter being copied, which is + // the one failure this whole flow is arranged to prevent. + // + // `source` is the letter the reveal actually has on screen, taken from + // the argument rather than reached for in `startFrom` — the two are the + // same record while there is one inherited entry, and taking the + // argument keeps this correct if a second is ever offered. + onCustomize={(source) => + openEditor("job", { letter: source, label: labelFor(source) }) + } + companyOffer={ + companyKey !== undefined + ? { + label: "Customize for this company", + onCustomize: (source) => + openEditor("company", { + letter: source, + label: labelFor(source), + }), + } + : undefined + } /> setStage("closed")} - jobId={jobId} + // Exactly one scope key, never both — the contract refuses a record + // carrying two. Composing for the company tier drops `jobId` entirely, + // which is what makes the saved letter reachable from every job at that + // employer rather than just this one. + jobId={composeScope === "company" ? undefined : jobId} + companyKey={composeScope === "company" ? companyKey : undefined} letter={editing} + startFrom={startFrom} + seed={seed} onSaved={onSaved} /> diff --git a/src/components/features/JobTracker.tsx b/src/components/features/JobTracker.tsx index 5bc95105..4446f51f 100644 --- a/src/components/features/JobTracker.tsx +++ b/src/components/features/JobTracker.tsx @@ -78,6 +78,10 @@ import { JobArchiveSweepDialog } from "./JobArchiveSweepDialog.tsx"; import { useJobTracker, type JobTracker as Tracker } from "../../hooks/useJobTracker.ts"; import { useSavedJobRatings } from "../../hooks/useSavedJobRatings.ts"; import { useJobLetters } from "../../hooks/useJobLetters.ts"; +import { StandardLetterButton } from "./StandardLetterButton.tsx"; +import type { InheritedLetter } from "./LetterRevealDialog.tsx"; +import { resolveLetterForJob } from "../../lib/letters/resolve-letter.ts"; +import { deriveCompanyKey } from "../../lib/storage/company-key.ts"; import { useJobDuplicates, type JobDuplicateSuggestion, @@ -87,6 +91,38 @@ import type { JobRepostCluster } from "../../lib/job-repost-clusters.ts"; import type { HeuristicParsedResume } from "../../lib/heuristics/types.ts"; import type { JobRating } from "../../lib/job-search/rating.ts"; +/** + * The letter one row INHERITS, phrased for display (#767), or `undefined` when + * the job has its own letter or there is nothing to inherit. + * + * The `scope === "job"` case is dropped here rather than inside + * `resolveLetterForJob`, because the chain answers "which letter applies" and + * this surface asks the narrower "is the applying letter someone else's" — the + * row's own drafts already reach it through `lettersById`. + * + * The phrase is built here because this is the layer holding `job.company`; + * the dialogs downstream only need something to print. It is a fragment, not a + * sentence, so a caller can capitalize it or embed it mid-sentence. + */ +function inheritedFor( + job: JobRecord, + letters: readonly LetterRecord[] | undefined, +): InheritedLetter | undefined { + if (!letters || letters.length === 0) return undefined; + const resolved = resolveLetterForJob(job, letters); + if (!resolved || resolved.scope === "job") return undefined; + return { + letter: resolved.letter, + // `job.company` verbatim, not the normalised key: the key is a lookup + // token ("northwind"), and printing it back at the user would show them a + // lowercased, suffix-stripped version of a name they typed. + label: + resolved.scope === "company" + ? `your ${job.company} letter` + : "your standard letter", + }; +} + interface JobTrackerProps { tracker: Tracker; /** Fitness rating per job id, or null when the library has not been rated — @@ -115,6 +151,14 @@ interface JobTrackerProps { /** Every letter, grouped by job id (#715) — `useJobLetters`' shape. A job id * absent from the map has no letters, so its row renders no indicator. */ lettersById?: ReadonlyMap; + /** Every live letter, flat (#767) — what each row's `resolveLetterForJob` + * runs against to find the company or standard letter it would inherit. + * Omitted resolves nothing, so a caller that has not read the store gets + * exactly the pre-#767 behaviour. */ + allLetters?: readonly LetterRecord[]; + /** The user's standard letter, if written (#767) — the panel-level button's + * state. Absent renders "Write a standard letter". */ + standardLetter?: LetterRecord; /** Re-read the letter store after a row writes one. Optional so a caller * that only displays letters need not supply one; without it a saved letter * will not appear until this view remounts. */ @@ -150,6 +194,8 @@ export function JobTrackerSection({ | "ratings" | "hasResume" | "lettersById" + | "allLetters" + | "standardLetter" | "onLettersChanged" | "duplicatesByJobId" | "onDismissDuplicate" @@ -171,6 +217,12 @@ export function JobTrackerSection({ ratings={ratings} hasResume={parsed !== undefined} lettersById={letters.byJobId} + allLetters={letters.all} + // `standard` is most-recently-updated first, so `[0]` is the current + // standard letter. Nothing writes a second one — the panel button edits + // the existing record — but the store holds a list, so this reads the + // newest rather than assuming there is exactly one. + standardLetter={letters.standard[0]} onLettersChanged={letters.refresh} duplicatesByJobId={duplicates.byJobId} onDismissDuplicate={duplicates.dismiss} @@ -188,6 +240,8 @@ export function JobTracker({ resumeName, resumeOptions, lettersById, + allLetters, + standardLetter, onLettersChanged, duplicatesByJobId, onDismissDuplicate, @@ -242,6 +296,21 @@ export function JobTracker({ // which would otherwise render as a page of headers over no rows, the very // failure collapse-by-default exists to avoid. Then every section opens; the // "only non-empty bucket is rejected" case is the single-bucket case of it. + // One pass over the letter set for the whole library, not one per row per + // render. `resolveLetterForJob` walks `allLetters` up to three times, and the + // row `.map()` below re-runs on every keystroke in an `EditableField` and + // every status-filter toggle — without this the cost is O(jobs x letters x 3) + // per render. Keyed by job id so a row still gets its own answer. + const inheritedByJobId = useMemo(() => { + const byId = new Map(); + if (!allLetters || allLetters.length === 0) return byId; + for (const job of jobs) { + const resolved = inheritedFor(job, allLetters); + if (resolved) byId.set(job.id, resolved); + } + return byId; + }, [jobs, allLetters]); + const anyOpenByDefault = groups.some(({ bucket }) => !isCollapsedByDefault(bucket)); if (!ready) return null; @@ -262,6 +331,12 @@ export function JobTracker({ {persisted ? "Persistent" : "Best-effort"} + {/* Panel-level, not per-row (#767): the standard letter is the one + letter with no job to hang off. See `StandardLetterButton`. */} +