Hacktoberfest 2026: the issues maintainers tagged for October, open and beginner-friendly. Browse Hacktoberfest issues

Editing a letter drops producer/resumeId: partial save through saveLetter is a full replace

Open
#929 0 comments 0 reactions 0 assignees View on GitHub

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

architecture bug gaal

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 producer block, and that absence is meaningful rather than incidental […] an existing record's own provenance survives the edit untouched (saveLetter spreads 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:

  1. Add updateLetter(id, patch) in a letters domain layer (the job-tracker.ts analogue), doing { ...existing, ...patch }, throwing on a missing-or-tombstoned id.
  2. Point LetterEditorDialog.save() at it for the revise path; keep saveLetter for the insert path (composing, and the start-from copy, which must not carry an id).
  3. Correct the LetterEditorDialog docblock to say what putRecordVia actually does — required whichever fix lands.
  4. Decide label clearing deliberately: either send label: undefined explicitly 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
  • label clearing behaves as whichever way step 4 decides, with a test pinning it
  • The LetterEditorDialog docblock no longer claims saveLetter merges
  • The insert path still writes no id (the start-from copy stays a copy — #767's whole model)
  • npm run verify green

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 by updateJob's merge at the domain layer.
  • Changing putRecordVia to merge. It is deliberately a replace — importAll and the touch: false housekeeping 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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from offlinecv/OfflineCV

All issues in offlinecv/OfflineCV

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.