Editing a letter drops producer/resumeId: partial save through saveLetter is a full replace
Maintainers usually reply within 1 day
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 55/100
- Issue type
- Bug
- Clarity
- Mostly clear
- Activity status
- Active
- Tech stack
- typescript
Research direction
Start with src/lib/storage/crud.ts, the saveLetter implementation in letters.ts, and the save() path in LetterEditorDialog.tsx; compare the read-modify-write behavior in job-tracker.ts. Trace the existing letter tests and egress acknowledgement path, then implement and test the chosen update behavior, tombstone protection, label semantics, documentation correction, and unchanged insert path. Finish with npm run verify.
Written by the indexing model from the issue text.
Description
LetterEditorDialog writes a partial record through saveLetter, but the store does a full replace. Every LetterRecord field the dialog does not name is silently deleted on save — including producer, which is the field the letter egress-acknowledgement gate reads.
Surfaced while reviewing #906. The behaviour is pre-existing on main, not introduced by that PR, so it wants its own issue.
The mechanism
putRecordVia (src/lib/storage/crud.ts:184-205) is the body behind putRecord:
const existing = (await db.get(store, record.id)) as T | undefined;
const written = {
...record,
createdAt: existing?.createdAt ?? record.createdAt ?? now,
updatedAt: options.touch === false
? (record.updatedAt ?? existing?.updatedAt ?? now)
: now,
} as T;
await db.put(store, written);
existing contributes createdAt and updatedAt only. It is never spread. db.put replaces the whole value.
This is correct and deliberate — saveLetter's own docblock (letters.ts:47-67) states the resulting obligation on callers:
a stored letter may carry unknown extra keys the contract PRESERVED on import, and a housekeeping write that spreads a record back through here must not silently drop them
So the store's contract is "callers pass complete records". The bug is a caller that doesn't, plus a docblock asserting the store does the merging.
Where the merge is supposed to live
jobs gets this right, one layer up. updateJob (src/lib/job-tracker.ts:91) does the read-modify-write:
// job-tracker.ts:49
// `updateJob` spreads `{ ...existing, ...patch }`
resumes gets it right differently: saveResume (resumes.ts:30-38) names every field of ResumeRecord (id, filename, blob, parse), so there is nothing to drop. Fragile — adding a ResumeRecord field obliges updating saveResume — but correct today.
letters has neither. There is no updateLetter, and LetterEditorDialog calls the raw store wrapper with a partial:
// LetterEditorDialog.tsx — save()
await saveLetter({
...(letter?.id ? { id: letter.id } : {}),
...(jobId !== undefined ? { jobId } : {}),
...(companyKey !== undefined ? { companyKey } : {}),
body,
...(label.trim() ? { label: label.trim() } : {}),
});
LetterRecord also carries resumeId and producer. Neither is sent, so both are destroyed on every edit.
What is actually lost
| Field | Consequence |
|---|---|
producer |
The egress warning stops firing for that letter, permanently, on every surface. hasOutsideProducer in JobLetterIndicator reads letter.producer !== undefined; docs/cover-letter-contract.md §6 reads an absent producer as "written by offlinecv itself". One edit of an imported, producer-written letter relabels it as hand-typed and the disclosure is gone for good. |
resumeId |
The letter → saved-résumé link is dropped. clearLetterResumeLink exists as an explicit operation; this does it by accident. |
label |
Cannot be cleared. A blank label is sent as absent (...(label.trim() ? … : {})), so the old value survives. Read as a merge this looks intentional; read as a replace it is why emptying the field silently does nothing. |
| unknown extra keys | The ones saveLetter's docblock explicitly says must survive a round-trip. |
The docblock is wrong, and it is load-bearing
LetterEditorDialog.tsx:15-23 justifies the egress rule with a claim about storage semantics:
A letter written HERE carries no
producerblock, and that absence is meaningful rather than incidental […] an existing record's own provenance survives the edit untouched (saveLetterspreads the input over the stored record, so keys it does not name are preserved).
The parenthetical is false. This matters more than a stale comment normally would: it is the stated reason the dialog is allowed to send a partial record, so the wrong comment is what makes the bug look safe to the next reader.
Secondary: no tombstone guard on letters
letters is one of the two stores that write deletedAt (types.ts:23-40), because it replicates. jobs guards resurrection explicitly — per getJob's docblock, updateJob "throws rather than quietly resurrecting it by writing a patch over the tombstone".
saveLetter has no such guard, and a full replace omitting deletedAt clears the tombstone. Not reachable from the UI today (getAllLetters filters tombstones, so the dialog never holds a deleted letter), but the letter store is a public contract for out-of-tree producers (#711), and a producer holding a stale id would resurrect a deleted letter with no error. Worth closing in the same change, since the fix is the same read-modify-write.
Proposed fix
Mirror the jobs shape rather than patching the call site:
- Add
updateLetter(id, patch)in a letters domain layer (thejob-tracker.tsanalogue), doing{ ...existing, ...patch }, throwing on a missing-or-tombstoned id. - Point
LetterEditorDialog.save()at it for the revise path; keepsaveLetterfor the insert path (composing, and the start-from copy, which must not carry an id). - Correct the
LetterEditorDialogdocblock to say whatputRecordViaactually does — required whichever fix lands. - Decide
labelclearing deliberately: either sendlabel: undefinedexplicitly so it can be emptied, or document that a label is not clearable.
A one-line ...(letter ?? {}) spread in the dialog fixes the field loss and nothing else. Acceptable as a stopgap, but it leaves the tombstone gap and repeats the pattern the next letter writer will also have to remember.
Acceptance criteria
- Editing an existing letter preserves
producer,resumeId, and any unknown extra keys — asserted against a record carrying all three - After editing a producer-written letter, the egress acknowledgement still fires for it (the #906 behaviour, tested end-to-end rather than by inspecting the record)
- A write against a tombstoned letter id is refused rather than resurrecting the record
-
labelclearing behaves as whichever way step 4 decides, with a test pinning it - The
LetterEditorDialogdocblock no longer claimssaveLettermerges - The insert path still writes no
id(the start-from copy stays a copy — #767's whole model) -
npm run verifygreen
Out of scope
saveResume's name-every-field style. Correct today; a separate call if anyone wants it made robust to new fields.saveJob/captureJob. Already covered byupdateJob's merge at the domain layer.- Changing
putRecordViato merge. It is deliberately a replace —importAlland thetouch: falsehousekeeping writes depend on writing a record verbatim — and flipping it would silently change every store's write semantics.
- Dominant language
- TypeScript
- Stars
- 11
- Forks
- 4
- Avg merge
- 1d 5h
- Merged PRs (30d)
- 66
Getting set up
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from offlinecv/OfflineCV
-
chore gaal
Difficulty 2/5 1-3 hours Newbie friendliness 82/100
Maintainers usually reply within 1 day
-
documentation
Difficulty 1/5 Under an hour Newbie friendliness 92/100
Maintainers usually reply within 1 day
-
gaal improvement ux:job-search
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
Maintainers usually reply within 1 day
-
documentation
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
Maintainers usually reply within 1 day
-
documentation
Difficulty 1/5 Under an hour Newbie friendliness 92/100
Maintainers usually reply within 1 day
All issues in offlinecv/OfflineCV
Similar issues
-
resources
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
railmapgen/rmg-palette#2445 ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
danielmiessler/LifeOS#2242 ·
Maintainers usually reply within 5 days
-
good first issue hacktoberfest help wanted translation
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
callstackincubator/appduct#129 ·
Maintainers usually reply within 1 day
-
Difficulty 1/5 1-3 hours Newbie friendliness 88/100