Fix ordering of words with special chars#916
Conversation
WalkthroughSmart quote characters are normalized during word sorting by removing them before locale-aware comparison. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
convert/convertReverseIndex.ts(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (2)
src/routes/lexicon/+page.svelte (1)
82-83: LGTM! Improved sorting approach.Using the pre-computed
letterfield is more efficient than deriving it from the first character of each word during every sort operation. This works correctly with the build-time letter assignment performed inconvertReverseIndex.ts.convert/convertReverseIndex.ts (1)
70-72: Verify whether the regex should include additional typographic characters.The current regex only removes double smart quotes (" and ") before sorting. Consider whether single smart quotes (' and '), em/en dashes (—/–), or other typographic punctuation should also be normalized to ensure consistent sort order across all potential input data.
88f14e8 to
92e813a
Compare
Instead of first char of display string
The chars are not removed from the string, only ignored for sorting during conversion process.
92e813a to
b15de39
Compare
|
Waiting for feedback on other characters other than smart quotes to ignore. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/routes/lexicon/+page.svelte (2)
73-78: Sorting byletterlooks correct for bucket ordering; consider edge cases and intra-letter orderUsing
alphabet.indexOf(a.letter)instead of deriving froma.word[0]is a good way to avoid leading smart-quote issues and centralizes normalization in the index/builder.Two follow-ups to consider:
- If any
ReversalWordhas a missing/invalidletter,indexOfwill be-1, and those entries will sort before all valid letters. Either ensureletteris always valid for this alphabet (preferred) or add a fallback (e.g., treat-1as the end of the alphabet).- This comparator only orders by
letter, so intra-letter ordering is left to whatever order the words currently have plus engine sort stability. If you ever need deterministic per-letter sorting here (beyond what the generator gives you), you might add a secondary key (e.g., a normalizedsortKeyfrom the index /word.localeCompare).
101-127:letterpopulation is sensible; tweak typing and naming for clarityStamping
letterfrom theloadLetterData(letter)parameter makes eachReversalWordself-describing, which works well with the new sort behavior.A couple of small improvements:
- The type at Line 101 uses
name: 'string', which is a literal type; this is almost certainly meant to bename: string:-const data: Record<string, { index: number; name: 'string' }[]> = +const data: Record<string, { index: number; name: string }[]> = await response.json();
- If
letteris conceptually the bucket key (not necessarily the literal first character inword), a slightly more explicit name likebucketLetteror a short comment could help future readers understand that this already incorporates the smart-quote / special-char normalization done when building the index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
convert/convertReverseIndex.ts(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- convert/convertReverseIndex.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
|
Based on discussion with @chrisvire and an independent investigation of the app-builders codebase, it should be enough for now to ignore only the smart quotes for sorting reversal entries during the conversion process. In the future, we may also need to rework how we do reversals in general to more closely align with the Android app. |
Words with smart quotes were being sorted ahead of words without. As far as I can tell, this problem only affected reversals.
Questions:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.