Skip to content

feat(letters): job → company → standard resolution, picker, and customize-from - #906

Open
Samhit21 wants to merge 1 commit into
mainfrom
feat/letter-tiering-ui-issue-767
Open

feat(letters): job → company → standard resolution, picker, and customize-from#906
Samhit21 wants to merge 1 commit into
mainfrom
feat/letter-tiering-ui-issue-767

Conversation

@Samhit21

@Samhit21 Samhit21 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

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:

resolveLetterForJob(job, letters)  { letter, scope } | undefined

Job → company → standard, first hit wins. The scope is 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:

  • A blank company skips the company rung rather than matching an empty key — deriveCompanyKey answers undefined for "", " " and " , ".
  • A letter naming another job is never inherited, even at the same employer. A both-keyed record is refused by the contract but the store doesn't enforce it, and one that got in names a specific posting — matching it on its company key alone would show job B the letter written for job A.
  • Specificity beats recency. The rungs are ordered, not scored: a standard letter updated today does not outrank a company letter written last month.

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:

  • Reveal — the inherited letter is one more entry, always last, never what opens by default. Selecting it names the scope ("This is your standard letter, not a letter for this job") and swaps Edit for Customize for this job. Edit is gone, not joined: editing in place would rewrite a letter this job doesn't own.
  • Editor — a "Start from…" picker while composing, absent while revising, never seeding on its own.

Customize-from is a copy, permanently

The editor carries no id from its starting point, so saveLetter inserts 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 seed prop. 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

JobLetterIndicator warns 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:

  • Widened — the reveal's inherited entry and the editor's starting point both put another scope's body on screen, so the gate reads the inherited letter's producer too, not just this job's own letters. The warning is about egress that already happened to the text being shown, whichever scope holds it.
  • Second doorStandardLetterButton reaches 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: LetterEgressAckDialog owns 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 optional jobId/companyKey), LetterRevealDialog, and JobLetterIndicator. One new component: StandardLetterButton, because no existing surface owns a panel-level (not per-row) letter affordance, and folding it into JobTracker would 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 company derives 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:

  • Own letter wins over company + standard, scope job
  • No own letter → company letter, scope company
  • Only a standard letter available → scope standard
  • Empty company never matches a company letter
  • Row glyph reflects the job's own letters only — asserted on the aria-label
  • Reveal names the scope when inherited, says nothing extra when it's the job's own
  • Customize writes a new record with a new id and this job's jobId; no id reaches saveLetter
  • Editing a customized letter leaves company/standard untouched (the absent id is the mechanism)
  • Standard-letter button opens the editor with no jobId and saves with neither key
  • "Start from…" never seeds without a pick, absent while revising
  • Pre-existing JobLetterIndicator.test.tsx passes untouched — I added new describes and changed no existing assertion except the two the scope-phrase casing fix moved

ACs 6 and 7 now have JobTracker-level coverage as well as unit coverage. They previously had only the latter, and it passed on a letters + inherited pair the tracker cannot actually produce — see the first review-fix note below.

Verification

npm run verify green on 1564070384 test files, 6428 tests passed, 10 skipped, build clean, fallow audit --base origin/main exit 0 (dead code 0, complexity 0, duplication 15 groups, warn-only). Also run by the pre-push hook.

Both blockers were mutation-checked rather than argued:

mutation result
restore resolveLetterForJob + drop the "job" answer 3 of 4 new JobTracker cases red, control green
remove the egress gate from StandardLetterButton 2 of 5 new cases red

Two notes:

  • Test-harness extraction. Fallow flagged my new suite re-deriving click/typeBody from LetterEditorDialog.test.tsx. Extracted both into __test-utils__/dialog-dom.ts as clickButtonIn/typeIntoTextArea and pointed both suites at them. The typeIntoTextArea docblock 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 local clickButton are now one at module scope, for the same reason.
  • Stale note corrected. Letter scope keys: optional jobId + companyKey (cover-letter contract v2) #766 left a comment in storage/index.ts predicting this change would add lettersForCompany/standardLetters/deriveCompanyKey to 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 through useJobLetters, which reads the store once), and deriveCompanyKey's new caller reaches the zero-dep leaf directly rather than pulling backup.ts + resumes.ts into the letters chunk.

