From 9a872603cbc75aae13868a6ecdcf0ecc4116ca19 Mon Sep 17 00:00:00 2001 From: Samhit21 Date: Wed, 26 Aug 2026 16:41:45 -0500 Subject: [PATCH] =?UTF-8?q?feat(letters):=20job=20=E2=86=92=20company=20?= =?UTF-8?q?=E2=86=92=20standard=20resolution,=20picker,=20customize-from?= =?UTF-8?q?=20(#767)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With #766's scope keys stored, a job could be reached by up to three letters — its own, its company's, and the standard one — and nothing decided which the user saw or let them create anything but a job letter. Adds `resolveLetterForJob`: a pure chain, job → company → standard, first hit wins, returning the SCOPE alongside the letter because every surface needs it to tell the user why they are looking at this text. A job whose company is blank skips the company rung rather than matching an empty key, and a letter naming another job is never inherited by a sibling job at the same employer — specificity beats recency, so a standard letter edited today does not outrank a company letter written last month. The row glyph is unchanged and still counts the job's OWN letters. A standard letter existing must not flip every row to "has letter": that 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 instead, where there is room to name it. Customize-from is a COPY. The editor carries no `id` from its starting point, so saving inserts rather than upserting over the source, and the dialog says so at the moment of copying — a live link would mean editing job B's letter rewrote the standard letter already submitted for job A, and for prose there is no merge that makes that safe (#765). Nothing seeds without an explicit pick, and the picker retires once a starting point is taken, so a mis-click cannot replace a body the user has since typed into. All three tiers are writable. "Customize for this company" in the reveal is the company tier's only write path — offered for the job's own drafts too, since "I wrote this for one posting and want it for every job here" is how a company letter actually comes to exist — and `StandardLetterButton` is the standard tier's, because that is the one scope with no job to hang off. The egress acknowledgement gates everything these surfaces can reach, not just the job's own letters: the reveal's inherited entry and the editor's starting point both put another scope's body on screen, and the warning is about egress that already happened to the text being shown, whichever scope holds it. `LetterEditorDialog` now authors all three scopes rather than gaining a sibling editor, per the issue's reuse analysis; `StandardLetterButton` is the one new component, because no existing surface owns a panel-level letter affordance. Extracts `clickButtonIn`/`typeIntoTextArea` into the shared dialog test harness rather than letting the new suite re-derive them — fallow flagged the clone. --- .../features/JobLetterIndicator.test.tsx | 193 +++++++++++++++ .../features/JobLetterIndicator.tsx | 163 +++++++++++-- src/components/features/JobTracker.tsx | 77 ++++++ src/components/features/JobTrackerEntry.tsx | 13 + .../features/LetterEditorDialog.test.tsx | 222 ++++++++++++++++-- .../features/LetterEditorDialog.tsx | 183 ++++++++++++++- .../features/LetterRevealDialog.test.tsx | 86 +++++++ .../features/LetterRevealDialog.tsx | 104 +++++++- .../features/StandardLetterButton.test.tsx | 106 +++++++++ .../features/StandardLetterButton.tsx | 79 +++++++ .../features/__test-utils__/dialog-dom.ts | 48 ++++ src/hooks/useJobLetters.test.tsx | 16 ++ src/hooks/useJobLetters.ts | 14 +- src/lib/letters/resolve-letter.test.ts | 142 +++++++++++ src/lib/letters/resolve-letter.ts | 101 ++++++++ src/lib/storage/index.ts | 19 +- 16 files changed, 1503 insertions(+), 63 deletions(-) create mode 100644 src/components/features/StandardLetterButton.test.tsx create mode 100644 src/components/features/StandardLetterButton.tsx create mode 100644 src/lib/letters/resolve-letter.test.ts create mode 100644 src/lib/letters/resolve-letter.ts 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`. */} +