Give token and segment links ids and pick ties by rule (#430)

[alex-rawlings-yyc]

Part of #424; closes #430.

Link ids. Token and segment links now carry the id phrase links already had; it moves onto AnalysisLink. The reducers mint one for every new link, the PT9 converter writes pt9:tl:<tokenRef>:<n>, and the perf generator draws them from a seeded stream of their own, so they never shift the draws a tier's records come from. validateTextAnalysis reports duplicate link ids on every layer.

Old data. A saved project or inline draft holding a token or segment link with no id loads empty, with a log entry. A draft shard holding one is treated as unusable and loads empty, so books saved since keep their work.

Picks by rule. src/utils/pick-rules.ts holds the rules. An approval tie goes to the latest updatedAt, then the smallest id, for tokens and segments alike. A merge folds into the identical payload with the earliest createdAt, then the smallest id. The reducers apply them wherever they picked by list order: approval ties, held and stale links, find-or-create, and token and phrase merges. The read selectors and the concordance apply them too, so reads and writes agree. An approval naming a missing payload is dropped from a tie and the pick falls to the rest. A collapse that leaves a token two links keeps the approved one.

Cached perf datasets lack link ids and load empty; regenerate them with npm run perf:datasets.

---

This change is 

Summary by CodeRabbit

• Bug Fixes
  • Duplicate approved analyses are now resolved consistently: the most recently updated link takes priority, with ties settled by link ID.
  • Choosing between shared analysis data now follows consistent rules, reducing differences based on item order.
• Compatibility
  • Saved analyses containing links without IDs now load as empty, with a warning. This affects older projects and drafts that lack link IDs.
• Reliability
  • Imported and newly created analysis links now receive stable, distinct IDs.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3071b337-64a5-44b0-a624-3f6acf5af3fb

📝 Walkthrough

Token and segment analysis links now have required IDs. Selection among duplicate links and payloads follows shared deterministic rules. Project and draft loading handles stored analyses with ID-less token or segment links by logging a warning and returning empty analysis or draft.

Changes: Analysis link identity and selection

Layer / File(s)	Summary
Link identity contract and producers src/types/*, src/converters/pt9/analysisMerger.ts, perf/dataset/generate.ts, src/__tests__/*	AnalysisLink now provides the required link ID, and validation checks IDs across link layers. The Paratext 9 converter, dataset generator, and test fixtures provide link IDs.
Deterministic link and payload selection src/utils/pick-rules.ts, src/utils/concordance.ts, src/store/analysisSlice.ts, src/__tests__/utils/*, src/__tests__/store/analysisSlice.test.ts	Shared rules prefer links by latest updatedAt, then smallest ID, and payloads by earliest createdAt, then smallest ID. Reducers and selectors use these rules, and reducers prepare IDs for newly created links.
Legacy analysis loading and recovery src/services/projectStorage.ts, src/__tests__/services/projectStorage.test.ts	Project, draft, and shard loading logs a warning and substitutes empty analysis or draft when token or segment links lack IDs. Tests cover legacy data and import, editable-copy, and draft save/reload paths.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Suggested reviewers: imnasnainaec

Merge Risk: 🔵 Low · up to 14bd2

When stored analysis contains a dangling approval, the concordance can show an empty gloss and count the token as analyzed despite a valid competing approval. This is a bounded issue, but it should be fixed or explicitly accepted before merging.

Pre-merge checks: 5

Check name	Status	Explanation
Linked Issues check		The PR satisfies the coding requirements in #430. AnalysisLink.id is required for token, segment, and phrase links. Reducers generate IDs for new token and segment links. The PT9 converter generates…
Out of Scope Changes check		The changes stay within #430. Type changes, storage handling, PT9 ID generation, seeded performance-dataset IDs, pick-rule utilities, reducer and selector updates, concordance updates, and their tests…
Docstring Coverage		Docstring coverage is 81.48% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 81 functions across 30 files. (1 skipped: 1…
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the main changes: adding IDs to token and segment links and applying deterministic tie-breaking rules.

Full details: Linked Issues check

Explanation

The PR satisfies the coding requirements in #430. AnalysisLink.id is required for token, segment, and phrase links. Reducers generate IDs for new token and segment links. The PT9 converter generates pt9:tl:<tokenRef>:<count> IDs. validateTextAnalysis reports duplicate link IDs for all layers. Storage code detects ID-less token or segment links, logs a warning, and loads the affected project, draft, or shard as empty. Shared pick rules select approval links by latest updatedAt and smallest id, and payloads by earliest createdAt and smallest id. The rules are used by reducers, selectors, and concordance reads. The added tests cover order-independent picks, missing payloads, merges, PT9 save/reload, and old-data handling.

Full details: Out of Scope Changes check

Explanation

The changes stay within #430. Type changes, storage handling, PT9 ID generation, seeded performance-dataset IDs, pick-rule utilities, reducer and selector updates, concordance updates, and their tests directly support link IDs, deterministic picks, or old-data compatibility. The documentation correction and fixture updates support the same implementation. No unrelated change is demonstrated.

Full details: Docstring Coverage

Explanation

Docstring coverage is 81.48% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 81 functions across 30 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[alex-rawlings-yyc]

@alex-rawlings-yyc made 13 comments and resolved 13 discussions.
Reviewable status: 23 of 32 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

---

src/__tests__/services/projectStorage.test.ts line 2566 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

⛏️ Fake GUIDS in other tests end with 1, 2, ... (instead of a, b)

Fixed.

---

src/__tests__/utils/pick-rules.test.ts line 14 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

⛏️ Spread FIXTURE_STAMPS, as payload does; EARLIER is its createdAt.

  return {
    ...FIXTURE_STAMPS,
    id,
    analysisId: 'ta-1',

Done.

[imnasnainaec]

@imnasnainaec reviewed 6 files and all commit messages, and made 2 comments.
Reviewable status: 29 of 32 files reviewed, 2 unresolved discussions (waiting on alex-rawlings-yyc).

---

src/__tests__/store/analysisSlice.test.ts line 6990 at r3 (raw file):

  }

  it('gives each link a gloss creates an id of its own, though the links share a payload', () => {

⛏️ The "gives ... creates ..." syntax of the it sentences in this describe are hard to parse.

How about (e.g.) "gives each link created by a gloss..." or "gives each gloss-created link ..."?

---

src/utils/concordance.ts line 203 at r3 (raw file):

 * payload is missing from `analysesById` losing to one whose payload is present.
 */
export function approvedAnalysisByToken(

⛏️ Optional logic consolidation:

Same best-approved-link-per-key loop as selectApprovedIdByTokenRef (analysisSlice.ts:1988) and selectApprovedSegmentAnalysisBySegment (analysisSlice.ts:2479). All three keep winners plus a value map derived from it.

Suggest a helper beside preferredLink:

/** The link an approval tie picks for each key `keyOf` gives, skipping links it gives none. */
export function preferredLinkByKey<L extends AnalysisLink>(
  links: readonly L[],
  keyOf: (link: L) => string | undefined,
  prefers: (link: L, rival: L) => boolean = prefersLink,
): Map<string, L> {
  const winners = new Map<string, L>();
  links.forEach((link) => {
    const key = keyOf(link);
    if (key === undefined) return;
    const held = winners.get(key);
    if (!held || prefers(link, held)) winners.set(key, link);
  });
  return winners;
}

Here:

const hasPayload = (l: TokenAnalysisLink) => analysesById.has(l.analysisId);
const winners = preferredLinkByKey(
  links,
  (l) =>
    l.status === 'approved'
      ? approvalKey(l.token.tokenRef, normalizeSurfaceForm(l.token.surfaceText))
      : undefined,
  (l, held) => (hasPayload(l) !== hasPayload(held) ? hasPayload(l) : prefersLink(l, held)),
);
return new Map([...winners].map(([key, l]) => [key, l.analysisId]));

Orphan handling differs: this counts an approval to a missing record as approved (pinned by "reads an approval naming a missing record as unglossed"), while selectApprovedIdByTokenRef drops it. The concordance can show a token as analyzed that the editor shows as unanalyzed. Intended? If so, worth a line in the doc comment.

[alex-rawlings-yyc]

@alex-rawlings-yyc made 2 comments and resolved 2 discussions.
Reviewable status: 29 of 32 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

---

src/__tests__/store/analysisSlice.test.ts line 6990 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

⛏️ The "gives ... creates ..." syntax of the it sentences in this describe are hard to parse.

How about (e.g.) "gives each link created by a gloss..." or "gives each gloss-created link ..."?

Done: all five now use the "created by" form.

---

src/utils/concordance.ts line 203 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

⛏️ Optional logic consolidation:

Same best-approved-link-per-key loop as selectApprovedIdByTokenRef (analysisSlice.ts:1988) and selectApprovedSegmentAnalysisBySegment (analysisSlice.ts:2479). All three keep winners plus a value map derived from it.

Suggest a helper beside preferredLink:

/** The link an approval tie picks for each key `keyOf` gives, skipping links it gives none. */
export function preferredLinkByKey<L extends AnalysisLink>(
  links: readonly L[],
  keyOf: (link: L) => string | undefined,
  prefers: (link: L, rival: L) => boolean = prefersLink,
): Map<string, L> {
  const winners = new Map<string, L>();
  links.forEach((link) => {
    const key = keyOf(link);
    if (key === undefined) return;
    const held = winners.get(key);
    if (!held || prefers(link, held)) winners.set(key, link);
  });
  return winners;
}

Here:

const hasPayload = (l: TokenAnalysisLink) => analysesById.has(l.analysisId);
const winners = preferredLinkByKey(
  links,
  (l) =>
    l.status === 'approved'
      ? approvalKey(l.token.tokenRef, normalizeSurfaceForm(l.token.surfaceText))
      : undefined,
  (l, held) => (hasPayload(l) !== hasPayload(held) ? hasPayload(l) : prefersLink(l, held)),
);
return new Map([...winners].map(([key, l]) => [key, l.analysisId]));

Orphan handling differs: this counts an approval to a missing record as approved (pinned by "reads an approval naming a missing record as unglossed"), while selectApprovedIdByTokenRef drops it. The concordance can show a token as analyzed that the editor shows as unanalyzed. Intended? If so, worth a line in the doc comment.

Done. preferredLinkByKey sits beside preferredLink, and all three loops now use it. I left out the prefers parameter because no caller needs a custom rule anymore. The difference in orphan handling wasn't intended: like the editor, the concordance now drops an approval that names a missing record. That removed the payload tie-break, and the pinned test now asserts the approval is left out.

[imnasnainaec]

@imnasnainaec reviewed 7 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).