Residual fallow duplication is warn-level and inherited (the beforeEach shape shared across five hook suites, and LetterRevealDialog.test.tsx:156-182, which predates this PR); the gate passes.

Not in this PR

#929saveLetter is a full replace, so LetterEditorDialog.save() drops producer and resumeId on every edit, and the docblock claiming otherwise is wrong. Pre-existing on main; filed and scoped out during review, with the fix identified as a new updateLetter rather than a change to putRecordVia. The "Edit this company letter" path added here inherits it, which is noted on the issue rather than half-fixed here.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 26, 2026

Copy link
Copy Markdown

Deploying offlinecv with  Cloudflare Pages  Cloudflare Pages

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

View logs

@rohithgollapalli rohithgollapalli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.body renders (LetterRevealDialog.tsx:194).
  • Editor, then startFromLetter: copies option.letter.body into 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, passing deriveCompanyKey(job.company) and no jobId, shown when the key is defined. LetterEditorDialog already handles the scope end to end.
  • Or amend the body and the StandardLetterButton docblock 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/components/features/LetterEditorDialog.tsx
Comment thread src/components/features/JobTracker.tsx Outdated
rated={ratings !== null}
rating={ratings?.get(job.id)}
letters={lettersById?.get(job.id)}
inherited={inheritedFor(job, allLetters)}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/components/features/JobLetterIndicator.tsx Outdated
@Samhit21
Samhit21 force-pushed the feat/letter-tiering-ui-issue-767 branch from a836901 to 9a87260 Compare August 27, 2026 06:39
@Samhit21

Copy link
Copy Markdown
Collaborator Author

Fix-up pushed — 9a87260

All seven findings addressed. Both blockers were real bugs, and the second one was an overclaim I wrote without checking — thanks for grepping it rather than taking the body's word.

# 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.

Comment thread src/components/features/JobTracker.tsx Fixed

@s-annam s-annam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:113inherited 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:74saveLetter is a full replace, not a merge (crud.ts:182-190 spreads only the input), so editing drops producer and resumeId. The docblock claiming otherwise is pre-existing on main, but this new surface is the one most likely pointed at a producer-written record — and stripping producer permanently 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:225letters.standard[0] is read with no letters.ready gate, 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: LetterEditorDialog 178 → 337, LetterRevealDialog 179 → 267, JobLetterIndicator 234 → 363. Being fair about it: stripping comments and blanks gives 174 / 162 / 197, all under the line, and fallow reports 0 complexity findings — so the substance behind the rule is not breached. But the repo's own cited numbers are raw wc -l (ModelSelector 556, ReconstructedRole 490 both match exactly), and CLAUDE.md's instruction for a file already over is to extract into a sibling rather than grow it. StandardLetterButton was 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 0 holds. 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 new useMemo is wedged between a pre-existing comment and the anyOpenByDefault it documents. One-click suggestion inline.
  • JobTracker.tsx:408deriveCompanyKey(job.company) runs inline per row per render, directly below a memo added to avoid exactly that.
  • resolve-letter.ts:31LetterScope and ResolvedLetter are 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)

Comment thread src/components/features/JobTracker.tsx Outdated
Comment thread src/components/features/StandardLetterButton.tsx
Comment thread src/components/features/StandardLetterButton.tsx
Comment thread src/components/features/JobLetterIndicator.tsx
Comment thread src/components/features/JobTracker.tsx
Comment thread src/components/features/JobLetterIndicator.tsx Outdated
Comment thread src/components/features/JobTracker.tsx Outdated
Comment thread src/components/features/JobTracker.tsx Outdated
Comment thread src/lib/letters/resolve-letter.ts
…-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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Letter tiering UI: job → company → standard resolution, picker, and customize-from

4 participants