feat(letters): job → company → standard resolution, picker, and customize-from - #906
feat(letters): job → company → standard resolution, picker, and customize-from#906Samhit21 wants to merge 1 commit into
Conversation
Deploying offlinecv with
|
| Latest commit: |
1564070
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b6823bbe.offlinecv.pages.dev |
| Branch Preview URL: | https://feat-letter-tiering-ui-issue.offlinecv.pages.dev |
rohithgollapalli
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES — 2 Blocking, 2 Secondary, 3 Nits. The chain, the copy model, and the glyph invariant are all right and well tested; both blockers are about what the new surfaces reach past.
Gates run: typecheck OK, lint OK, build OK, fallow audit --base origin/main exit 0 (dead code 0, complexity 0, duplication warn-only), targeted suites 67/67 OK, full suite 6391 passed / 4 failed — all four in src/lib/jd-extract/schema-org*.test.ts, all Intl thousands-grouping under an en-IN machine locale ('USD 1,50,000' vs 'USD 150,000'), untouched by this diff and not attributable to it. Fixture-PII gate skipped: no fixture binaries in the diff.
AC checklist for #767: 11 of 11 met, each with a direct assertion. Verified by reading the tests, not the checkboxes.
Blocking
1. An inherited letter reaches the screen without the egress acknowledgement. JobLetterIndicator.open() computes mustWarn from hasOutsideProducer(letters) — this job's own letters — but the diff adds two routes that render an inherited letter's body, and neither consults its producer:
- Reveal, then the inherited chip:
selected.bodyrenders (LetterRevealDialog.tsx:194). - Editor, then
startFromLetter: copiesoption.letter.bodyinto the visible textarea (LetterEditorDialog.tsx:194).
Failure, concretely: standard letter carries producer: { contract: 1, producer: "claude-code-letter-skill" }; the ack has never been recorded (fresh browser, or private mode — letter-egress-ack.ts fails silent to always ask again). Job row has one hand-typed letter, no producer. Click the glyph, mustWarn === false, reveal opens, click "Your standard letter" — outside-produced text on screen, warning never shown. The second path is worse: a job with no own letters takes open()'s early return straight to the editor, which never consults the ack at all, so one click on the picker chip puts the same text in the textarea.
JobLetterIndicator.test.tsx:9 states the contract this breaks: "the egress acknowledgement gates exactly [a letter an outside producer wrote]". That is now false for a letter reached by inheritance.
Fix — gate on everything the indicator can surface, keeping the fresh-read discipline the letter-egress-ack.ts docblock insists on:
function open() {
const outside =
hasOutsideProducer(letters) ||
(inherited !== undefined && inherited.letter.producer !== undefined);
if (outside && !hasAcknowledgedLetterEgress()) {
setStage("ack");
return;
}
reveal();
}
function reveal() {
if (!hasLetters) {
setEditing(undefined);
setSeed(undefined);
setStage("edit");
return;
}
setStage("reveal");
}
function acknowledge() {
recordLetterEgressAcknowledged();
reveal();
}Plus a test per path: a producer-bearing inherited letter alongside a clean own letter, and a producer-bearing inherited letter with no own letters at all.
2. The description and a code docblock both claim a company-letter entry point that does not exist. The body says "Company letters get no separate entry point, deliberately: one is created by customizing from a job row, where the company is already known." Nothing in the tree does that. Grep for companyKey reaching LetterEditorDialog:
src/components/features/LetterEditorDialog.test.tsx:208: companyKey="northwind"
— the test file, and nothing else. The reveal offers only Customize for this job (LetterRevealDialog.tsx:215), which composes with jobId. So a user cannot create a company letter at all, and TITLES.company, STORAGE_LINE.company, and scopeOf's company branch are unreachable in the shipped bundle. The scope: "company" rung of the chain can only ever fire for a record written by an outside producer or restored from a backup — a third of the feature named in the PR title has no write path.
That #767's own step 5 says the same thing is where this came from, but its step 4 specifies only "Customize for this job", so the issue is internally inconsistent and this PR resolved it silently in the narrower direction. The same claim is repeated as a code comment at StandardLetterButton.tsx:23-26, where it outlives the PR page and reads as a description of a shipped path.
Either resolution is fine, but pick one before merge — Closes #767 means nothing reopens it:
- Add the affordance: a second button in the reveal,
Customize for this company, passingderiveCompanyKey(job.company)and nojobId, shown when the key is defined.LetterEditorDialogalready handles the scope end to end. - Or amend the body and the
StandardLetterButtondocblock to say the company tier is read-only in this change, and file the follow-up now.
Secondary
3. Re-clicking the picker silently discards typed work. startFromLetter (LetterEditorDialog.tsx:193) does an unconditional setBody(option.letter.body), and the picker stays mounted after a pick — offers is gated on letter === undefined, not on whether a seed already happened, so it also renders in the Customize-for-this-job flow where seededFrom is already set on open. A user who customizes, writes 500 words, and mis-clicks the still-present "Your standard letter" ghost button loses all of it: the textarea value is replaced, and a controlled <textarea> has no undo across a React re-render. Either hide the picker once seededFrom !== undefined, or confirm before overwriting a body that differs from the source.
4. inheritedFor re-scans the whole letter set for every row on every render. JobTracker.tsx:391 calls it inside the row .map(), and resolveLetterForJob walks allLetters up to three times per call, with no memo anywhere on the path. Every keystroke in an EditableField and every status-filter toggle re-runs the whole thing — O(jobs x letters x 3). A useMemo over [jobs, allLetters] producing a Map<string, InheritedLetter> is a one-line change and keeps inheritedFor exactly as written.
Nits — non-blocking
5. open()'s !hasLetters early return is the one route into the editor that does not setSeed(undefined); onEdit and onCompose both do. Unreachable today (a seed can only be set from the reveal, which only opens when hasLetters), so this is about not leaving the invariant to an argument. The fix in finding 1 covers it.
6. LetterRevealDialog passes the source through — onCustomize?.(selected) — but JobLetterIndicator's handler ignores the argument and reaches for startFrom[0]. Same record today; taking the argument keeps it true if a second inherited entry is ever offered.
7. capitalize() uppercases only the first character of the caller's phrase, so a company the user typed lowercase renders as "Your northwind letter" in the picker chip. Cosmetic, and the alternative (title-casing free text) is worse — worth a line in the docblock saying the phrase is echoed as typed.
Description accuracy (gate 3f)
Accurate on the chain, the glyph invariant, the copy model, and the seed-on-open nuance — the "one nuance worth flagging" paragraph is exactly the kind of self-disclosure that makes a body worth reading. One overclaim (finding 2). The verification line checks out apart from the four locale-dependent jd-extract failures noted above, which are not this PR's.
No fixes were pushed and the branch was not collapsed: the auto-fix path requires 0 Blocking findings. The branch already holds exactly one commit, so the one-commit invariant is intact. No suggestion blocks — every non-blocking finding here is behavioural, and a behavioural change applied by a one-click button is a change nobody reviewed.
Reviewed at a8369016fad2b61c36de3d354635b593a8ac1d51.
Reviewed by: Claude Opus 5 (high)
| // 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 |
There was a problem hiding this comment.
Blocking (1/2) — the egress acknowledgement does not cover what this exposes.
This is where an inherited letter enters the component's surfaces, but open() below still computes mustWarn from hasOutsideProducer(letters) — this job's own letters only. Two new paths render an inherited letter's body without ever reading its producer:
- reveal, then the inherited chip:
LetterRevealDialog.tsx:194 - editor, then
startFromLetter:LetterEditorDialog.tsx:194
Standard letter with producer: { contract: 1, producer: "claude-code-letter-skill" }, ack never recorded (fresh browser, or private mode — letter-egress-ack.ts fails silent to always ask again), job row with one hand-typed letter: mustWarn === false, reveal opens, one click on "Your standard letter" puts outside-produced text on screen with no warning. A job with no own letters is worse — open() returns early to the editor, which never consults the ack at all.
JobLetterIndicator.test.tsx:9 says the ack "gates exactly" a letter an outside producer wrote. That is now false for an inherited one. Fix in the review body — it also picks up nit 5 (this early return is the one editor route that doesn't clear seed).
There was a problem hiding this comment.
Fixed in 9a87260. Confirmed the bug exactly as described — both paths, and the no-own-letters one is indeed the worse of the two.
open() now gates on everything the component can surface:
const exposesOutsideProducer =
hasOutsideProducer(letters) || inherited?.letter.producer !== undefined;
if (exposesOutsideProducer && !hasAcknowledgedLetterEgress()) {
setStage("ack");
return;
}
reveal();Took your open/reveal/acknowledge split as suggested — the ack path hard-coding "reveal" was the second half of the bug, correct only while the empty case could never warn, and sharing reveal() makes the post-warning destination structurally identical to the no-warning one. That also picks up nit 5: compose now goes through one openEditor(scope, from?) helper that always clears editing and sets the seed and scope together, so the early return can't skip a reset.
Three tests, and I mutation-tested them rather than trusting they bite — reverting just the || inherited?.letter.producer !== undefined clause fails exactly the two that should:
× warns before revealing, when only the INHERITED letter came from outside
× warns before the EDITOR too, when the job has no letters of its own
Plus the negative direction (hand-typed own + hand-typed inherited must NOT warn), since the whole point of the gate is that it doesn't tell the user something untrue about their own typing.
| * `LetterEditorDialog` with no `jobId` and no `companyKey`, which is exactly | ||
| * the shape that writes a letter with neither scope key. | ||
| * | ||
| * Company letters get no equivalent entry point, deliberately: one is created |
There was a problem hiding this comment.
Blocking (2/2) — this describes a path that does not exist.
Nothing in the tree creates a company letter. companyKey reaches LetterEditorDialog from exactly one place:
src/components/features/LetterEditorDialog.test.tsx:208: companyKey="northwind"
The reveal offers only Customize for this job (LetterRevealDialog.tsx:215), which composes with jobId. So TITLES.company, STORAGE_LINE.company and scopeOf's company branch are unreachable in the shipped bundle, and scope: "company" can only fire for a record an outside producer wrote or a backup restored.
The PR body repeats the claim, and #767's own step 5 is where it came from — but step 4 specifies only "Customize for this job", so the issue is internally inconsistent and this resolved it silently in the narrower direction. A comment is the worse of the two places to leave it: it outlives the PR page.
Either add the affordance (a second reveal button passing deriveCompanyKey(job.company) and no jobId — the editor already handles the scope end to end), or amend this docblock and the body to say the company tier is read-only here, and file the follow-up now. Closes #767 means nothing reopens it.
There was a problem hiding this comment.
Fixed in 9a87260 — took the first option and added the affordance, so this docblock now describes something real.
You were right that the claim was mine to check and I hadn't. LetterRevealDialog gains a companyOffer, rendered as "Customize for this company" whenever the job has a derived key. JobLetterIndicator takes a companyKey prop, tracks a composeScope, and passes exactly one scope key to the editor:
jobId={composeScope === "company" ? undefined : jobId}
companyKey={composeScope === "company" ? companyKey : undefined}One deviation from your sketch, and I think it's the better shape: the offer is not limited to the inherited entry. It's available for the job's own drafts too, because "I wrote this for one posting and want it for every job at this employer" is how a company letter actually comes to exist — restricting it to inherited letters would mean you could only create a company letter if you already had a standard one to descend from.
deriveCompanyKey(job.company) is computed in JobTracker and threaded down, so a job with a blank company simply has no key and gets no offer — which matches the chain's own rule about never matching a blank key.
Three tests: the offer appears with a key, is absent without one, and opens the editor titled "Write a company letter" with the source body seeded and no jobId.
| rated={ratings !== null} | ||
| rating={ratings?.get(job.id)} | ||
| letters={lettersById?.get(job.id)} | ||
| inherited={inheritedFor(job, allLetters)} |
There was a problem hiding this comment.
Secondary — this re-scans every letter for every row on every render.
resolveLetterForJob walks allLetters up to three times per call, and this sits inside the row .map() with no memo on the path. Every keystroke in an EditableField and every status-filter toggle re-runs the whole thing: O(jobs x letters x 3).
A useMemo over [jobs, allLetters] producing a Map<string, InheritedLetter> fixes it without touching inheritedFor, which is well factored as written.
There was a problem hiding this comment.
Fixed in 9a87260. inheritedFor is untouched, exactly as you suggested — the memo wraps it:
const inheritedByJobId = useMemo(() => {
const byId = new Map<string, InheritedLetter>();
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]);Row lookup is inheritedByJobId.get(job.id). Deps hand-audited both directions per CLAUDE.md's note that exhaustive-deps isn't enforced here: jobs and allLetters are the only values read, inheritedFor is module-scope, and both come from hooks that hand back a new array only on an actual store change — so this recomputes on a letter or job change and not on an EditableField keystroke.
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => onCustomize?.(selected)} |
There was a problem hiding this comment.
Nit — the source is passed here, but JobLetterIndicator's handler ignores the argument and reaches for startFrom[0] instead. Same record today; taking source keeps this correct if a second inherited entry is ever offered.
There was a problem hiding this comment.
Fixed in 9a87260 — the handler now takes source and uses it:
onCustomize={(source) =>
openEditor("job", { letter: source, label: labelFor(source) })
}Needed a labelFor(source) helper alongside it, since the copy notice names what it copied from and the answer now depends on which record came through: the inherited entry has a scope phrase, one of the job's own drafts has only its user-set label. That fell out of the company-offer work too, where the source really can be a different record from startFrom[0] — so this stopped being hypothetical in the same commit.
a836901 to
9a87260
Compare
Fix-up pushed —
|
| # | Severity | Resolution |
|---|---|---|
| 1 | Blocking | Egress gate now covers the inherited letter; open/reveal/acknowledge split as suggested |
| 2 | Blocking | Added the affordance — "Customize for this company" is now the company tier's write path |
| 3 | Secondary | Picker retires once a seed is taken |
| 4 | Secondary | useMemo over [jobs, allLetters] → Map<string, InheritedLetter> |
| 5 | Nit | Covered by 1's restructure — one openEditor(scope, from?) sets all three pieces of compose state |
| 6 | Nit | Handler takes source; needed a labelFor helper, which stopped being hypothetical once 2 landed |
| 7 | Nit | Behaviour kept, reasoning documented (title-casing free text mangles "eBay"/"iRobot") |
On finding 2 — I took the "add it" branch, with one deviation. Your sketch scoped the new button to the inherited entry; I made it available for the job's own drafts too. Restricting it to inherited letters would mean a company letter is only creatable if you already have a standard one to descend from, and "I wrote this for one posting and want it for every job at this employer" is the path that actually produces one. TITLES.company, STORAGE_LINE.company and scopeOf's company branch are all live in the shipped bundle now, and scope: "company" can fire for a user-authored record.
On finding 1 — I mutation-tested rather than assuming the new tests bite. Reverting only the || inherited?.letter.producer !== undefined clause fails exactly the two cases it should:
× warns before revealing, when only the INHERITED letter came from outside
× warns before the EDITOR too, when the job has no letters of its own
The negative direction is covered too — a hand-typed own letter plus a hand-typed inherited one must not warn, since the gate's whole purpose is not telling the user something untrue about their own typing.
Verification. npm run verify green end to end: 384 test files, 6404 passed (9 new), 10 skipped, build clean, fallow dead code 0 · complexity 0 · duplication warn-only, exit 0. Your four jd-extract/schema-org*.test.ts failures do not reproduce here — consistent with your en-IN locale diagnosis.
Still one commit; the branch was amended, not appended. Re-requesting review.
s-annam
left a comment
There was a problem hiding this comment.
Careful, well-argued work — the pure chain in resolve-letter.ts is the right shape, its 11 cases cover ordering and non-matching properly, and the reuse decisions match the issue's analysis. Two blockers, though, and the first one is structural: the reveal's entire inherited half cannot execute in production, so two acceptance criteria are met only in unit tests.
Blocking
1. JobTracker.tsx:113 — inherited is always undefined by the time the reveal opens, so ACs 6 and 7 are unmet in the product. inheritedFor discards scope === "job", and resolveLetterForJob answers "job" exactly when some letter carries this job's id — which is exactly when lettersById.get(job.id) is non-empty, which is hasLetters, which is what makes reveal() open the reveal instead of the editor. The two predicates are exact complements. Verified end-to-end against the real JobTracker: with a job that owns a letter and a standard letter present, the opened reveal contains no scope notice, no inherited chip, no "Customize for this job", and not the inherited body. Detail and fix inline.
2. StandardLetterButton.tsx:71 — the new panel button shows outside-produced letter text with no egress acknowledgement. JobLetterIndicator.open() gates any body carrying a producer block, and this PR deliberately widened that gate to inherited letters. The new button hands letter straight to LetterEditorDialog, which has no gate. Reproduced: an imported standard letter with a producer block renders in full, with nothing written to letter-egress-ack.
Secondary
StandardLetterButton.tsx:74—saveLetteris a full replace, not a merge (crud.ts:182-190spreads only the input), so editing dropsproducerandresumeId. The docblock claiming otherwise is pre-existing onmain, but this new surface is the one most likely pointed at a producer-written record — and strippingproducerpermanently disables blocker 2's warning for that letter everywhere.JobLetterIndicator.tsx:333— "Customize for this company" always forks a new record; the company tier has no edit path, and the button is offered even when the selected letter already is that company's letter.JobTracker.tsx:225—letters.standard[0]is read with noletters.readygate, so a click inside the load window writes a second standard letter and orphans the first. This falsifies the new comment directly above it.- Three feature components pushed past the ~200 LOC guideline with no decomposition:
LetterEditorDialog178 → 337,LetterRevealDialog179 → 267,JobLetterIndicator234 → 363. Being fair about it: stripping comments and blanks gives 174 / 162 / 197, all under the line, andfallowreports 0 complexity findings — so the substance behind the rule is not breached. But the repo's own cited numbers are rawwc -l(ModelSelector556,ReconstructedRole490 both match exactly), and CLAUDE.md's instruction for a file already over is to extract into a sibling rather than grow it.StandardLetterButtonwas extracted; these three were not. - Description omissions (gate 3f). Every checkable claim in the body is accurate — I checked all eleven ACs against real assertions, and
fallow … exit 0holds. Two omissions: the body never mentions the egress gate it extended (nor the new surface that bypasses it), and "Company letters get no separate entry point, deliberately" omits that the tier consequently has no edit path and that re-customizing stacks records.
Nits
JobLetterIndicator.tsx:161— the scope phrase's casing is inverted between its two slots: capitalized where it lands mid-sentence, lowercase where it is a standalone chip.JobTracker.tsx:299— the newuseMemois wedged between a pre-existing comment and theanyOpenByDefaultit documents. One-click suggestion inline.JobTracker.tsx:408—deriveCompanyKey(job.company)runs inline per row per render, directly below a memo added to avoid exactly that.resolve-letter.ts:31—LetterScopeandResolvedLetterare exported with no importer anywhere in the tree.
Gates
| Gate | Result |
|---|---|
| 3a fixture PII | skipped — no fixture binaries touched |
| 3b design system / reuse | pass — no raw interactive elements added; StandardLetterButton carries a written Reuse analysis. The raw <input> in LetterEditorDialog is pre-existing on main |
| 3c style tokens | pass — no hex, no raw palette class, no manual dark: variant |
| 3d fallow | pass (report-only) — dead code 0, complexity 0; the 12 duplication groups are inherited, incl. LetterRevealDialog.test.tsx:156-182, which predates this PR |
| 3e skill/script command bugs | skipped — no scripts/ or .claude/ files touched |
| 3f description accuracy | accurate; two omissions (above) |
npm run verify |
green locally on 9a87260 — typecheck, eslint, check:nul, check:fixtures, check:baselines, check:core, tests, build. CI verify + fallow also green |
AC checklist (#767): 9 of 11 met. ACs 6 ("the reveal names the scope whenever the letter is inherited") and 7 ("Customize for this job" writes a new record) have direct unit assertions but are unreachable in the product via blocker 1 — their tests pass letters=[own] together with inherited, a combination JobTracker cannot produce. JobTracker.test.tsx covers none of allLetters / inherited / standardLetter, which is why nothing caught it. AC 11 confirmed: JobLetterIndicator.test.tsx has 0 deleted lines.
Verdict rule: ≥1 Blocking → REQUEST_CHANGES. Nothing was committed or pushed to your branch; the two blockers need behavioural changes, which are yours to make. Your branch is still one commit — please keep it that way when you push the fix.
Reviewed by: Claude Opus 5 (high)
…-from (#767) 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.
9a87260 to
1564070
Compare
Closes #767. Part of #765, and the consumer of the lattice #766 stored — both merged, so this branches straight off
main.The resolution chain
src/lib/letters/resolve-letter.ts(new) — pure, no React, no storage access:Job → company → standard, first hit wins. The
scopeis not decoration: every surface below needs it to tell the user why they're looking at this text. Three properties beyond the happy path, each with its own test:companyskips the company rung rather than matching an empty key —deriveCompanyKeyanswersundefinedfor""," "and" , ".The row glyph is unchanged
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 didn't intend for it. The glyph still counts the job's own letters only, asserted directly.
Inheritance surfaces inside the dialogs, where there's room to name it:
Customize-from is a copy, permanently
The editor carries no
idfrom its starting point, sosaveLetterinserts rather than upserting over the source. That's the whole model — a live link would mean editing job B's letter rewrote the standard letter already submitted for job A, and for prose there's no merge that makes that safe (#765; it's why letters ship before résumés). The dialog says so at the moment of copying, because "Started from your Northwind letter" alone reads as a link.One nuance worth flagging: "Customize for this job" does seed on open, via a separate
seedprop. That isn't a loophole in "never seeds automatically" — the pick happened, one dialog earlier. A plain "Write a cover letter" click still opens empty however many offers exist, and that's tested.The egress acknowledgement covers the new surfaces
JobLetterIndicatorwarns once, ever, before putting text an outside producer wrote on screen (#711). This PR widens what that gate sees and adds a second door behind it, so both are stated here rather than left to the diff:producertoo, not just this job's own letters. The warning is about egress that already happened to the text being shown, whichever scope holds it.StandardLetterButtonreaches the editor without going through a row. An outside-produced standard letter is the record shape Letter scope keys: optional jobId + companyKey (cover-letter contract v2) #766 created and a backup import lands, so the gate applies there too.One implementation, not two:
LetterEgressAckDialogowns the wording and the flag, and both surfaces call it. A hand-rolled second copy would drift in wording, and a surface that forgot to write the flag would re-ask a user who had already accepted.Reuse
Per the issue's analysis — extended
LetterEditorDialog(now authors all three scopes via optionaljobId/companyKey),LetterRevealDialog, andJobLetterIndicator. One new component:StandardLetterButton, because no existing surface owns a panel-level (not per-row) letter affordance, and folding it intoJobTrackerwould push a file already past the ~200 LOC guideline further past it. No new design-system primitive.Company letters get no panel-level entry point, deliberately: one is created — and now edited — by "Customize for this company" / "Edit this company letter" in the reveal, reached from a job row where the company is already known. A panel-level version would need a company picker for an app that has no company entity.
The tier is fully writable, but only from a row, and only for a job whose
companyderives a key. Two consequences worth stating rather than leaving to be discovered: a job with a blank company can neither create nor reach a company letter, and the offer is a fork except when the letter on screen already is that company's — where it edits in place, which is what stops it forking the letter from itself.Acceptance criteria
All eleven met, each with a direct assertion:
jobcompanystandardcompanynever matches a company letteraria-labeljobId; noidreachessaveLetteridis the mechanism)jobIdand saves with neither keyJobLetterIndicator.test.tsxpasses untouched — I added newdescribes and changed no existing assertion except the two the scope-phrase casing fix movedACs 6 and 7 now have
JobTracker-level coverage as well as unit coverage. They previously had only the latter, and it passed on aletters+inheritedpair the tracker cannot actually produce — see the first review-fix note below.Verification
npm run verifygreen on1564070— 384 test files, 6428 tests passed, 10 skipped, build clean,fallow audit --base origin/mainexit 0 (dead code 0, complexity 0, duplication 15 groups, warn-only). Also run by thepre-pushhook.Both blockers were mutation-checked rather than argued:
resolveLetterForJob+ drop the"job"answerJobTrackercases red, control greenStandardLetterButtonTwo notes:
click/typeBodyfromLetterEditorDialog.test.tsx. Extracted both into__test-utils__/dialog-dom.tsasclickButtonIn/typeIntoTextAreaand pointed both suites at them. ThetypeIntoTextAreadocblock records why the native-setter dance is needed, since a plain.value =is swallowed by React's value tracker and fails silently.JobTracker.test.tsx's three copies of a localclickButtonare now one at module scope, for the same reason.storage/index.tspredicting this change would addlettersForCompany/standardLetters/deriveCompanyKeyto the barrel. It didn't, and the comment now says why rather than sitting stale: the two readers still have no caller (every surface goes throughuseJobLetters, which reads the store once), andderiveCompanyKey's new caller reaches the zero-dep leaf directly rather than pullingbackup.ts+resumes.tsinto the letters chunk.Residual fallow duplication is warn-level and inherited (the
beforeEachshape shared across five hook suites, andLetterRevealDialog.test.tsx:156-182, which predates this PR); the gate passes.Not in this PR
#929 —
saveLetteris a full replace, soLetterEditorDialog.save()dropsproducerandresumeIdon every edit, and the docblock claiming otherwise is wrong. Pre-existing onmain; filed and scoped out during review, with the fix identified as a newupdateLetterrather than a change toputRecordVia. The "Edit this company letter" path added here inherits it, which is noted on the issue rather than half-fixed here.