Multiple Reversals completed implementation #807
Conversation
Issue sillsdev#804 Transferred code from different branch and fixed small errors with letter loading
WalkthroughThis update refactors the handling of reversal languages across the lexicon components and page logic. The main change is shifting from supporting a single reversal language to supporting multiple reversal languages, represented as an array. Component props, rendering logic, and data-fetching conditions are updated to work with arrays of reversal languages. The logic for identifying reversal writing systems is also refined to use a specific property. Additionally, the word list loading logic is improved for robustness and flexibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LexiconListViewHeader
participant LexiconLanguageTabs
participant LexiconPage
User->>LexiconListViewHeader: Selects a language tab
LexiconListViewHeader->>LexiconLanguageTabs: Passes reversalLanguages array
LexiconLanguageTabs->>User: Renders tabs for each reversal language
User->>LexiconPage: Triggers language change
LexiconPage->>LexiconPage: Checks if selected language is in reversalLanguageList
LexiconPage->>LexiconPage: Loads and filters word data for selected language
LexiconPage->>User: Displays updated word list
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
src/routes/lexicon/+page.ts (2)
24-30:⚠️ Potential issue
reversalWritingSystemslength is never falsy – adjust the guard
Arrayobjects are always truthy, so the current test will never throw even when no reversal systems exist.-if (!reversalWritingSystems) { - throw new Error('Reversal language not found'); -} +if (reversalWritingSystems.length === 0) { + throw new Error('Reversal language not found'); +}
39-48: 🛠️ Refactor suggestionKeep reversal-index loading consistent with the new filter rule
The loop still loads indexes for every non-main writing system, even those without
reversalFilename.
To avoid unnecessary fetches (and 404s) restrict the loop to the samereversalWritingSystemslist:-for (const [key, ws] of Object.entries(dictionaryConfig.writingSystems)) { - if (!ws.type.includes('main')) { +for (const [key, ws] of reversalWritingSystems) {src/routes/lexicon/+page.svelte (2)
31-35:⚠️ Potential issueOnly first reversal alphabet is used — multi-language support is incomplete
alphabets.reversalis set fromreversalAlphabets[0], so any additional reversal languages are silently ignored. Down-stream logic (sorting, letter strip) will malfunction once a user switches to a second reversal language. Consider storing reversal alphabets in a map keyed by writing-system id and selecting the alphabet that matchesselectedLanguage.
198-205: 🛠️ Refactor suggestionScroll-to-bottom detector only works for first reversal language
selectedLanguage === reversalLanguagerestricts infinite-scroll to the very first reversal language. Replace the equality check with membership in the newreversalLanguageList.-(selectedLanguage === reversalLanguage && reversalWordsList.length > 0) +reversalLanguageList.includes(selectedLanguage) && reversalWordsList.length > 0
🧹 Nitpick comments (2)
src/lib/components/LexiconListViewHeader.svelte (1)
5-12: Consider typing & default-value safety for exported propsA number of exported props (
alphabet,reversalLanguages, etc.) default to non-null values while others (selectedLanguage,vernacularLanguage) do not. In production builds it’s easy to forget passing a required prop – especially now thatreversalLanguagesis an array and may be empty for some dictionaries.Adding simple runtime guards or TypeScript typings (using
lang="ts") will fail fast and save debugging time.- export let selectedLanguage; - export let vernacularLanguage; +/** Currently selected language (display name ‑ required) */ +export let selectedLanguage: string; +/** Vernacular language display name (required) */ +export let vernacularLanguage: string;src/lib/components/LexiconLanguageTabs.svelte (1)
28-46: Add keyed{#each}block to prevent DOM re-ordering glitchesBecause the array can be re-created when the store updates, Svelte may recycle DOM nodes in unexpected ways. Keying the loop makes tab state deterministic and (bonus) removes a React-style warning in dev-mode.
-{#each reversalLanguages as lang} +{#each reversalLanguages as lang (lang)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/LexiconLanguageTabs.svelte(2 hunks)src/lib/components/LexiconListViewHeader.svelte(2 hunks)src/routes/lexicon/+page.svelte(7 hunks)src/routes/lexicon/+page.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: lint
| let defaultReversalKey = Object.keys(reversalAlphabets[0])[0]; | ||
| let loadedReversalLetters = new Set(); | ||
| let reversalWordsList; | ||
| let vernacularWordsList; | ||
| let reversalWordsList = []; | ||
| let vernacularWordsList = []; | ||
| let selectedLanguage = vernacularLanguage; | ||
| const reversalLanguageList = Object.values(reversalLanguages[0]); | ||
| let scrollContainer; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
defaultReversalKey & reversalLanguageList assume a single language
Both variables are derived from the first element of their arrays, causing incorrect fetch paths and “language not found” behaviour when the user picks another reversal language. You can compute them on demand:
-const defaultReversalKey = Object.keys(reversalAlphabets[0])[0];
-const reversalLanguageList = Object.values(reversalLanguages[0]);
+const reversalAlphabetMap = Object.fromEntries(
+ reversalAlphabets.flatMap((obj) => Object.entries(obj))
+);
+const reversalLanguageMap = Object.fromEntries(
+ reversalLanguages.flatMap((obj) => Object.entries(obj))
+);
+const getKeyByLang = (lang) =>
+ Object.keys(reversalLanguageMap).find((k) => reversalLanguageMap[k] === lang);This map can then drive loadLetterData, handleLetterChange, scrolling logic, etc.
Committable suggestion skipped: line range outside the PR's diff.
| let fileIndex = 1; | ||
| let moreFiles = true; | ||
| let newWords = []; | ||
|
|
||
| const index = reversalIndexes[defaultReversalKey]; | ||
| const files = index[letter] || []; | ||
| for (const file of files) { | ||
| const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`; | ||
| const response = await fetch(reversalFile); | ||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| const currentFileWords = Object.entries(data).map(([word, entries]) => { | ||
| return { | ||
| word: word, | ||
| indexes: entries.map((entry) => entry.index), | ||
| vernacularWords: entries | ||
| .map((entry) => { | ||
| const foundWord = vernacularWordsList.find( | ||
| (vw) => vw.id === entry.index | ||
| ); | ||
| if (foundWord) { | ||
| return { | ||
| name: foundWord.name, | ||
| homonymIndex: foundWord.homonym_index || 0 | ||
| }; | ||
| } else { | ||
| console.log( | ||
| `Index ${entry.index} not found in vernacularWordsList` | ||
| while (moreFiles) { | ||
| try { | ||
| const response = await fetch( | ||
| `${base}/reversal/${defaultReversalKey}/${letter}-${String(fileIndex).padStart(3, '0')}.json` | ||
| ); | ||
| if (response.ok) { |
There was a problem hiding this comment.
Letter data is always fetched from the first reversal language
loadLetterData hard-codes defaultReversalKey; if the UI is on a different language the wrong files are requested.
-`${base}/reversal/${defaultReversalKey}/${letter}-${String(fileIndex).padStart(3,'0')}.json`
+const langKey = getKeyByLang(selectedLanguage);
+`${base}/reversal/${langKey}/${letter}-${String(fileIndex).padStart(3,'0')}.json`Coupled with the previous comment this will correctly download the right reversal index set.
Committable suggestion skipped: line range outside the PR's diff.
Issue #804
Transferred code from different branch
and fixed small errors with letter loading
Summary by CodeRabbit