Distinguish same-gloss suggestions by their morpheme breakdown - #271
Distinguish same-gloss suggestions by their morpheme breakdown#271alex-rawlings-yyc wants to merge 2 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe suggestion engine now disambiguates same-gloss analyses by carrying morpheme breakdowns. The dropdown displays these breakdowns and includes them in accessible labels. Localization wiring and tests cover the new behavior. ChangesSuggestion breakdown disambiguation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized UI change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Analysis as glossedSuggestionEntries
participant Chip as TokenChip
participant Dropdown as SuggestionDropdown
Analysis->>Chip: provide entries with optional breakdown
Chip->>Dropdown: pass breakdownLabelTemplate
Dropdown->>Dropdown: append formatted breakdown to aria-label
Dropdown->>Dropdown: render muted suggestion-breakdown text
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
307fb85 to
eec6c1e
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec made 2 comments.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on alex-rawlings-yyc).
src/utils/suggestion-engine.ts line 204 at r1 (raw file):
* differ — one a breakdown cannot separate stays bare. */ export function glossedSuggestionEntries(
⛏️ Suggested rewrite of the entire unwieldy glossedSuggestionEntries JSDocs:
/**
* Flattens the merged per-token read into the entries the gloss UI renders, in rank order, keeping
* only those with a non-blank gloss in the active language.
*
* Single home of suggestion-presentation policy, so every surface offers the same rows rather
* than re-deriving any of it from row position; {@link GlossedSuggestionEntry} documents what each
* row carries. At most one entry is `'suggested'`, and often none (an approved token offers only
* promotions), so read `status` rather than assuming the first row is the accept row.
*/
src/utils/suggestion-engine.ts line 239 at r1 (raw file):
) .map((entry) => entry.gloss), );
⛏️ Possible replacement for ambiguousGlosses:
// Built from post-filter rows, so membership tracks what's actually on screen. Keyed per gloss:
// once two rows sharing a gloss break down differently, every row with that gloss is annotated.
const glossesSplitByBreakdown = new Set(
glossed
.filter((entry) =>
glossed.some((other) => other.gloss === entry.gloss && other.breakdown !== entry.breakdown),
)
.map((entry) => entry.gloss),
);
And a test could be added to go with it:
it('annotates all three rows when a same-gloss pair is contested by a third breakdown', () => {
// The duplicate is the point: unlike the pair above, this annotation marks both off from the third.
const entries = glossedSuggestionEntries(
{
status: 'suggested',
suggested: parsed('t1', 'ran', ['run', 'PST']),
candidates: [parsed('t2', 'ran', ['run', 'PST']), parsed('t3', 'ran', ['ran'])],
},
'en',
);
expect(entries).toEqual([
{ id: 't1', gloss: 'ran', status: 'suggested', breakdown: 'run PST' },
{ id: 't2', gloss: 'ran', status: 'candidate', breakdown: 'run PST' },
{ id: 't3', gloss: 'ran', status: 'candidate', breakdown: 'ran' },
]);
});
Two analyses of one surface form can share a gloss but differ in how they break the token down, which rendered as identical dropdown rows offering no way to choose. Rows now carry their breakdown, but only where the breakdowns actually differ — annotating rows a breakdown cannot separate adds noise without resolving anything.
eec6c1e to
6b3f039
Compare
Drop the redundant self-comparison guard from the collision filter and rename the set for what it holds. Move the status-ordering and approved-exclusion notes into the function's doc comment, where they stand alone on hover, rather than repeating them inline.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 2 comments.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on imnasnainaec).
src/utils/suggestion-engine.ts line 204 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Suggested rewrite of the entire unwieldy
glossedSuggestionEntriesJSDocs:/** * Flattens the merged per-token read into the entries the gloss UI renders, in rank order, keeping * only those with a non-blank gloss in the active language. * * Single home of suggestion-presentation policy, so every surface offers the same rows rather * than re-deriving any of it from row position; {@link GlossedSuggestionEntry} documents what each * row carries. At most one entry is `'suggested'`, and often none (an approved token offers only * promotions), so read `status` rather than assuming the first row is the accept row. */
Taken, with one thing added back.
Your opening and the "read status rather than assuming the first row is the accept row" line are both in — the latter is caller-facing advice the previous version only implied.
Kept two facts your rewrite drops. That status is assigned after blank picks are filtered: it's what makes the fall-through correct, and it isn't recoverable from hasAccept && index === 0 alone. And the approved-token self-exclusion, which is what the filter((a) => a.id !== resolved.analysis.id) branch implements. Folded the second into the ordering paragraph rather than restoring my original sentence, so that paragraph now covers both post-filter behaviors.
Knock-on: pulling those up into the doc comment made two inline comments in the body redundant — one restating the exclusion, one restating the ordering verbatim — so both are deleted.
src/utils/suggestion-engine.ts line 239 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Possible replacement for
ambiguousGlosses:// Built from post-filter rows, so membership tracks what's actually on screen. Keyed per gloss: // once two rows sharing a gloss break down differently, every row with that gloss is annotated. const glossesSplitByBreakdown = new Set( glossed .filter((entry) => glossed.some((other) => other.gloss === entry.gloss && other.breakdown !== entry.breakdown), ) .map((entry) => entry.gloss), );And a test could be added to go with it:
it('annotates all three rows when a same-gloss pair is contested by a third breakdown', () => { // The duplicate is the point: unlike the pair above, this annotation marks both off from the third. const entries = glossedSuggestionEntries( { status: 'suggested', suggested: parsed('t1', 'ran', ['run', 'PST']), candidates: [parsed('t2', 'ran', ['run', 'PST']), parsed('t3', 'ran', ['ran'])], }, 'en', ); expect(entries).toEqual([ { id: 't1', gloss: 'ran', status: 'suggested', breakdown: 'run PST' }, { id: 't2', gloss: 'ran', status: 'candidate', breakdown: 'run PST' }, { id: 't3', gloss: 'ran', status: 'candidate', breakdown: 'ran' }, ]); });
Both applied as written.
Dropping the otherIndex !== index guard is safe: a row can't satisfy other.breakdown !== entry.breakdown against itself, so the self-comparison was inert. glossesSplitByBreakdown is the better name too — the set holds glosses that need annotating, not glosses that are ambiguous.
On the test: it passes against the pre-existing implementation, so it's pinning behavior rather than catching a bug — but it covers a shape nothing else did, since every existing case has distinct breakdowns per row. Worth being explicit that it locks in t1 and t2 rendering identically: the annotation marks them off from t3 without separating them from each other, which is the opposite of what the two-row same-breakdown case does. Kept your comment making that point, with "the pair above" swapped for an explicit referent — the test directly above it is now the blank-in-active-language case.
2 participants
Closes #213
Two analyses of one surface form can share a gloss while differing in how they break the token down. Both are legitimately distinct payloads —
analysesAreIdenticalcounts morphemes, part of speech, features, and lexicon sense as identity — so both land in the same pool bucket and render as identical dropdown rows, offering no way to choose between them.The data was already reaching the UI and being discarded.
glossedSuggestionEntriesprojected each ranked analysis down to{ id, gloss }, dropping the morphemes one line before the rows were built.Approach
Rows now carry a
breakdown, but only where it resolves something: the gloss is contested by a differing breakdown. Two consequences worth knowing:The policy lives in
glossedSuggestionEntriesrather than the component, since that function's doc declares it the single home of suggestion-presentation policy, andTokenChip's memoization depends on the entry list being derived there.The breakdown is also appended to each row's
aria-labeland the visible span isaria-hidden. Without the label change the rows would still sound identical — the same bug, just moved off-screen.Not covered
Same-gloss rows differing only by part of speech, features, or lexicon sense remain indistinguishable. Fixing that means rendering those fields in a dropdown row, which is a wider UI question than this issue asked for — worth a follow-up if wanted.
Testing
1929 tests pass (baseline 1921; 6 engine + 2 component tests added). Coverage stays at 100% across statements, branches, functions, and lines, with no new ignore comments.
Verified the tests fail against the unfixed source: stashing only the two source files fails exactly the 4 tests asserting new behavior, while the negative cases correctly hold both ways.
This change is
Summary by CodeRabbit
New Features
Localization
Tests