From 156407074816413a2ae200e4f7d8be74fe53b3af 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. Review fixes (s-annam, #906): The reveal's inherited half could not execute. `inheritedFor` asked the chain "which letter applies" and dropped a `"job"` answer, but the chain answers `"job"` exactly when a letter carries this job's id — which is exactly when the row has its own letters, which is what makes the indicator open the REVEAL rather than the editor. So `inherited` was `undefined` for precisely the jobs whose reveal opens, and the inherited chip, the scope notice and "Customize for this job" were dead in the product while their unit tests passed on a `letters` + `inherited` pair the tracker cannot produce. Adds `inheritedLetterForJob` — the same chain entered one rung down — so the two questions are asked separately and rung order stays owned in one place, plus `JobTracker`-level tests that derive both letter props from one `groupByScope` call, the wiring no test exercised. `StandardLetterButton` was a second door onto outside-produced text with no egress acknowledgement: an imported standard letter carrying a `producer` block rendered in full with nothing written to `letter-egress-ack`. Extracts the gate into `LetterEgressAckDialog` — one wording, one flag — and applies it here as well as in the indicator, rather than hand-rolling a second copy. "Customize for this company" always inserted, so with the company's own letter selected it forked that letter from itself, and the duplicate was unreachable forever — the chain surfaces only the newest per rung, it has no `jobId` for `deleteLettersForJob` to cascade to, and there is no company-letter list. The offer now edits in place when the selection already is that company's letter, via `isCompanyLetter`, the same predicate the chain reads the rung with. That also gives the company tier the edit path it lacked. The standard-letter button waits for `useJobLetters.ready`. `standard[0]` of a not-yet-loaded list is `undefined`, indistinguishable from "the user has none", and composing in that window wrote a second unscoped record and orphaned the first — the standard tier is the one that loses a record that way, being the one with a single window onto it. Also: the scope phrase is stored as a lowercase fragment and capitalized by the render sites that stand it alone (`scope-phrase.ts`), which was inverted in both directions at once; `deriveCompanyKey` moves into a memo beside the inherited one instead of running per row per render; and the tracker's memo no longer sits between a pre-existing comment and the statement it documents. Field-dropping on save (`producer`, `resumeId`) is pre-existing on `main` and tracked in #929, including for the company edit path added here. --- .../features/JobLetterIndicator.test.tsx | 292 ++++++++++++++++++ .../features/JobLetterIndicator.tsx | 217 +++++++++---- src/components/features/JobTracker.test.tsx | 167 +++++++++- src/components/features/JobTracker.tsx | 135 ++++++++ src/components/features/JobTrackerEntry.tsx | 13 + .../features/LetterEditorDialog.test.tsx | 222 +++++++++++-- .../features/LetterEditorDialog.tsx | 188 ++++++++++- .../features/LetterEgressAckDialog.tsx | 98 ++++++ .../features/LetterRevealDialog.test.tsx | 89 ++++++ .../features/LetterRevealDialog.tsx | 117 ++++++- .../features/StandardLetterButton.test.tsx | 200 ++++++++++++ .../features/StandardLetterButton.tsx | 110 +++++++ .../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 | 237 ++++++++++++++ src/lib/letters/resolve-letter.ts | 140 +++++++++ src/lib/letters/scope-phrase.ts | 29 ++ src/lib/storage/index.ts | 19 +- 19 files changed, 2241 insertions(+), 110 deletions(-) create mode 100644 src/components/features/LetterEgressAckDialog.tsx 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 create mode 100644 src/lib/letters/scope-phrase.ts diff --git a/src/components/features/JobLetterIndicator.test.tsx b/src/components/features/JobLetterIndicator.test.tsx index 9f40060c..ddd35191 100644 --- a/src/components/features/JobLetterIndicator.test.tsx +++ b/src/components/features/JobLetterIndicator.test.tsx @@ -172,3 +172,295 @@ 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."); + }); +}); + +/** + * #767 review, secondary: the company offer used to ALWAYS insert, so with the + * company's own letter on screen it offered to fork that letter from itself. + * The duplicate was then unreachable forever — the chain surfaces only the + * most-recently-updated record per rung, it has no `jobId` for + * `deleteLettersForJob` to cascade to, and there is no company-letter list to + * delete it from. + */ +describe("JobLetterIndicator company tier: edit, not fork (#767 review)", () => { + const companyLetter = ownLetter({ + id: "company-1", + jobId: undefined, + companyKey: "northwind", + label: "Northwind", + body: "Company letter v1.", + }); + + const inheritedCompany = { + letter: companyLetter, + label: "your Northwind letter", + }; + + it("edits in place when the letter on screen IS this company's letter", () => { + dom.render( + , + ); + clickButton("View cover letter"); + clickButton("Your Northwind letter"); + + // The offer renames itself, because it is now a different action. + expect( + [...dom.container.querySelectorAll("button")].some( + (b) => b.textContent === "Customize for this company", + ), + ).toBe(false); + clickButton("Edit this company letter"); + + // REVISING: the editor titles it as an edit and carries the body through + // with no copy notice — the notice is what marks an insert. + expect(dom.openDialogText()).toContain("Edit company letter"); + expect(dom.openDialogText()).not.toContain("Started from"); + expect(dom.container.querySelector("textarea")!.value).toBe( + "Company letter v1.", + ); + }); + + it("still forks when the letter on screen belongs to another scope", () => { + // Lifting one job's draft to company scope is the offer's original job, and + // it must stay an insert — that is a new record at a new scope. + dom.render( + , + ); + clickButton("View cover letter"); + clickButton("Customize for this company"); + + expect(dom.openDialogText()).toContain("Write a company letter"); + expect(dom.openDialogText()).toContain("Started from"); + expect(dom.container.querySelector("textarea")!.value).toBe("Own body."); + }); + + it("forks, not edits, when the selected letter is ANOTHER company's", () => { + // Same key check the chain reads the rung with: a letter at a different key + // is not this company's letter, whatever else it carries. + dom.render( + , + ); + clickButton("View cover letter"); + clickButton("Your Contoso letter"); + expect( + [...dom.container.querySelectorAll("button")].some( + (b) => b.textContent === "Edit this company letter", + ), + ).toBe(false); + }); +}); diff --git a/src/components/features/JobLetterIndicator.tsx b/src/components/features/JobLetterIndicator.tsx index 62655793..2c66d6fc 100644 --- a/src/components/features/JobLetterIndicator.tsx +++ b/src/components/features/JobLetterIndicator.tsx @@ -27,7 +27,9 @@ * the résumé and the job details to that model's API, and the user deserves to * be told once. A letter typed into `LetterEditorDialog` sent nothing * anywhere, so gating it behind that warning would be telling the user - * something untrue about their own typing. `hasOutsideProducer` is the test: + * something untrue about their own typing. {@link letterEgressNeedsAck} is the + * test — shared with `StandardLetterButton` since #767 gave that surface its own + * door onto the same class of text, so one wording and one flag serve both: * `docs/cover-letter-contract.md` §6 reads an absent `producer` block as * "written by offlinecv itself", and the editor never writes one. That marker * is self-reported and optional, so it is used ONLY in this direction — a @@ -57,13 +59,17 @@ */ import { useState } from "react"; -import { Button, Dialog } from "@design-system"; +import { Button } from "@design-system"; import { - hasAcknowledgedLetterEgress, - recordLetterEgressAcknowledged, -} from "../../lib/letter-egress-ack.ts"; -import { LetterRevealDialog } from "./LetterRevealDialog.tsx"; -import { LetterEditorDialog } from "./LetterEditorDialog.tsx"; + LetterEgressAckDialog, + letterEgressNeedsAck, +} from "./LetterEgressAckDialog.tsx"; +import { isCompanyLetter } from "../../lib/letters/resolve-letter.ts"; +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 +119,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. */ @@ -123,45 +145,110 @@ interface JobLetterIndicatorProps { type Stage = "closed" | "ack" | "reveal" | "edit"; -/** True when any letter here was written OUTSIDE this app — the only case the - * egress warning is about. See the docblock on why this is read one-way. */ -function hasOutsideProducer(letters: readonly LetterRecord[]): boolean { - return letters.some((letter) => letter.producer !== undefined); -} - 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. + // The label stays the lowercase FRAGMENT it arrives as; the editor's picker + // capitalizes it for the chip and its copy notice embeds it mid-sentence. + // See `scope-phrase.ts` for why the casing cannot be decided here. + const startFrom: readonly LetterStartingPoint[] = inherited + ? [{ letter: inherited.letter, label: inherited.label }] + : []; const label = !hasLetters ? "Write a cover letter" : letters.length === 1 ? "View cover letter" : `View cover letters (${letters.length})`; + /** 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 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"); + } + + /** + * Open into an editor REVISING `letter` under `scope` — the counterpart of + * {@link openEditor}, and the one route in, so the seed can never survive + * from a previous Customize click into a revise that must not be a copy. + */ + function editLetter(letter: LetterRecord, scope: "job" | "company"): void { + setEditing(letter); + setSeed(undefined); + setComposeScope(scope); + setStage("edit"); + } + function open() { - if (!hasLetters) { - setEditing(undefined); - setStage("edit"); - return; - } + // 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"); + if (letterEgressNeedsAck([...letters, inherited?.letter])) { + setStage("ack"); + return; + } + reveal(); } - function acknowledge() { - recordLetterEgressAcknowledged(); + /** 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"); } @@ -182,51 +269,69 @@ export function JobLetterIndicator({ {hasLetters ? : } - setStage("closed")} - title="Before you view this letter" - className="max-w-md" - > -
-

- offlinecv stores this letter — it did not write it. To draft it, - whatever generated the text sent your résumé and the job details - out to a model’s API. That step happened outside this - app’s on-device guarantee. Reading the letter here, or - copying it, sends nothing further. -

-

- Your confirmation is saved in this browser, so you normally see - this once. If this browser blocks that storage, it will ask again. -

-
- -
-
- + onAcknowledged={reveal} + /> setStage("closed")} letters={letters} - onEdit={(letter) => { - setEditing(letter); - setStage("edit"); - }} - onCompose={() => { - setEditing(undefined); - setStage("edit"); - }} + inherited={inherited} + onEdit={(letter) => editLetter(letter, "job")} + 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) }) + } + // Fork OR revise, decided by what is actually on screen. Lifting a job + // letter to company scope must insert — that is a new record at a new + // scope. But the reveal also offers this while the COMPANY's own letter + // is selected, and inserting there forks the letter from itself: the + // first record keeps the same key, only the newest is ever surfaced by + // the chain, it has no `jobId` for `deleteLettersForJob` to cascade to, + // and there is no company-letter list to delete it from — so it is + // unreachable forever (#767 review). Same predicate the chain reads the + // rung with, so the two cannot disagree about what "the company's + // letter" is. + companyOffer={ + companyKey !== undefined + ? { + companyKey, + onCustomize: (source) => + isCompanyLetter(source, companyKey) + ? editLetter(source, "company") + : 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.test.tsx b/src/components/features/JobTracker.test.tsx index 1343cd97..445468bc 100644 --- a/src/components/features/JobTracker.test.tsx +++ b/src/components/features/JobTracker.test.tsx @@ -21,6 +21,7 @@ import type { JobRecord, LetterRecord } from "../../lib/storage/index.ts"; import type { JobRating } from "../../lib/job-search/rating.ts"; import type { JobDuplicateSuggestion } from "../../hooks/useJobDuplicates.ts"; import { findRepostClusters } from "../../lib/job-repost-clusters.ts"; +import { groupByScope } from "../../hooks/useJobLetters.ts"; import { installDialogPolyfill } from "./__test-utils__/dialog-dom.ts"; (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = @@ -45,6 +46,20 @@ afterEach(() => { container.remove(); }); +/** Click the button whose visible text is `label`, and hand it back so a caller + * can assert on presence in the same line. `undefined` rather than a throw when + * there is no match: "this control is absent" is asserted directly here. + * + * Module scope, one copy — three describes had grown their own identical one + * and fallow was reporting the clone family. */ +function clickButton(label: string) { + const button = [...container.querySelectorAll("button")].find( + (b) => b.textContent === label, + ); + act(() => button?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + return button; +} + function job(over: Partial): JobRecord { return { id: over.id ?? crypto.randomUUID(), @@ -254,14 +269,6 @@ describe("JobTracker: resume link picker", () => { { id: "r2", filename: "resume-v2.pdf" }, ]; - function clickButton(label: string) { - const button = [...container.querySelectorAll("button")].find( - (b) => b.textContent === label, - ); - act(() => button?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); - return button; - } - it("offers the saved resumes and links the one picked", () => { const tracker = makeTracker([job({ id: "j1" })]); act(() => @@ -339,6 +346,150 @@ describe("JobTracker: letter indicator (#715)", () => { }); }); +describe("JobTracker: inherited letters, wired as the app wires them (#767)", () => { + /** + * The wiring is the point of this block, not the components. + * + * `lettersById` and `allLetters` are BOTH derived here from one letter set + * through the real `groupByScope`, exactly as `JobTrackerSection` derives them + * from one store read. That is the only arrangement in which the #767 review's + * blocker is visible: the component-level suites pass `letters` and + * `inherited` as independent props, so they can hand a row its own letter AND + * something to inherit — a pair the production wiring cannot produce, because + * owning a letter is exactly what used to make `inheritedFor` answer + * `undefined`. Every assertion below was red before `inheritedLetterForJob`. + */ + function wire(letters: readonly LetterRecord[]) { + const grouped = groupByScope(letters); + return { lettersById: grouped.byJobId, allLetters: letters }; + } + + function standardLetter(over: Partial = {}): LetterRecord { + return { + id: "standard-1", + createdAt: 1, + updatedAt: 5, + body: "STANDARD-BODY.", + ...over, + }; + } + + function ownLetter(jobId: string): LetterRecord { + return { + id: "own-1", + jobId, + createdAt: 1, + updatedAt: 9, + label: "Mine", + body: "OWN-BODY.", + }; + } + + function mount(letters: readonly LetterRecord[]) { + const tracker = makeTracker([ + job({ id: "j1", title: "SWE", company: "Northwind" }), + ]); + act(() => root.render()); + } + + it("offers the standard letter inside the reveal of a job that HAS its own", () => { + mount([ownLetter("j1"), standardLetter()]); + clickButton("View cover letter"); + + // The row's own draft is what opens — the glyph promised a letter for this + // job, so the inherited one must never be what is showing first. + expect(container.textContent).toContain("OWN-BODY."); + expect(container.textContent).not.toContain("STANDARD-BODY."); + + // …and the inherited entry is offered beside it. This is the half that was + // dead in production: `inherited` arrived `undefined` for exactly the jobs + // whose reveal opens. + expect(clickButton("Your standard letter")).toBeTruthy(); + expect(container.textContent).toContain("STANDARD-BODY."); + expect(container.textContent).toContain( + "This is your standard letter, not a letter for this job", + ); + }); + + it("swaps Edit for Customize on that inherited entry (AC 6 + 7, in the product)", () => { + mount([ownLetter("j1"), standardLetter()]); + clickButton("View cover letter"); + clickButton("Your standard letter"); + + expect( + [...container.querySelectorAll("button")].some( + (b) => b.textContent === "Edit", + ), + ).toBe(false); + expect( + [...container.querySelectorAll("button")].some( + (b) => b.textContent === "Customize for this job", + ), + ).toBe(true); + }); + + it("prefers the company letter over the standard one, through the same wiring", () => { + mount([ + ownLetter("j1"), + standardLetter(), + { + id: "company-1", + companyKey: "northwind", + createdAt: 1, + updatedAt: 3, + body: "COMPANY-BODY.", + }, + ]); + clickButton("View cover letter"); + + // Specificity, not recency: the standard letter is the newer record. + expect(clickButton("Your Northwind letter")).toBeTruthy(); + expect(container.textContent).toContain("COMPANY-BODY."); + expect(container.textContent).not.toContain("STANDARD-BODY."); + }); + + it("still offers nothing to inherit when the only letter IS the job's own", () => { + mount([ownLetter("j1")]); + clickButton("View cover letter"); + expect(container.textContent).not.toContain("not a letter for this job"); + expect(clickButton("Customize for this job")).toBeUndefined(); + }); +}); + +describe("JobTracker: the standard-letter button waits for the store (#767)", () => { + it("renders no standard-letter control while the letter read is in flight", () => { + // `standardLetter` is `standard[0]`, and `[0]` of a not-yet-loaded list is + // `undefined` — the same value as "the user has none". Offering to COMPOSE + // in that window writes a second unscoped record and orphans the first, + // because only `[0]` is ever surfaced (#767 review). + act(() => + root.render( + , + ), + ); + expect(clickButton("Write a standard letter")).toBeUndefined(); + expect(clickButton("Edit standard letter")).toBeUndefined(); + }); + + it("renders it once the read has landed", () => { + act(() => + root.render( + , + ), + ); + expect(clickButton("Write a standard letter")).toBeTruthy(); + }); + + it("defaults to ready, so a caller holding the record is unaffected", () => { + // The flag is about a live store read in flight; a caller that passes the + // record directly has already done one. + act(() => + root.render(), + ); + expect(clickButton("Write a standard letter")).toBeTruthy(); + }); +}); + describe("JobTracker: origin phrase on the tracker row (#745)", () => { it("shows a short phrase for a recognised origin", () => { const tracker = makeTracker([job({ id: "j1", title: "Shared job", origin: "shared" })]); diff --git a/src/components/features/JobTracker.tsx b/src/components/features/JobTracker.tsx index 5bc95105..985843aa 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 { inheritedLetterForJob } from "../../lib/letters/resolve-letter.ts"; +import { deriveCompanyKey } from "../../lib/storage/company-key.ts"; import { useJobDuplicates, type JobDuplicateSuggestion, @@ -87,6 +91,50 @@ 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. + * + * This surface asks the narrower "what would this job inherit", not the chain's + * "which letter applies" — the row's own drafts already reach it through + * `lettersById` — so it calls the entry that starts one rung down. See + * {@link inheritedLetterForJob} for why asking the wide question and dropping + * the `"job"` answer is not the same thing. + * + * The phrase is built here because this is the layer holding `job.company`; + * the dialogs downstream only need something to print. It is a lowercase + * FRAGMENT — the render sites that stand it alone capitalize it themselves + * (`scope-phrase.ts`), because the same phrase is embedded mid-sentence + * elsewhere and no one string can be right in both. + */ +function inheritedFor( + job: JobRecord, + letters: readonly LetterRecord[] | undefined, +): InheritedLetter | undefined { + if (!letters || letters.length === 0) return undefined; + // `inheritedLetterForJob`, NOT `resolveLetterForJob` with the `"job"` answer + // filtered out. The two are not the same question, and filtering answers the + // wrong one: `resolveLetterForJob` returns `"job"` exactly when some letter + // carries this job's id, which is exactly when `lettersById` is non-empty, + // which is what makes the indicator open the REVEAL rather than the editor. + // So the filtered form was `undefined` for precisely the jobs whose reveal + // opens — the inherited chip, the scope notice and "Customize for this job" + // could never render in the product, though their unit tests passed on a + // `letters` + `inherited` pair this component cannot produce (#767 review). + const resolved = inheritedLetterForJob(job, letters); + if (!resolved) 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 +163,30 @@ 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 `inheritedLetterForJob` + * 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", so it is only a truthful + * answer once {@link JobTrackerProps.lettersReady} is true. */ + standardLetter?: LetterRecord; + /** Whether the letter store has actually been read (#767 review). + * + * `standardLetter` is `standard[0]`, and `[0]` of a not-yet-loaded list is + * `undefined` — indistinguishable from "the user has none". The tracker + * early-returns on ITS OWN `ready` only, and `useJobLetters` resolves + * independently, so there is a window where the rows are on screen and the + * letters are not. Composing in that window writes a SECOND unscoped record, + * and since only `[0]` is ever surfaced the original becomes unreachable — + * the standard tier is the one that loses a record this way, because it is + * the one with a single window onto it. + * + * Defaults to `true` so a caller that passes `standardLetter` directly (every + * test, any future read-only view) is unaffected: the flag is about a live + * store read in flight, which a caller holding the record has already done. */ + lettersReady?: boolean; /** 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 +222,9 @@ export function JobTrackerSection({ | "ratings" | "hasResume" | "lettersById" + | "allLetters" + | "standardLetter" + | "lettersReady" | "onLettersChanged" | "duplicatesByJobId" | "onDismissDuplicate" @@ -171,6 +246,17 @@ 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. + // + // `ready` travels with it because that "nothing writes a second one" holds + // only AFTER the read lands: before it, `standard` is `[]`, `[0]` is + // `undefined`, and an unguarded button would offer to compose one. + standardLetter={letters.standard[0]} + lettersReady={letters.ready} onLettersChanged={letters.refresh} duplicatesByJobId={duplicates.byJobId} onDismissDuplicate={duplicates.dismiss} @@ -188,6 +274,9 @@ export function JobTracker({ resumeName, resumeOptions, lettersById, + allLetters, + standardLetter, + lettersReady = true, onLettersChanged, duplicatesByJobId, onDismissDuplicate, @@ -244,6 +333,35 @@ export function JobTracker({ // "only non-empty bucket is rejected" case is the single-bucket case of it. const anyOpenByDefault = groups.some(({ bucket }) => !isCollapsedByDefault(bucket)); + // One pass over the letter set for the whole library, not one per row per + // render. `inheritedLetterForJob` walks `allLetters` up to twice, 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 2) 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]); + + // Same reason, its own memo: `deriveCompanyKey` is a unicode regex replace, a + // split, a filter and an 11-entry suffix scan, and it was running per row per + // render directly under the memo above (#767 review). Keyed off `jobs` alone + // because the key is derived from `job.company` and owes nothing to the + // letter set — so a letter write does not recompute it. + const companyKeyByJobId = useMemo(() => { + const byId = new Map(); + for (const job of jobs) { + const key = deriveCompanyKey(job.company); + if (key !== undefined) byId.set(job.id, key); + } + return byId; + }, [jobs]); + if (!ready) return null; return ( @@ -262,6 +380,21 @@ 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`. + + Held back until the letter store has been read, rather than + rendered against an `undefined` that cannot yet be told from "the + user has none" — see `lettersReady`. Absent, not disabled: the + control appears once with the right label, instead of flickering + from a disabled "Write a standard letter" to "Edit standard + letter" and inviting a click at the wrong moment. */} + {lettersReady && ( + + )}