Feature/Search/683 #805
Feature/Search/683 #805
Conversation
WalkthroughThis update introduces several new Svelte components, including Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchForm
participant dab-search-worker
participant LexiconXmlView
User->>SearchForm: Submit search phrase & options
SearchForm->>dab-search-worker: searchDictionary(phrase, options)
dab-search-worker-->>SearchForm: [wordIds]
SearchForm->>LexiconXmlView: Pass wordIds
LexiconXmlView->>LexiconXmlView: Query DB, parse XML, render entries
User->>LexiconXmlView: Click anchor (word reference)
LexiconXmlView->>SearchForm: onSelectWord callback
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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:
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 (
|
Create component for reversal view Create reusable alphabet strip component Implement initial lexicon reversal interface Add lexicon page data loading Add DAB program type route handling Add missing imports to lexicon and main routing pages lint format for page.js file for lexicon Create first reversal page draft Converted page to components Dynamically load alphabet from dictionary config Dynamically load alphabet from dictionary config Lift language state to parent component Add reversal lanuage name and letter change detection Lazy load English words and base for XML implementation Create Language Tabs Component and Parse lang Issue #685 Moved the code for the language tabs from LexiconReversalView and created a separate component. Also, parsed lang from writing systems and updated index.d.ts with displayLang Change English wording and fix selectedLetter Name vernacular language correctly and organize code Create new index.ts file for lexicon page Name vernacular language correctly and organize code Fix language tab naming convention Removed duplicate code for parsing lang Issue #685 Removed Unnecessary Code and Lexicon Folder Issue #685 Removed hardcoding Switch order of language tabs
EthanFennell
force-pushed
the
feature/search/683
branch
from
8115092 to
4328c85
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 17
♻️ Duplicate comments (1)
src/routes/lexicon/+page.svelte (1)
158-161:⚠️ Potential issueSame null‑selection bug as in Search page
Dereferencing
selectedWordafter potentially setting it tonullwill crash. Apply the same guard fix here.
🧹 Nitpick comments (17)
src/lib/components/AlphabetStrip.svelte (1)
7-9: Consider adding keyboard navigation to the alphabet strip.For better accessibility, implement keyboard navigation that allows users to move through the letters using arrow keys.
<script lang="ts"> export let alphabet: string[]; export let activeLetter: string; export let onLetterSelect: (letter: string) => void; + + function handleKeydown(event: KeyboardEvent) { + if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return; + + const currentIndex = alphabet.indexOf(activeLetter); + let newIndex: number; + + if (event.key === 'ArrowLeft') { + newIndex = currentIndex > 0 ? currentIndex - 1 : alphabet.length - 1; + } else { + newIndex = currentIndex < alphabet.length - 1 ? currentIndex + 1 : 0; + } + + onLetterSelect(alphabet[newIndex]); + } </script> <div class="flex m-2 gap-1 md:gap-2 mb-4 justify-start overflow-x-auto whitespace-nowrap pb-2 snap-x min-w-[100vw]" + tabindex="0" + role="toolbar" + aria-label="Alphabet navigation" + on:keydown={handleKeydown} >src/routes/quiz/[collection]/[id]/+page.js (2)
5-8: Function signature and validation look good, but consider improving error message clarityThe validation logic correctly checks for the presence of writing systems configuration. However, the error message could be more specific to help with troubleshooting.
- throw new Error('Writing systems configuration not found'); + throw new Error('Configuration error: config.writingSystems is missing or empty');
18-26: Consider adding more robust error handling for initial data loadingThe code silently continues if there's an error loading the initial reversal data, which could lead to subtle issues later. Consider adding more detailed error handling or at least a fallback mechanism.
let initialReversalData = {}; try { const response = await fetch(`${base}/reversal/language/${defaultKey}/${alphabet[0]}.json`); if (response.ok) { initialReversalData = await response.json(); + } else { + console.warn(`Failed to load initial reversal data: ${response.status} ${response.statusText}`); } } catch (error) { console.error('Error loading initial reversal data:', error); }src/lib/search-worker/dab-search-worker.ts (3)
10-19: Consider optimizing database initialization and console loggingThe database initialization might be expensive, and console logging large result sets could impact performance in production.
- let db = await initializeDatabase({ fetch }); + // Reuse database connection if possible + const db = await initializeDatabase({ fetch }); const dynamicQuery = searchWords.map(() => `${column} LIKE ?`).join(' OR '); const dynamicParams = searchWords.map((word) => (options.wholeWords ? word : `%${word}%`)); results = db.exec(`SELECT locations FROM search_words WHERE ${dynamicQuery}`, dynamicParams); - console.log('results:', results); + // Use more targeted logging or remove in production + if (process.env.NODE_ENV !== 'production') { + console.log('Search results count:', results?.[0]?.values?.length || 0); + }
21-27: The flatMap and processing logic is efficient, but could benefit from commentsThe location extraction logic is somewhat complex. Consider adding more detailed comments to explain the data format and processing steps.
// Extract and process locations from the query result + // Format of locations: "id1(weight1) id2(weight2) ..." + // We parse this into an array of objects with id and weight properties let locations = results[0].values.flatMap((value) => value[0].split(' ').map((loc) => { const [id, weight] = loc.split('(').map((v) => v.replace(')', '')); return { id: parseInt(id, 10), weight: parseInt(weight, 10) }; }) );
41-44: Consider adding a limit to the number of results returnedFor performance reasons, you might want to limit the number of results returned, especially for very common search terms.
locations = locations.sort((a, b) => b.weight - a.weight); + // Limit results to avoid overwhelming the UI + const MAX_RESULTS = 100; // Adjust as needed + locations = locations.slice(0, MAX_RESULTS); const ids = locations.map((location) => location.id); return ids;src/lib/components/SearchForm.svelte (2)
13-15: Remove commented-out codeCommented-out code should be removed before merging to production to keep the codebase clean.
- // const specialCharacters = - // config.mainFeatures['input-buttons']?.split(' ').filter((c) => c.length) ?? []; -
83-83: Consider refactoring repetitive conditional logicThe conditions for displaying UI elements follow the same pattern and could be refactored to reduce duplication.
You could create a helper function at the top of the script section:
<script lang="ts"> // ...existing imports // Helper function to determine if a feature should be displayed function shouldShowFeature(featureName: string): boolean { return !!config.mainFeatures[featureName] || config.programType === 'DAB'; } // ...rest of script </script> <!-- Then in the template: --> {#if shouldShowFeature('search-input-buttons') && specialCharacters.length > 0} <!-- Special characters section --> {/if} {#if shouldShowFeature('search-whole-words-show')} <!-- Whole words section --> {/if} {#if shouldShowFeature('search-accents-show')} <!-- Accents section --> {/if}Also applies to: 98-98, 108-108
src/lib/components/LexiconLanguageTabs.svelte (1)
11-45: Extract hardcoded animation values to variables and add aria-label attributesThe animation values are hardcoded and the tabs could benefit from aria-label attributes for better accessibility.
<div class="flex w-full" style="background-color: var(--TabBackgroundColor);"> + <!-- Animation constants could be defined in the script section --> + <!-- const ANIMATION_DISTANCE = 70; --> <div role="button" tabindex="0" aria-pressed={selectedLanguage === vernacularLanguage} + aria-label={`Switch to ${vernacularLanguage} language`} on:click={() => onSwitchLanguage(vernacularLanguage)} on:keydown={(e) => e.key === 'Enter' && onSwitchLanguage(vernacularLanguage)} class="py-2.5 px-3.5 text-sm uppercase text-center relative dy-tabs dy-tabs-bordered mb-1" > {vernacularLanguage} {#if selectedLanguage === vernacularLanguage} <div - transition:fly={{ axis: 'x', easing: expoInOut, x: 70 }} + transition:fly={{ axis: 'x', easing: expoInOut, x: 70 }} class="absolute -bottom-1 left-0 w-full h-1 bg-black" ></div> {/if} </div> <div role="button" tabindex="0" aria-pressed={selectedLanguage === reversalLanguage} + aria-label={`Switch to ${reversalLanguage} language`} on:click={() => onSwitchLanguage(reversalLanguage)} on:keydown={(e) => e.key === 'Enter' && onSwitchLanguage(reversalLanguage)} class="py-2.5 px-3.5 text-sm uppercase text-center relative dy-tabs dy-tabs-bordered mb-1" > {reversalLanguage} {#if selectedLanguage === reversalLanguage} <div - transition:fly={{ axis: 'x', easing: expoInOut, x: -70 }} + transition:fly={{ axis: 'x', easing: expoInOut, x: -70 }} class="absolute -bottom-1 left-0 w-full h-1 bg-black" ></div> {/if} </div> <div class="flex-1"></div> </div>src/lib/components/LexiconXMLView.svelte (1)
150-156: Guard against missingsingleEntryStyles
config.singleEntryStylesis optional. If the dictionary has no styles, thefor…ofloop crashes.- for (let stl of config.singleEntryStyles) { + if (!config.singleEntryStyles) return; + for (let stl of config.singleEntryStyles) {src/routes/lexicon/+page.ts (3)
36-40: Data shape ofreversalAlphabets/reversalLanguagesis awkwardEach element is
{ [key]: value }, producing an array of singleton objects:[ { 'fr': [...] }, { 'es': [...] } ]Down‑stream consumers must
Object.values(obj)[0]every time.
Either:
- Keep them as true maps
{ [key: string]: alphabet }, or- Return an array of tuples
Array<{ lang: string; alphabet: string[] }>Pick whichever matches the consuming component; otherwise this will cause unnecessary iteration and brittle
as anycasts.
70-86: First‑letter extraction can overrun the string
entry.name.substring(startingPosition, 2 + startingPosition)will produce a substring of variable length ≥ 2 when the word is only one character long, but you later testvernacularAlphabet.includes(firstTwoChars)which expects a single letter or digraph present in the alphabet list.Use
slice(start, start + 2)(which clamps) and normalise length:- firstTwoChars = entry.name - .substring(startingPosition, 2 + startingPosition) - .toLowerCase(); + firstTwoChars = entry.name + .slice(startingPosition, startingPosition + 2) + .toLowerCase();
98-103: Return type lacks vernacular language & words
load()sets the stores but the returneddataobject omits them.
Including them allows page components to access vialoadprops without subscribing to the stores again:return { vernacularAlphabet, + vernacularLanguage, + vernacularWords: vernacularWordsList, reversalAlphabets, reversalLanguages, reversalIndexes };src/lib/components/WordNavigationStrip.svelte (1)
60-74: Normalise property names to prevent downstream type errorsThe vernacular branch sets
index, whereas the reversal branch setsindexes. Call‑sites need to handle both spellings, causing brittle code (see similar bug in search & lexicon pages). Consider standardising on one key (e.g., always exposeindexes, even for a single ID).Also applies to: 80-96
src/routes/lexicon/search/+page.svelte (2)
50-52: Remove noisy console logging or wrap in debug flagThe
console.log(wordIds)statement will spam production logs. Either gate it behind a debug flag or remove it before shipping.
103-113: UI duplication: Search form & “No results” render togetherWhen a search returns zero matches, both the form and the No results found message are shown. Consider hiding the form to keep the feedback concise or placing the empty‑state message inside the form component.
src/routes/lexicon/+page.svelte (1)
210-238: Scroll handler can become O(n²) with many letters
checkIfScrolledToBottomqueries all[id^="letter-"]elements on every scroll event, which can be expensive for large lexicons. Consider throttling the handler and caching the NodeList.
🛑 Comments failed to post (17)
src/lib/components/AlphabetStrip.svelte (1)
1-24: 🛠️ Refactor suggestion
Good implementation with room for accessibility improvements.
The component is well-structured with responsive design, snap scrolling for touch devices, and clear visual feedback for the active letter. The design scales appropriately across different screen sizes.
Consider these accessibility enhancements:
- Add ARIA attributes for screen readers
- Implement keyboard navigation support
<button class="px-3 py-2 text-sm font-bold border rounded-md bg-gray-100 hover:bg-gray-200 cursor-pointer snap-start sm:px-4 sm:py-3 sm:text-base md:px-5 md:py-4 md:text-base lg:px-6 lg:py-4 lg:text-lg" style={activeLetter === letter ? 'background-color: var(--TitleBackgroundColor); border-color: black;' : ''} on:click={() => onLetterSelect(letter)} + aria-current={activeLetter === letter ? 'true' : undefined} + aria-label={`Navigate to letter ${letter}`} > {letter} </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<script lang="ts"> export let alphabet: string[]; export let activeLetter: string; export let onLetterSelect: (letter: string) => void; </script> <div class="flex m-2 gap-1 md:gap-2 mb-4 justify-start overflow-x-auto whitespace-nowrap pb-2 snap-x min-w-[100vw]" > {#each alphabet as letter} <button class="px-3 py-2 text-sm font-bold border rounded-md bg-gray-100 hover:bg-gray-200 cursor-pointer snap-start sm:px-4 sm:py-3 sm:text-base md:px-5 md:py-4 md:text-base lg:px-6 lg:py-4 lg:text-lg" style={activeLetter === letter ? 'background-color: var(--TitleBackgroundColor); border-color: black;' : ''} on:click={() => onLetterSelect(letter)} aria-current={activeLetter === letter ? 'true' : undefined} aria-label={`Navigate to letter ${letter}`} > {letter} </button> {/each} </div>src/routes/quiz/[collection]/[id]/+page.js (1)
10-14: 🛠️ Refactor suggestion
Validate that writing systems exist before destructuring
The code assumes that
Object.entries(config.writingSystems)returns at least one entry. Ifconfig.writingSystemsis an empty object, this will lead to undefined values.- const [defaultKey, writingSystem] = Object.entries(config.writingSystems)[0]; + const entries = Object.entries(config.writingSystems); + if (entries.length === 0) { + throw new Error('No writing systems found in configuration'); + } + const [defaultKey, writingSystem] = entries[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const entries = Object.entries(config.writingSystems); if (entries.length === 0) { throw new Error('No writing systems found in configuration'); } const [defaultKey, writingSystem] = entries[0]; if (!writingSystem?.alphabet) { throw new Error('Writing system alphabet not found'); }src/lib/components/SearchForm.svelte (1)
16-24: 🛠️ Refactor suggestion
Add type safety for writingSystems access
The code accesses properties of writingSystems objects without type checking, which could lead to runtime errors.
let specialCharacters = []; if (config.programType == 'SAB') { specialCharacters = config.mainFeatures['input-buttons']?.split(' ').filter((c) => c.length) ?? []; } else if (config.programType === 'DAB') { + // Type-safe access to writing system properties specialCharacters = Object.values(config.writingSystems ?? {}) - .filter((ws: any) => ws.type && ws.type.includes('main')) - .flatMap((ws: any) => ws.inputButtons || []); + .filter((ws) => ws?.type && Array.isArray(ws.type) && ws.type.includes('main')) + .flatMap((ws) => Array.isArray(ws.inputButtons) ? ws.inputButtons : []); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let specialCharacters = []; if (config.programType == 'SAB') { specialCharacters = config.mainFeatures['input-buttons']?.split(' ').filter((c) => c.length) ?? []; } else if (config.programType === 'DAB') { // Type-safe access to writing system properties specialCharacters = Object.values(config.writingSystems ?? {}) .filter((ws) => ws?.type && Array.isArray(ws.type) && ws.type.includes('main')) .flatMap((ws) => Array.isArray(ws.inputButtons) ? ws.inputButtons : []); }src/lib/components/LexiconLanguageTabs.svelte (2)
1-9: 🛠️ Refactor suggestion
Add TypeScript type definitions for the component props
The component is missing TypeScript type definitions for its props, which would improve type safety and code documentation.
-<script> +<script lang="ts"> import { expoInOut } from 'svelte/easing'; import { fly } from 'svelte/transition'; - export let reversalLanguage; - export let selectedLanguage; - export let onSwitchLanguage; - export let vernacularLanguage; + export let reversalLanguage: string; + export let selectedLanguage: string; + export let onSwitchLanguage: (language: string) => void; + export let vernacularLanguage: string; </script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<script lang="ts"> import { expoInOut } from 'svelte/easing'; import { fly } from 'svelte/transition'; export let reversalLanguage: string; export let selectedLanguage: string; export let onSwitchLanguage: (language: string) => void; export let vernacularLanguage: string; </script>
1-45: 💡 Verification agent
❓ Verification inconclusive
Add validation for duplicate language identifiers
The component should handle the case where vernacularLanguage and reversalLanguage might be the same, which could lead to confusing UI behavior.
🏁 Script executed:
#!/bin/bash # Check if there are cases where vernacularLanguage and reversalLanguage might be the same rg -r '$1' -g "*.svelte" -g "*.ts" -g "*.js" "LexiconLanguageTabs.*vernacularLanguage=\"?(\w+)\"?.*reversalLanguage=\"?(\w+)\"?" --replace '$1 $2' | grep -P '(\w+) \1'Length of output: 165
Validate unique language props in LexiconLanguageTabs
Currently, if vernacularLanguage and reversalLanguage are identical, both tabs render the same label—which can confuse users. Add a runtime check at the top of src/lib/components/LexiconLanguageTabs.svelte to guard against this:
• File to update:
- src/lib/components/LexiconLanguageTabs.svelte
• Suggested change:
<script> export let vernacularLanguage; export let reversalLanguage; export let selectedLanguage; export let onSwitchLanguage; // Validate that the two languages differ if (vernacularLanguage === reversalLanguage) { throw new Error( 'vernacularLanguage and reversalLanguage must be distinct' ); } </script>You can also choose to emit a console warning or render a single tab instead of throwing—whatever best fits the UX requirements.
src/lib/components/LexiconXMLView.svelte (4)
125-148: 🛠️ Refactor suggestion
afterUpdate& cloning logic can create an infinite fetch / listener loop
afterUpdatecallsupdateXmlData()unconditionally;updateXmlDatasetsxmlData, causing another update → endless network & DOM churn.
Use a reactive statement oronMountwatcher keyed towordIdsinstead.Detaching listeners by cloning every
.clickablenode forces a complete DOM rebuild each update. Consider:<span class="clickable" on:click={() => handleClick(word, index, homonym)} >to rely on Svelte’s diffing instead of manual cloning.
These two issues combined will seriously hurt performance on every keystroke in the search page.
68-77: 🛠️ Refactor suggestion
Potential NPE & stale data when looking up vernacular words
get(vernacularWordsStore)can beundefinedthe first time this runs, and the store’s static typing isstring[], even though you later treat each item as an object withid,name,homonym_index.
Update the store’s generic (instores/lexicon.ts) and guard for an empty store here.- const wordObject = get(vernacularWordsStore).find( + const words = get(vernacularWordsStore) ?? []; + const wordObject = words.find( (item) => item.id === index );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const href = node.getAttribute('href'); const match = href.match(/E-(\d+)/); // Extract index number if (match) { const index = parseInt(match[1], 10); // Extracted number as integer const words = get(vernacularWordsStore) ?? []; const wordObject = words.find( (item) => item.id === index ); const word = wordObject ? wordObject.name : 'Unknown'; // Fallback if not found const homonymIndex = wordObject ? wordObject.homonym_index : 1; // Default to 1 if not found
18-30: 💡 Verification agent
❓ Verification inconclusive
queryXmlByWordIdreturns rows, not raw XML strings
db.execreturns an array of result objects, where eachvalueselement is itself an array containing one cell per column.
return results[0].valuestherefore produces e.g.[ [ '<entry>…</entry>' ], [ '<entry>…</entry>' ], … ].
Down‑stream,updateXmlDatatreats every element as a raw string and callsformatXmlByClass(xml), which will receive an array and explode inDOMParser.- return results[0].values; + // Flatten to a simple array of xml strings + return results[0].values.map((row) => row[0]);Also remove the debug
console.logbefore merging.
Flatten returned XML rows in queryXmlByWordId and remove debug log
- In
src/lib/components/LexiconXMLView.svelte(around lines 18–30):
• Remove the debug log:• Replace the raw return with a flattened array of strings:- console.log('results:', results[0].values);- return results[0].values; + // Flatten to a simple array of xml strings + return results[0].values.map(row => row[0]);This ensures
updateXmlData(and its call toformatXmlByClass(xml)) receives plain XML strings, not nested arrays.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async function queryXmlByWordId(wordIds) { try { let db = await initializeDatabase({ fetch }); let results; const dynamicQuery = wordIds.map(() => `id = ?`).join(' OR '); const dynamicParams = wordIds.map((id) => id); results = db.exec(`SELECT xml FROM entries WHERE ${dynamicQuery}`, dynamicParams); // Flatten to a simple array of xml strings return results[0].values.map((row) => row[0]); } catch (error) { console.error(`Error querying XML for word IDs ${wordIds}:`, error);
48-66:
⚠️ Potential issue
processNodemay dereferencenullparentsFor the root element
node.parentNodeisnull, sonode.parentNode.childrenthrows.
Guard before use:- [...node.parentNode.children].some( + (node.parentNode && [...node.parentNode.children].some( (child) => child.getAttribute && (child.getAttribute('class') || '').includes('sensenumber') - ); + ));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function processNode(node, parentHasSenseNumber = false) { let output = ''; if (node.nodeType === Node.TEXT_NODE) { return node.nodeValue.trim() ? node.nodeValue + ' ' : ''; } if (node.nodeType === Node.ELEMENT_NODE) { let className = node.getAttribute('class') || ''; let isSenseNumber = className.includes('sensenumber'); let parentContainsSenseNumber = parentHasSenseNumber || (node.parentNode && [...node.parentNode.children].some( (child) => child.getAttribute && (child.getAttribute('class') || '').includes('sensenumber') ));src/lib/components/LexiconReversalView.svelte (1)
20-25: 🛠️ Refactor suggestion
Reactive blocks continuously reset
currentLetterEvery change to
alphabetorselectedLettertriggers the two$:assignments, overwriting a user’s in‑progress selection:$: if (alphabet && alphabet.length > 0) { currentLetter = alphabet[0]; // ← keeps firing }Combine the two blocks and guard:
-$: if (alphabet && alphabet.length > 0) { - currentLetter = alphabet[0]; -} -$: if (selectedLetter !== currentLetter) { - currentLetter = selectedLetter; -} +$: { + if (selectedLetter) { + currentLetter = selectedLetter; + } else if (alphabet?.length) { + currentLetter = alphabet[0]; + } +}This prevents accidental resets and honours the controlled
selectedLetterprop.src/routes/lexicon/+page.ts (2)
54-60: 🛠️ Refactor suggestion
Type mismatch:
vernacularWordsStoredeclared asstring[]but receives objects
stores/lexicon.ts:export const vernacularWordsStore = writable<string[]>();Here you push objects with
{ id, name, … }.
Update the store type to an explicit interface so TypeScript (and IDE tooling) can help:-export const vernacularWordsStore = writable<string[]>(); +export interface VernacularEntry { + id: number; + name: string; + homonym_index: number; + type: string; + num_senses: number; + summary: string; + letter: string; +} +export const vernacularWordsStore = writable<VernacularEntry[]>();Then audit usages (
LexiconXMLViewetc.) to removeanycasts.
32-34:
⚠️ Potential issue
reversalWritingSystemslength should be checked, not truthiness
filteralways returns an array;if (!reversalWritingSystems)will never execute.
Use a length check to throw when none are configured:-if (!reversalWritingSystems) { +if (reversalWritingSystems.length === 0) { throw new Error('Reversal language not found'); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (reversalWritingSystems.length === 0) { throw new Error('Reversal language not found'); }src/lib/components/WordNavigationStrip.svelte (2)
16-22:
⚠️ Potential issueMake
wordsListreactive instead of a one–time snapshot
get()only captures the stores’ values at component instantiation, sowordsListwill never update when the language or the underlying lists change (e.g., after switching tabs or when the store is populated asynchronously). Convert the computation to a reactive statement that re‑evaluates whenever either store updates.-// List of all words (should come from either vernacularWordsList or reversalWordsList) -let wordsList; -if (get(selectedLanguageStore) === get(vernacularLanguageStore)) { - wordsList = get(vernacularWordsStore); -} else { - wordsList = get(currentReversalWordsStore); -} +// Reactively choose the correct word list +$: wordsList = + $selectedLanguageStore === $vernacularLanguageStore + ? $vernacularWordsStore + : $currentReversalWordsStore;
24-56: 🛠️ Refactor suggestion
Guard against
currentIndex === -1to avoid incorrect navigationWhen the current word is not found,
currentIndexbecomes‑1, which makesnextWordpoint towordsList[0](the first word) and leavespreviousWordnull, producing a confusing UI jump. Handle the sentinel value explicitly.-$: previousWord = currentIndex > 0 ? wordsList[currentIndex - 1] : null; -$: nextWord = currentIndex < wordsList.length - 1 ? wordsList[currentIndex + 1] : null; +$: { + previousWord = currentIndex > 0 ? wordsList[currentIndex - 1] : null; + nextWord = + currentIndex !== -1 && currentIndex < wordsList.length - 1 + ? wordsList[currentIndex + 1] + : null; +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Compute previous and next words $: { previousWord = currentIndex > 0 ? wordsList[currentIndex - 1] : null; nextWord = currentIndex !== -1 && currentIndex < wordsList.length - 1 ? wordsList[currentIndex + 1] : null; }src/routes/lexicon/search/+page.svelte (1)
54-57:
⚠️ Potential issueNull‑selection path throws a runtime error
If the same word is clicked twice,
selectedWordbecomesnull, yet the next line dereferences it (selectedWord.indexes). This raisesTypeError: cannot read properties of null.-function selectWord(word) { - selectedWord = selectedWord && selectedWord.word === word ? null : word; - wordIds = selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]; +function selectWord(word) { + const newSelection = + selectedWord && selectedWord.word === word ? null : word; + selectedWord = newSelection; + + if (selectedWord) { + wordIds = selectedWord.indexes ?? [selectedWord.index]; + } else { + wordIds = []; + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function selectWord(word) { const newSelection = selectedWord && selectedWord.word === word ? null : word; selectedWord = newSelection; if (selectedWord) { wordIds = selectedWord.indexes ?? [selectedWord.index]; } else { wordIds = []; } }src/lib/data/stores/lexicon.ts (2)
48-69: 🛠️ Refactor suggestion
Possible double‑initialisation race
Multiple callers invoking
initializeDatabasesimultaneously will all run the fetch & init logic because the guard only checks current values, not a pending promise. Use a shared promise or lock to ensure the work happens once.
8-10:
⚠️ Potential issueIncorrect store type for vernacular words
vernacularWordsStoreholds objects, not strings, sowritable<string[]>()is misleading and loses type safety.-export const vernacularWordsStore = writable<string[]>(); +export interface VernacularWord { + id: string; + name: string; + homonym_index?: number; +} + +export const vernacularWordsStore = writable<VernacularWord[]>([]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Store for vernacularWordsList export interface VernacularWord { id: string; name: string; homonym_index?: number; } export const vernacularWordsStore = writable<VernacularWord[]>([]);
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
src/lib/components/LexiconXMLView.svelte (1)
151-155:singleEntryStylesmay be undefinedAccessing
for (let stl of config.singleEntryStyles)will crash when the property is absent. Add a guard:- for (let stl of config.singleEntryStyles) { + if (!config.singleEntryStyles) return; + for (let stl of config.singleEntryStyles) {src/routes/lexicon/+page.ts (1)
32-34: Ineffective reversal‑language presence check
reversalWritingSystemsis always an array; the current truthiness test never fails. Use a length check instead:- if (!reversalWritingSystems) { + if (reversalWritingSystems.length === 0) { throw new Error('Reversal language not found'); }src/routes/lexicon/+page.svelte (4)
35-35: Use reactive declaration for derived valueThe
showBackButtonvariable can be simplified to a reactive declaration.-let selectedWord = null; -$: showBackButton = selectedWord ? true : false; +let selectedWord = null; +$: showBackButton = !!selectedWord;
310-325: Refactor duplicate scroll container codeThe scroll container div is duplicated across three different conditional branches with the same classes and event handlers.
Consider refactoring the three similar scroll container divs into a single component or at least into a variable template:
{#if selectedWord} <WordNavigationStrip currentWord={selectedWord} onSelectWord={selectWord} /> <div class="flex-1 overflow-y-auto bg-base-100 width-full" bind:this={scrollContainer} on:scroll={checkIfScrolledToBottom} > <LexiconXmlView {wordIds} onSelectWord={selectWord} /> </div> {:else} <div class="flex-1 overflow-y-auto bg-base-100 width-full" bind:this={scrollContainer} on:scroll={checkIfScrolledToBottom} > {#if selectedLanguage === vernacularLanguage} <LexiconVernacularListView {vernacularWordsList} onSelectWord={selectWord} /> {:else} <LexiconReversalListView {reversalWordsList} onSelectWord={selectWord} /> {/if} </div> {/if}
60-61: Remove console.log statementConsole.log statements should not be present in production code.
if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { - console.log('Loading letter data:', letter); + // Loading letter data
241-242: Improve reactive statement readabilityThe reactive statement for
currentAlphabetcan be made more readable.-$: currentAlphabet = - selectedLanguage === reversalLanguage ? alphabets.reversal : alphabets.vernacular; +$: currentAlphabet = selectedLanguage === reversalLanguage + ? alphabets.reversal + : alphabets.vernacular;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
config/index.d.ts(1 hunks)convert/convertConfig.ts(1 hunks)convert/convertReverseIndex.ts(4 hunks)convert/tests/sab/convertConfigSAB.test.ts(1 hunks)package.json(1 hunks)src/lib/components/AlphabetStrip.svelte(1 hunks)src/lib/components/LexiconLanguageTabs.svelte(1 hunks)src/lib/components/LexiconReversalListView.svelte(1 hunks)src/lib/components/LexiconReversalView.svelte(1 hunks)src/lib/components/LexiconVernacularListView.svelte(1 hunks)src/lib/components/LexiconXMLView.svelte(1 hunks)src/lib/components/SearchForm.svelte(4 hunks)src/lib/components/Sidebar.svelte(2 hunks)src/lib/components/WordNavigationStrip.svelte(1 hunks)src/lib/data/stores/lexicon.ts(1 hunks)src/lib/lexicon/index.ts(1 hunks)src/lib/search-worker/dab-search-worker.ts(1 hunks)src/lib/search/domain/interfaces/data-interfaces.ts(1 hunks)src/routes/+layout.svelte(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)src/routes/lexicon/+page.ts(1 hunks)src/routes/lexicon/search/+page.svelte(1 hunks)src/routes/quiz/[collection]/[id]/+page.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/routes/+layout.svelte
🚧 Files skipped from review as they are similar to previous changes (18)
- convert/tests/sab/convertConfigSAB.test.ts
- src/lib/components/Sidebar.svelte
- src/lib/search/domain/interfaces/data-interfaces.ts
- package.json
- src/lib/components/LexiconReversalListView.svelte
- src/lib/components/AlphabetStrip.svelte
- convert/convertReverseIndex.ts
- config/index.d.ts
- src/lib/components/LexiconReversalView.svelte
- convert/convertConfig.ts
- src/lib/search-worker/dab-search-worker.ts
- src/lib/lexicon/index.ts
- src/lib/components/SearchForm.svelte
- src/lib/components/LexiconVernacularListView.svelte
- src/lib/components/LexiconLanguageTabs.svelte
- src/routes/lexicon/search/+page.svelte
- src/lib/components/WordNavigationStrip.svelte
- src/lib/data/stores/lexicon.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/routes/lexicon/+page.ts (3)
config/index.d.ts (1)
DictionaryConfig(279-295)src/lib/lexicon/index.ts (1)
ReversalIndex(1-3)src/lib/data/stores/lexicon.ts (3)
initializeDatabase(48-69)vernacularLanguageStore(6-6)vernacularWordsStore(9-9)
🔇 Additional comments (3)
src/routes/quiz/[collection]/[id]/+page.js (1)
5-15:Object.entries(...)[0]is nondeterministicJavaScript object enumeration order isn’t guaranteed to match user expectations. Relying on “first entry” to pick a default writing system can yield random results across browsers or future config changes.
Consider picking the writing system flagged as default (e.g.,
ws.type.includes('main')) or introduce an explicitdefaultWritingSystemkey in the configuration.src/routes/lexicon/+page.ts (2)
36-40: Data shape mismatch may confuse consumers
reversalAlphabetsandreversalLanguagesare arrays of single‑key objects ([{ en: [...] }, { fr: [...] }]).
Callers typically expect a plain map ({ en: [...], fr: [...] }). Confirm the expected shape or refactor:-const reversalAlphabets = reversalWritingSystems.map(([key, ws]) => ({ [key]: ws.alphabet })); +const reversalAlphabets = Object.fromEntries( + reversalWritingSystems.map(([key, ws]) => [key, ws.alphabet]) +);
69-89:⚠️ Potential issue
entry.name.substring(startingPosition, 2 + startingPosition)is off‑by‑one
substring’s second argument is an end index, not a length. For two characters you needstartingPosition + 2, not2 + startingPosition(works only whenstartingPosition≤ 1). Extracting from offset 1 would currently yield one char.- firstTwoChars = entry.name - .substring(startingPosition, 2 + startingPosition) + firstTwoChars = entry.name + .substring(startingPosition, startingPosition + 2)Likely an incorrect or invalid review comment.
| output += '<' + node.tagName; | ||
| for (let attr of node.attributes) { | ||
| output += ` ${attr.name}="${attr.value}"`; | ||
| } | ||
| output += '>'; | ||
|
|
||
| for (let child of node.childNodes) { | ||
| output += processNode(child, parentContainsSenseNumber || isSenseNumber); | ||
| } | ||
|
|
||
| output += `</${node.tagName}>`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential XSS: arbitrary XML is injected as raw HTML
All element names & attribute values from the DB are retransmitted verbatim into xmlData, which is later rendered with {@html …}.
A malicious entry containing <script> or an inline event handler (onerror="steal()") will execute client‑side, compromising the app.
Mitigate by whitelisting tags/attributes or sanitising with a library such as DOMPurify before returning the string.
| async function queryXmlByWordId(wordIds) { | ||
| try { | ||
| let db = await initializeDatabase({ fetch }); | ||
|
|
||
| let results; | ||
| const dynamicQuery = wordIds.map(() => `id = ?`).join(' OR '); | ||
| const dynamicParams = wordIds.map((id) => id); | ||
| results = db.exec(`SELECT xml FROM entries WHERE ${dynamicQuery}`, dynamicParams); | ||
| console.log('results:', results[0].values); | ||
|
|
||
| return results[0].values; | ||
| } catch (error) { | ||
| console.error(`Error querying XML for word IDs ${wordIds}:`, error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against empty‐result crashes & remove noisy logging
db.exec returns an empty array when no rows match. Accessing results[0].values will then throw TypeError: Cannot read properties of undefined.
Additionally, the console.log leaks internal data and can flood the console in production.
- results = db.exec(`SELECT xml FROM entries WHERE ${dynamicQuery}`, dynamicParams);
- console.log('results:', results[0].values);
-
- return results[0].values;
+ results = db.exec(
+ `SELECT xml FROM entries WHERE ${dynamicQuery}`,
+ dynamicParams
+ );
+
+ if (!results.length) {
+ return []; // ← safe fallback
+ }
+
+ return results[0].values;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function queryXmlByWordId(wordIds) { | |
| try { | |
| let db = await initializeDatabase({ fetch }); | |
| let results; | |
| const dynamicQuery = wordIds.map(() => `id = ?`).join(' OR '); | |
| const dynamicParams = wordIds.map((id) => id); | |
| results = db.exec(`SELECT xml FROM entries WHERE ${dynamicQuery}`, dynamicParams); | |
| console.log('results:', results[0].values); | |
| return results[0].values; | |
| } catch (error) { | |
| console.error(`Error querying XML for word IDs ${wordIds}:`, error); | |
| return null; | |
| } | |
| } | |
| async function queryXmlByWordId(wordIds) { | |
| try { | |
| let db = await initializeDatabase({ fetch }); | |
| let results; | |
| const dynamicQuery = wordIds.map(() => `id = ?`).join(' OR '); | |
| const dynamicParams = wordIds.map((id) => id); | |
| results = db.exec( | |
| `SELECT xml FROM entries WHERE ${dynamicQuery}`, | |
| dynamicParams | |
| ); | |
| if (!results.length) { | |
| return []; // ← safe fallback | |
| } | |
| return results[0].values; | |
| } catch (error) { | |
| console.error(`Error querying XML for word IDs ${wordIds}:`, error); | |
| return null; | |
| } | |
| } |
| const spans = document.querySelectorAll('.clickable'); | ||
|
|
||
| spans.forEach((span) => { | ||
| const oldSpan = span.cloneNode(true); | ||
| span.parentNode.replaceChild(oldSpan, span); | ||
| }); | ||
|
|
||
| const freshSpans = document.querySelectorAll('.clickable'); | ||
| freshSpans.forEach((span) => { | ||
| span.addEventListener('click', () => { | ||
| selectedLanguageStore.set(get(vernacularLanguageStore)); | ||
| const word = span.getAttribute('data-word'); | ||
| const index = parseInt(span.getAttribute('data-index'), 10); | ||
| const homonym_index = parseInt(span.getAttribute('data-homonym'), 10); | ||
|
|
||
| onSelectWord({ | ||
| word, | ||
| index, | ||
| homonym_index | ||
| }); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inefficient DOM manipulation & event re‑binding
Cloning every .clickable span on each update is O(n²) over time and breaks browser DevTools event tracing.
Prefer event delegation once on a stable parent node:
-function attachEventListeners() {
- const spans = document.querySelectorAll('.clickable');
- …
- freshSpans.forEach((span) => {
- span.addEventListener('click', () => {
- …
- });
- });
-}
+function attachEventListeners() {
+ const container = document.getElementById('xml-container');
+ if (!container) return;
+ container.addEventListener('click', (e) => {
+ const span = e.target.closest('.clickable');
+ if (!span) return;
+ selectedLanguageStore.set(get(vernacularLanguageStore));
+ onSelectWord({
+ word: span.dataset.word,
+ index: Number(span.dataset.index),
+ homonym_index: Number(span.dataset.homonym)
+ });
+ });
+}(You’ll need to wrap the rendered HTML in an element with id="xml-container".)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const spans = document.querySelectorAll('.clickable'); | |
| spans.forEach((span) => { | |
| const oldSpan = span.cloneNode(true); | |
| span.parentNode.replaceChild(oldSpan, span); | |
| }); | |
| const freshSpans = document.querySelectorAll('.clickable'); | |
| freshSpans.forEach((span) => { | |
| span.addEventListener('click', () => { | |
| selectedLanguageStore.set(get(vernacularLanguageStore)); | |
| const word = span.getAttribute('data-word'); | |
| const index = parseInt(span.getAttribute('data-index'), 10); | |
| const homonym_index = parseInt(span.getAttribute('data-homonym'), 10); | |
| onSelectWord({ | |
| word, | |
| index, | |
| homonym_index | |
| }); | |
| }); | |
| }); | |
| } | |
| function attachEventListeners() { | |
| const container = document.getElementById('xml-container'); | |
| if (!container) return; | |
| container.addEventListener('click', (e) => { | |
| const span = e.target.closest('.clickable'); | |
| if (!span) return; | |
| selectedLanguageStore.set(get(vernacularLanguageStore)); | |
| onSelectWord({ | |
| word: span.dataset.word, | |
| index: Number(span.dataset.index), | |
| homonym_index: Number(span.dataset.homonym) | |
| }); | |
| }); | |
| } |
| const response = await fetch(`${base}/reversal/language/${defaultKey}/${alphabet[0]}.json`); | ||
| if (response.ok) { | ||
| initialReversalData = await response.json(); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error loading initial reversal data:', error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
404‑prone hard‑coded URL segment
The fetch path contains /reversal/language/${defaultKey}/…, whereas other code (e.g., lexicon loader) uses /reversal/${key}/index.json. Verify that the extra language segment exists on the server; otherwise the request will silently fail and initialReversalData remains {}.
src/routes/lexicon/+page.svelte
Outdated
| function switchLanguage(language) { | ||
| selectedLanguageStore.set(language); | ||
| selectedLetter = currentAlphabet[0]; | ||
| if (selectedLanguage != vernacularLanguage) { | ||
| fetchWords(); | ||
| } | ||
| const scrollableDiv = document.querySelector('.flex-1.overflow-y-auto.bg-base-100'); | ||
| if (scrollableDiv) { | ||
| scrollableDiv.scrollTop = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix non-strict comparison and use Svelte binding for DOM access
The function is using a non-strict comparison and directly accessing the DOM with querySelector instead of using Svelte bindings.
function switchLanguage(language) {
selectedLanguageStore.set(language);
selectedLetter = currentAlphabet[0];
- if (selectedLanguage != vernacularLanguage) {
+ if (selectedLanguage !== vernacularLanguage) {
fetchWords();
}
- const scrollableDiv = document.querySelector('.flex-1.overflow-y-auto.bg-base-100');
- if (scrollableDiv) {
- scrollableDiv.scrollTop = 0;
- }
+ if (scrollContainer) {
+ scrollContainer.scrollTop = 0;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function switchLanguage(language) { | |
| selectedLanguageStore.set(language); | |
| selectedLetter = currentAlphabet[0]; | |
| if (selectedLanguage != vernacularLanguage) { | |
| fetchWords(); | |
| } | |
| const scrollableDiv = document.querySelector('.flex-1.overflow-y-auto.bg-base-100'); | |
| if (scrollableDiv) { | |
| scrollableDiv.scrollTop = 0; | |
| } | |
| } | |
| function switchLanguage(language) { | |
| selectedLanguageStore.set(language); | |
| selectedLetter = currentAlphabet[0]; | |
| if (selectedLanguage !== vernacularLanguage) { | |
| fetchWords(); | |
| } | |
| if (scrollContainer) { | |
| scrollContainer.scrollTop = 0; | |
| } | |
| } |
src/routes/lexicon/+page.svelte
Outdated
| async function fetchWords(letter = selectedLetter) { | ||
| if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { | ||
| console.log('Loading letter data:', letter); | ||
|
|
||
| const letterIndex = alphabets.reversal.indexOf(letter); | ||
| const lettersToLoad = alphabets.reversal | ||
| .slice(0, letterIndex) | ||
| .filter((l) => !loadedReversalLetters.has(l)); | ||
|
|
||
| // Load all required letters in parallel | ||
| await Promise.all(lettersToLoad.map(loadLetterData)); | ||
|
|
||
| // Load the current letter | ||
| await loadLetterData(letter); | ||
|
|
||
| // Sort the results based on the selectedLanguage's alphabet | ||
| reversalWordsStore.update((words) => { | ||
| const updatedWords = { ...words }; | ||
| updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( | ||
| (a, b) => { | ||
| const alphabet = currentAlphabet; | ||
| return ( | ||
| alphabet.indexOf(a.word[0].toLowerCase()) - | ||
| alphabet.indexOf(b.word[0].toLowerCase()) | ||
| ); | ||
| } | ||
| ); | ||
| return updatedWords; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to fetchWords function
The fetchWords function lacks error handling for potential failures during letter data loading. If any of the letter data loading operations fail, the function will throw an unhandled exception, potentially breaking the UI.
async function fetchWords(letter = selectedLetter) {
if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) {
console.log('Loading letter data:', letter);
const letterIndex = alphabets.reversal.indexOf(letter);
const lettersToLoad = alphabets.reversal
.slice(0, letterIndex)
.filter((l) => !loadedReversalLetters.has(l));
// Load all required letters in parallel
+ try {
await Promise.all(lettersToLoad.map(loadLetterData));
// Load the current letter
await loadLetterData(letter);
// Sort the results based on the selectedLanguage's alphabet
reversalWordsStore.update((words) => {
const updatedWords = { ...words };
updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort(
(a, b) => {
const alphabet = currentAlphabet;
return (
alphabet.indexOf(a.word[0].toLowerCase()) -
alphabet.indexOf(b.word[0].toLowerCase())
);
}
);
return updatedWords;
});
+ } catch (error) {
+ console.error('Error loading letter data:', error);
+ // Consider showing a user-friendly error message
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function fetchWords(letter = selectedLetter) { | |
| if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { | |
| console.log('Loading letter data:', letter); | |
| const letterIndex = alphabets.reversal.indexOf(letter); | |
| const lettersToLoad = alphabets.reversal | |
| .slice(0, letterIndex) | |
| .filter((l) => !loadedReversalLetters.has(l)); | |
| // Load all required letters in parallel | |
| await Promise.all(lettersToLoad.map(loadLetterData)); | |
| // Load the current letter | |
| await loadLetterData(letter); | |
| // Sort the results based on the selectedLanguage's alphabet | |
| reversalWordsStore.update((words) => { | |
| const updatedWords = { ...words }; | |
| updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( | |
| (a, b) => { | |
| const alphabet = currentAlphabet; | |
| return ( | |
| alphabet.indexOf(a.word[0].toLowerCase()) - | |
| alphabet.indexOf(b.word[0].toLowerCase()) | |
| ); | |
| } | |
| ); | |
| return updatedWords; | |
| }); | |
| } | |
| } | |
| async function fetchWords(letter = selectedLetter) { | |
| if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { | |
| console.log('Loading letter data:', letter); | |
| const letterIndex = alphabets.reversal.indexOf(letter); | |
| const lettersToLoad = alphabets.reversal | |
| .slice(0, letterIndex) | |
| .filter((l) => !loadedReversalLetters.has(l)); | |
| // Load all required letters in parallel | |
| try { | |
| await Promise.all(lettersToLoad.map(loadLetterData)); | |
| // Load the current letter | |
| await loadLetterData(letter); | |
| // Sort the results based on the selectedLanguage's alphabet | |
| reversalWordsStore.update((words) => { | |
| const updatedWords = { ...words }; | |
| updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( | |
| (a, b) => { | |
| const alphabet = currentAlphabet; | |
| return ( | |
| alphabet.indexOf(a.word[0].toLowerCase()) - | |
| alphabet.indexOf(b.word[0].toLowerCase()) | |
| ); | |
| } | |
| ); | |
| return updatedWords; | |
| }); | |
| } catch (error) { | |
| console.error('Error loading letter data:', error); | |
| // Consider showing a user-friendly error message | |
| } | |
| } | |
| } |
src/routes/lexicon/+page.svelte
Outdated
| async function loadLetterData(letter) { | ||
| 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` | ||
| ); | ||
| return null; // Return null for missing indexes | ||
| } | ||
| }) | ||
| .filter((index) => index !== null), // Filter out null values | ||
| letter: letter | ||
| }; | ||
| }); | ||
|
|
||
| currentFileWords.forEach((newWord) => { | ||
| const existingWord = newWords.find((w) => w.word === newWord.word); | ||
| if (existingWord) { | ||
| existingWord.indexes = [ | ||
| ...new Set([...existingWord.indexes, ...newWord.indexes]) | ||
| ]; | ||
| } else { | ||
| newWords.push(newWord); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| reversalWordsStore.update((words) => { | ||
| const updatedWords = { ...words }; | ||
| updatedWords[selectedLanguage] = [ | ||
| ...(updatedWords[selectedLanguage] || []), | ||
| ...newWords | ||
| ]; | ||
| return updatedWords; | ||
| }); | ||
|
|
||
| reversalLettersStore.update((letters) => { | ||
| const updatedLetters = { ...letters }; | ||
| updatedLetters[selectedLanguage] = [ | ||
| ...(updatedLetters[selectedLanguage] || []), | ||
| letter | ||
| ]; | ||
| return updatedLetters; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and remove console.log in loadLetterData
The function contains a console.log statement for debugging and lacks proper error handling for failed responses or potential issues with JSON parsing.
async function loadLetterData(letter) {
let newWords = [];
const index = reversalIndexes[defaultReversalKey];
const files = index[letter] || [];
for (const file of files) {
const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`;
+ try {
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`
- );
+ // Silently ignore missing indices or log to a dedicated logging system
return null; // Return null for missing indexes
}
})
.filter((index) => index !== null), // Filter out null values
letter: letter
};
});
currentFileWords.forEach((newWord) => {
const existingWord = newWords.find((w) => w.word === newWord.word);
if (existingWord) {
existingWord.indexes = [
...new Set([...existingWord.indexes, ...newWord.indexes])
];
} else {
newWords.push(newWord);
}
});
+ } else {
+ console.error(`Failed to fetch file: ${reversalFile}. Status: ${response.status}`);
+ }
+ } catch (error) {
+ console.error(`Error loading data for letter ${letter} from ${file}:`, error);
+ }
}
reversalWordsStore.update((words) => {
const updatedWords = { ...words };
updatedWords[selectedLanguage] = [
...(updatedWords[selectedLanguage] || []),
...newWords
];
return updatedWords;
});
reversalLettersStore.update((letters) => {
const updatedLetters = { ...letters };
updatedLetters[selectedLanguage] = [
...(updatedLetters[selectedLanguage] || []),
letter
];
return updatedLetters;
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function loadLetterData(letter) { | |
| 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` | |
| ); | |
| return null; // Return null for missing indexes | |
| } | |
| }) | |
| .filter((index) => index !== null), // Filter out null values | |
| letter: letter | |
| }; | |
| }); | |
| currentFileWords.forEach((newWord) => { | |
| const existingWord = newWords.find((w) => w.word === newWord.word); | |
| if (existingWord) { | |
| existingWord.indexes = [ | |
| ...new Set([...existingWord.indexes, ...newWord.indexes]) | |
| ]; | |
| } else { | |
| newWords.push(newWord); | |
| } | |
| }); | |
| } | |
| } | |
| reversalWordsStore.update((words) => { | |
| const updatedWords = { ...words }; | |
| updatedWords[selectedLanguage] = [ | |
| ...(updatedWords[selectedLanguage] || []), | |
| ...newWords | |
| ]; | |
| return updatedWords; | |
| }); | |
| reversalLettersStore.update((letters) => { | |
| const updatedLetters = { ...letters }; | |
| updatedLetters[selectedLanguage] = [ | |
| ...(updatedLetters[selectedLanguage] || []), | |
| letter | |
| ]; | |
| return updatedLetters; | |
| }); | |
| } | |
| async function loadLetterData(letter) { | |
| let newWords = []; | |
| const index = reversalIndexes[defaultReversalKey]; | |
| const files = index[letter] || []; | |
| for (const file of files) { | |
| const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`; | |
| try { | |
| 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 { | |
| // Silently ignore missing indices or log to a dedicated logging system | |
| return null; // Return null for missing indexes | |
| } | |
| }) | |
| .filter((index) => index !== null), // Filter out null values | |
| letter: letter | |
| }; | |
| }); | |
| currentFileWords.forEach((newWord) => { | |
| const existingWord = newWords.find((w) => w.word === newWord.word); | |
| if (existingWord) { | |
| existingWord.indexes = [ | |
| ...new Set([...existingWord.indexes, ...newWord.indexes]) | |
| ]; | |
| } else { | |
| newWords.push(newWord); | |
| } | |
| }); | |
| } else { | |
| console.error(`Failed to fetch file: ${reversalFile}. Status: ${response.status}`); | |
| } | |
| } catch (error) { | |
| console.error(`Error loading data for letter ${letter} from ${file}:`, error); | |
| } | |
| } | |
| reversalWordsStore.update((words) => { | |
| const updatedWords = { ...words }; | |
| updatedWords[selectedLanguage] = [ | |
| ...(updatedWords[selectedLanguage] || []), | |
| ...newWords | |
| ]; | |
| return updatedWords; | |
| }); | |
| reversalLettersStore.update((letters) => { | |
| const updatedLetters = { ...letters }; | |
| updatedLetters[selectedLanguage] = [ | |
| ...(updatedLetters[selectedLanguage] || []), | |
| letter | |
| ]; | |
| return updatedLetters; | |
| }); | |
| } |
src/routes/lexicon/+page.svelte
Outdated
| async function checkIfScrolledToBottom(event) { | ||
| if (isFetching) return; | ||
|
|
||
| if ( | ||
| (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) || | ||
| (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0) | ||
| ) { | ||
| let div = event.target; | ||
| const threshold = 100; | ||
|
|
||
| if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) { | ||
| const currentIndex = currentAlphabet.indexOf(selectedLetter); | ||
| if (!loadedReversalLetters.has(currentAlphabet[currentIndex + 1])) { | ||
| if (currentIndex < currentAlphabet.length - 1) { | ||
| isFetching = true; | ||
| await fetchWords(currentAlphabet[currentIndex + 1]); | ||
| isFetching = false; | ||
| } | ||
| } | ||
| } else if ( | ||
| (selectedLanguage === reversalLanguage && | ||
| loadedReversalLetters.has(selectedLetter)) || | ||
| selectedLanguage === vernacularLanguage | ||
| ) { | ||
| const allLetters = div.querySelectorAll('[id^="letter-"]'); | ||
| let visibleLetter = null; | ||
|
|
||
| allLetters.forEach((letterDiv) => { | ||
| const rect = letterDiv.getBoundingClientRect(); | ||
| if (rect.top >= 0 && rect.bottom <= window.innerHeight) { | ||
| visibleLetter = letterDiv.id.split('-')[1]; | ||
| } | ||
| }); | ||
|
|
||
| if (visibleLetter) { | ||
| selectedLetter = visibleLetter; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify scroll handling logic and add bounds checking
The checkIfScrolledToBottom function is overly complex and doesn't properly check array bounds when accessing the next letter.
async function checkIfScrolledToBottom(event) {
if (isFetching) return;
if (
(selectedLanguage === reversalLanguage && reversalWordsList.length > 0) ||
(selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0)
) {
let div = event.target;
const threshold = 100;
if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) {
const currentIndex = currentAlphabet.indexOf(selectedLetter);
- if (!loadedReversalLetters.has(currentAlphabet[currentIndex + 1])) {
- if (currentIndex < currentAlphabet.length - 1) {
+ if (currentIndex < currentAlphabet.length - 1) {
+ const nextLetter = currentAlphabet[currentIndex + 1];
+ if (!loadedReversalLetters.has(nextLetter)) {
isFetching = true;
- await fetchWords(currentAlphabet[currentIndex + 1]);
+ await fetchWords(nextLetter);
isFetching = false;
}
}
} else if (
(selectedLanguage === reversalLanguage &&
loadedReversalLetters.has(selectedLetter)) ||
selectedLanguage === vernacularLanguage
) {
const allLetters = div.querySelectorAll('[id^="letter-"]');
let visibleLetter = null;
+ // Find the first letter that is at least partially visible
allLetters.forEach((letterDiv) => {
const rect = letterDiv.getBoundingClientRect();
- if (rect.top >= 0 && rect.bottom <= window.innerHeight) {
+ if (rect.top <= window.innerHeight && rect.bottom >= 0) {
visibleLetter = letterDiv.id.split('-')[1];
}
});
if (visibleLetter) {
selectedLetter = visibleLetter;
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function checkIfScrolledToBottom(event) { | |
| if (isFetching) return; | |
| if ( | |
| (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) || | |
| (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0) | |
| ) { | |
| let div = event.target; | |
| const threshold = 100; | |
| if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) { | |
| const currentIndex = currentAlphabet.indexOf(selectedLetter); | |
| if (!loadedReversalLetters.has(currentAlphabet[currentIndex + 1])) { | |
| if (currentIndex < currentAlphabet.length - 1) { | |
| isFetching = true; | |
| await fetchWords(currentAlphabet[currentIndex + 1]); | |
| isFetching = false; | |
| } | |
| } | |
| } else if ( | |
| (selectedLanguage === reversalLanguage && | |
| loadedReversalLetters.has(selectedLetter)) || | |
| selectedLanguage === vernacularLanguage | |
| ) { | |
| const allLetters = div.querySelectorAll('[id^="letter-"]'); | |
| let visibleLetter = null; | |
| allLetters.forEach((letterDiv) => { | |
| const rect = letterDiv.getBoundingClientRect(); | |
| if (rect.top >= 0 && rect.bottom <= window.innerHeight) { | |
| visibleLetter = letterDiv.id.split('-')[1]; | |
| } | |
| }); | |
| if (visibleLetter) { | |
| selectedLetter = visibleLetter; | |
| } | |
| } | |
| } | |
| } | |
| async function checkIfScrolledToBottom(event) { | |
| if (isFetching) return; | |
| if ( | |
| (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) || | |
| (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0) | |
| ) { | |
| let div = event.target; | |
| const threshold = 100; | |
| if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) { | |
| const currentIndex = currentAlphabet.indexOf(selectedLetter); | |
| if (currentIndex < currentAlphabet.length - 1) { | |
| const nextLetter = currentAlphabet[currentIndex + 1]; | |
| if (!loadedReversalLetters.has(nextLetter)) { | |
| isFetching = true; | |
| await fetchWords(nextLetter); | |
| isFetching = false; | |
| } | |
| } | |
| } else if ( | |
| (selectedLanguage === reversalLanguage && | |
| loadedReversalLetters.has(selectedLetter)) || | |
| selectedLanguage === vernacularLanguage | |
| ) { | |
| const allLetters = div.querySelectorAll('[id^="letter-"]'); | |
| let visibleLetter = null; | |
| // Find the first letter that is at least partially visible | |
| allLetters.forEach((letterDiv) => { | |
| const rect = letterDiv.getBoundingClientRect(); | |
| if (rect.top <= window.innerHeight && rect.bottom >= 0) { | |
| visibleLetter = letterDiv.id.split('-')[1]; | |
| } | |
| }); | |
| if (visibleLetter) { | |
| selectedLetter = visibleLetter; | |
| } | |
| } | |
| } | |
| } |
Load Data from SQL for search Issue #863 work in progress for dab search Issue #683 Added search button Search UI changes, Hamburger menu, Back navigation Added Search sub-bar Removed hardcoded color for search Issue #683 Added special characters, whole words, accents and tones options to DAB search First full implementation of Search Continued work on search Finished select word support for search Freeze header on search scroll Fix search icon misalign Freeze word navigation strip Added support for multi-word searching. Greatly improved efficiency Formatting
EthanFennell
force-pushed
the
feature/search/683
branch
from
4328c85 to
69166fa
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/routes/lexicon/+page.svelte
Outdated
| let scrollContainer; | ||
| let wordIds; | ||
| let selectedLanguage; | ||
|
|
||
| // Subscribe to stores | ||
| currentReversalLettersStore.subscribe((value) => (loadedReversalLetters = new Set(value))); |
There was a problem hiding this comment.
I didn't notice this code from the previous PRs. This is not the correct way to use stores. You shouldn't be directly subscribing to them from a .svelte file. You should be using the $store syntax which does auto subscription/unsubscription.
| {#if selectedWord} | ||
| {#if !showBackButton} | ||
| {(showBackButton = true)} | ||
| {/if} |
There was a problem hiding this comment.
I am not quite sure what you are trying to achieve with showBackButton, but this is not the way to do it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/routes/lexicon/+page.svelte (7)
186-196: 🛠️ Refactor suggestionFix non-strict comparison and use Svelte binding for DOM access
The
switchLanguagefunction has two issues:
- It uses non-strict comparison (
!=) instead of strict comparison (!==)- It directly accesses the DOM with
querySelectorinstead of using the Svelte bindingApply these changes:
function switchLanguage(language) { selectedLanguageStore.set(language); selectedLetter = currentAlphabet[0]; - if (selectedLanguage != vernacularLanguage) { + if (selectedLanguage !== vernacularLanguage) { fetchWords(); } - const scrollableDiv = document.querySelector('.flex-1.overflow-y-auto.bg-base-100'); - if (scrollableDiv) { - scrollableDiv.scrollTop = 0; - } + if (scrollContainer) { + scrollContainer.scrollTop = 0; + } }
90-156: 🛠️ Refactor suggestionImprove error handling in loadLetterData function
The function lacks proper error handling for failed responses and contains console.log statements for debugging.
Add try-catch blocks and improve error handling:
async function loadLetterData(letter) { let newWords = []; const index = reversalIndexes[defaultReversalKey]; const files = index[letter] || []; for (const file of files) { const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`; + try { 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` - ); + // Silently ignore missing indices or log to a dedicated logging system return null; // Return null for missing indexes } }) .filter((index) => index !== null), // Filter out null values letter: letter }; }); currentFileWords.forEach((newWord) => { const existingWord = newWords.find((w) => w.word === newWord.word); if (existingWord) { existingWord.indexes = [ ...new Set([...existingWord.indexes, ...newWord.indexes]) ]; } else { newWords.push(newWord); } }); + } else { + console.error(`Failed to fetch file: ${reversalFile}. Status: ${response.status}`); + } + } catch (error) { + console.error(`Error loading data for letter ${letter} from ${file}:`, error); + } } // Update stores with the new data // ...rest of function remains unchanged
58-88: 🛠️ Refactor suggestionAdd error handling to fetchWords function
The
fetchWordsfunction lacks error handling for potential failures during letter data loading.Add a try-catch block:
async function fetchWords(letter = selectedLetter) { if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { console.log('Loading letter data:', letter); const letterIndex = alphabets.reversal.indexOf(letter); const lettersToLoad = alphabets.reversal .slice(0, letterIndex) .filter((l) => !loadedReversalLetters.has(l)); // Load all required letters in parallel + try { await Promise.all(lettersToLoad.map(loadLetterData)); // Load the current letter await loadLetterData(letter); // Sort the results based on the selectedLanguage's alphabet reversalWordsStore.update((words) => { const updatedWords = { ...words }; updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( (a, b) => { const alphabet = currentAlphabet; return ( alphabet.indexOf(a.word[0].toLowerCase()) - alphabet.indexOf(b.word[0].toLowerCase()) ); } ); return updatedWords; }); + } catch (error) { + console.error('Error loading letter data:', error); + // Consider showing a user-friendly error message + } } }
200-239: Simplify scroll handling logic and add bounds checkingThe
checkIfScrolledToBottomfunction has overly complex logic and doesn't properly check array bounds.Simplify the logic:
async function checkIfScrolledToBottom(event) { if (isFetching) return; if ( (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) || (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0) ) { let div = event.target; const threshold = 100; if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) { const currentIndex = currentAlphabet.indexOf(selectedLetter); - if (!loadedReversalLetters.has(currentAlphabet[currentIndex + 1])) { - if (currentIndex < currentAlphabet.length - 1) { + if (currentIndex < currentAlphabet.length - 1) { + const nextLetter = currentAlphabet[currentIndex + 1]; + if (!loadedReversalLetters.has(nextLetter)) { isFetching = true; - await fetchWords(currentAlphabet[currentIndex + 1]); + await fetchWords(nextLetter); isFetching = false; } } } else if ( (selectedLanguage === reversalLanguage && loadedReversalLetters.has(selectedLetter)) || selectedLanguage === vernacularLanguage ) { const allLetters = div.querySelectorAll('[id^="letter-"]'); let visibleLetter = null; + // Find the first letter that is at least partially visible allLetters.forEach((letterDiv) => { const rect = letterDiv.getBoundingClientRect(); - if (rect.top >= 0 && rect.bottom <= window.innerHeight) { + if (rect.top <= window.innerHeight && rect.bottom >= 0) { visibleLetter = letterDiv.id.split('-')[1]; } }); if (visibleLetter) { selectedLetter = visibleLetter; } } } }
46-54: 🛠️ Refactor suggestionUse
$storesyntax instead of direct subscriptionsDirect subscription to stores in Svelte components is not the recommended pattern. This approach has several drawbacks:
- Manual subscription management leading to potential memory leaks
- More verbose code than necessary
- Doesn't leverage Svelte's built-in reactivity system
Replace the direct subscriptions with the
$storesyntax:-// Subscribe to stores -currentReversalLettersStore.subscribe((value) => (loadedReversalLetters = new Set(value))); -currentReversalWordsStore.subscribe((value) => (reversalWordsList = value)); -vernacularLanguageStore.subscribe((value) => (vernacularLanguage = value)); -vernacularWordsStore.subscribe((value) => (vernacularWordsList = value)); - -selectedLanguageStore.subscribe((value) => { - selectedLanguage = value; -}); -selectedLanguageStore.set(vernacularLanguage); +// Use Svelte's auto-subscription syntax +$: loadedReversalLetters = new Set($currentReversalLettersStore); +$: reversalWordsList = $currentReversalWordsStore; +$: vernacularLanguage = $vernacularLanguageStore; +$: vernacularWordsList = $vernacularWordsStore; +$: selectedLanguage = $selectedLanguageStore; + +// Initialize selectedLanguageStore +onMount(() => { + selectedLanguageStore.set(vernacularLanguage); +});
158-161:⚠️ Potential issueFix potential null reference in selectWord function
The
selectWordfunction doesn't properly handle the case whereselectedWordcould be null after toggling.Fix the potential null reference by adding a null check:
function selectWord(word) { selectedWord = selectedWord && selectedWord.word === word ? null : word; - wordIds = selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]; + wordIds = selectedWord ? (selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]) : null; }
297-301: 🛠️ Refactor suggestionRemove assignment in conditional template
Using an assignment inside a conditional template is a bad practice and can lead to maintenance issues.
Remove the assignment from the template:
{#if selectedWord} - {#if !showBackButton} - {(showBackButton = true)} - {/if} <WordNavigationStrip currentWord={selectedWord} onSelectWord={selectWord} />Instead, consider updating the reactive declaration for
showBackButtonto ensure it's correctly derived fromselectedWord:-$: showBackButton = selectedWord ? true : false; +$: showBackButton = !!selectedWord;
🧹 Nitpick comments (1)
src/routes/lexicon/+page.svelte (1)
35-35: Simplify reactive declaration for showBackButtonThe current declaration uses a redundant ternary operator.
Simplify the reactive declaration:
-$: showBackButton = selectedWord ? true : false; +$: showBackButton = !!selectedWord;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/LexiconXMLView.svelte(1 hunks)src/routes/lexicon/+page.svelte(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/LexiconXMLView.svelte
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
src/routes/lexicon/+page.svelte (2)
260-283: Improve the search button implementationThe search button implementation is well-structured but could benefit from some minor improvements.
The search button is a good addition that enhances the user experience by providing easy access to the search functionality. The code is generally well-written, using the new
getRoutefunction for navigation and properly resettingwordIdsbefore navigation.
285-328: Approve the conditional rendering of lexicon viewsThe conditional rendering of lexicon views based on the selected word and language is well-implemented.
The code effectively uses conditional rendering to display different views based on the user's selection. The structure is clean and follows good Svelte practices by using consistent binding and event handling across all conditions.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/routes/lexicon/+page.svelte (1)
245-245: 🛠️ Refactor suggestionFix non-strict comparison in onMount function
Similar to the issue in
switchLanguage, this function is using a non-strict comparison.-if (selectedLetter && selectedLanguage != vernacularLanguage) { +if (selectedLetter && selectedLanguage !== vernacularLanguage) { fetchWords(); }
♻️ Duplicate comments (8)
src/routes/lexicon/+page.svelte (8)
186-196: 🛠️ Refactor suggestionFix non-strict comparison and use Svelte binding for DOM access
The function is using a non-strict comparison and directly accessing the DOM with querySelector instead of using Svelte bindings.
function switchLanguage(language) { selectedLanguageStore.set(language); selectedLetter = currentAlphabet[0]; - if (selectedLanguage != vernacularLanguage) { + if (selectedLanguage !== vernacularLanguage) { fetchWords(); } - const scrollableDiv = document.querySelector('.flex-1.overflow-y-auto.bg-base-100'); - if (scrollableDiv) { - scrollableDiv.scrollTop = 0; - } + if (scrollContainer) { + scrollContainer.scrollTop = 0; + } }
58-88: 🛠️ Refactor suggestionAdd error handling to fetchWords function
The
fetchWordsfunction lacks error handling for potential failures during letter data loading.async function fetchWords(letter = selectedLetter) { if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { console.log('Loading letter data:', letter); const letterIndex = alphabets.reversal.indexOf(letter); const lettersToLoad = alphabets.reversal .slice(0, letterIndex) .filter((l) => !loadedReversalLetters.has(l)); // Load all required letters in parallel + try { await Promise.all(lettersToLoad.map(loadLetterData)); // Load the current letter await loadLetterData(letter); // Sort the results based on the selectedLanguage's alphabet reversalWordsStore.update((words) => { const updatedWords = { ...words }; updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( (a, b) => { const alphabet = currentAlphabet; return ( alphabet.indexOf(a.word[0].toLowerCase()) - alphabet.indexOf(b.word[0].toLowerCase()) ); } ); return updatedWords; }); + } catch (error) { + console.error('Error loading letter data:', error); + isFetching = false; // Make sure to reset the flag in case of error + // Consider showing a user-friendly error message + } } }
90-156: 🛠️ Refactor suggestionImprove error handling in loadLetterData function
The
loadLetterDatafunction lacks proper error handling for the fetch operation and contains debugging console logs.async function loadLetterData(letter) { let newWords = []; const index = reversalIndexes[defaultReversalKey]; const files = index[letter] || []; for (const file of files) { const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`; + try { 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` - ); + // Silently ignore missing indices or log to a dedicated logging system return null; // Return null for missing indexes } }) .filter((index) => index !== null), // Filter out null values letter: letter }; }); currentFileWords.forEach((newWord) => { const existingWord = newWords.find((w) => w.word === newWord.word); if (existingWord) { existingWord.indexes = [ ...new Set([...existingWord.indexes, ...newWord.indexes]) ]; } else { newWords.push(newWord); } }); + } else { + console.error(`Failed to fetch file: ${reversalFile}. Status: ${response.status}`); + } + } catch (error) { + console.error(`Error loading data for letter ${letter} from ${file}:`, error); + } } // Continue with store updates...
200-239: 🛠️ Refactor suggestionSimplify scroll handling logic and add bounds checking
The
checkIfScrolledToBottomfunction has issues with scroll logic and doesn't properly check array bounds when accessing the next letter.async function checkIfScrolledToBottom(event) { if (isFetching) return; if ( (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) || (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0) ) { let div = event.target; const threshold = 100; if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) { const currentIndex = currentAlphabet.indexOf(selectedLetter); - if (!loadedReversalLetters.has(currentAlphabet[currentIndex + 1])) { - if (currentIndex < currentAlphabet.length - 1) { + if (currentIndex < currentAlphabet.length - 1) { + const nextLetter = currentAlphabet[currentIndex + 1]; + if (!loadedReversalLetters.has(nextLetter)) { isFetching = true; - await fetchWords(currentAlphabet[currentIndex + 1]); + await fetchWords(nextLetter); isFetching = false; } } } else if ( (selectedLanguage === reversalLanguage && loadedReversalLetters.has(selectedLetter)) || selectedLanguage === vernacularLanguage ) { const allLetters = div.querySelectorAll('[id^="letter-"]'); let visibleLetter = null; + // Find the first letter that is at least partially visible allLetters.forEach((letterDiv) => { const rect = letterDiv.getBoundingClientRect(); - if (rect.top >= 0 && rect.bottom <= window.innerHeight) { + if (rect.top <= window.innerHeight && rect.bottom >= 0) { visibleLetter = letterDiv.id.split('-')[1]; } }); if (visibleLetter) { selectedLetter = visibleLetter; } } } }
51-54:⚠️ Potential issueStore subscription should use Svelte's reactive syntax
Direct store subscription in Svelte components can lead to memory leaks as these subscriptions aren't automatically cleaned up. Use Svelte's reactive
$storesyntax instead, which handles subscriptions and unsubscriptions automatically.-selectedLanguageStore.subscribe((value) => { - selectedLanguage = value; -}); -selectedLanguageStore.set(vernacularLanguage); +$: selectedLanguage = $selectedLanguageStore; + +onMount(() => { + selectedLanguageStore.set(vernacularLanguage); +});
46-49:⚠️ Potential issueReplace direct store subscriptions with Svelte's reactive syntax
Similar to the issue above, all store subscriptions should use Svelte's reactive
$storesyntax instead of direct subscriptions.-// Subscribe to stores -currentReversalLettersStore.subscribe((value) => (loadedReversalLetters = new Set(value))); -currentReversalWordsStore.subscribe((value) => (reversalWordsList = value)); -vernacularLanguageStore.subscribe((value) => (vernacularLanguage = value)); -vernacularWordsStore.subscribe((value) => (vernacularWordsList = value)); +// Using Svelte's reactive store syntax +$: loadedReversalLetters = new Set($currentReversalLettersStore); +$: reversalWordsList = $currentReversalWordsStore; +$: vernacularLanguage = $vernacularLanguageStore; +$: vernacularWordsList = $vernacularWordsStore;
158-161:⚠️ Potential issueFix potential null reference in selectWord function
The
selectWordfunction doesn't properly handle the case whereselectedWordcould be null after toggling.function selectWord(word) { selectedWord = selectedWord && selectedWord.word === word ? null : word; - wordIds = selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]; + wordIds = selectedWord ? (selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]) : null; }
297-301:⚠️ Potential issueRemove assignment in conditional template
Using an assignment inside a conditional template is a bad practice and can lead to maintenance issues.
{#if selectedWord} - {#if !showBackButton} - {(showBackButton = true)} - {/if} <WordNavigationStrip currentWord={selectedWord} onSelectWord={selectWord} />Since you're already setting
showBackButtonreactively on line 35, this assignment is unnecessary.
🧹 Nitpick comments (1)
src/routes/lexicon/+page.svelte (1)
35-35: Simplify reactive declarationThis reactive declaration can be simplified.
-$: showBackButton = selectedWord ? true : false; +$: showBackButton = !!selectedWord;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/LexiconReversalListView.svelte(1 hunks)src/lib/components/LexiconVernacularListView.svelte(1 hunks)src/lib/components/LexiconXMLView.svelte(1 hunks)src/routes/lexicon/+page.svelte(7 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/lib/components/LexiconReversalListView.svelte
- src/lib/components/LexiconVernacularListView.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/LexiconXMLView.svelte
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (2)
src/routes/lexicon/+page.svelte (2)
269-281: The search button implementation looks good!The addition of the search button with proper navigation to the search page is well-implemented. It correctly handles setting wordIds to null before navigating, which prevents state issues.
254-259: Good use of conditional classes for layoutThe implementation of conditional grid rows based on whether a word is selected is a good practice that improves layout management.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/routes/lexicon/+page.svelte (7)
58-88: Add error handling to fetchWords functionThe function lacks error handling for potential failures during letter data loading. If any operation fails, it would throw an unhandled exception.
async function fetchWords(letter = selectedLetter) { if (selectedLanguage === reversalLanguage && !loadedReversalLetters.has(letter)) { console.log('Loading letter data:', letter); const letterIndex = alphabets.reversal.indexOf(letter); const lettersToLoad = alphabets.reversal .slice(0, letterIndex) .filter((l) => !loadedReversalLetters.has(l)); // Load all required letters in parallel + try { await Promise.all(lettersToLoad.map(loadLetterData)); // Load the current letter await loadLetterData(letter); // Sort the results based on the selectedLanguage's alphabet reversalWordsStore.update((words) => { const updatedWords = { ...words }; updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( (a, b) => { const alphabet = currentAlphabet; return ( alphabet.indexOf(a.word[0].toLowerCase()) - alphabet.indexOf(b.word[0].toLowerCase()) ); } ); return updatedWords; }); + } catch (error) { + console.error('Error loading letter data:', error); + isFetching = false; // Ensure we reset fetching state on error + } } }
90-156: Improve error handling and remove console.log in loadLetterDataThe function lacks proper error handling for failed responses or potential issues with network requests and JSON parsing.
async function loadLetterData(letter) { let newWords = []; const index = reversalIndexes[defaultReversalKey]; const files = index[letter] || []; for (const file of files) { const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`; + try { 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` - ); + // Silently ignore missing indices or log to a dedicated logging system return null; // Return null for missing indexes } }) .filter((index) => index !== null), // Filter out null values letter: letter }; }); currentFileWords.forEach((newWord) => { const existingWord = newWords.find((w) => w.word === newWord.word); if (existingWord) { existingWord.indexes = [ ...new Set([...existingWord.indexes, ...newWord.indexes]) ]; } else { newWords.push(newWord); } }); + } else { + console.error(`Failed to fetch file: ${reversalFile}. Status: ${response.status}`); + } + } catch (error) { + console.error(`Error loading data for letter ${letter} from ${file}:`, error); + } } reversalWordsStore.update((words) => { const updatedWords = { ...words }; updatedWords[selectedLanguage] = [ ...(updatedWords[selectedLanguage] || []), ...newWords ]; return updatedWords; }); reversalLettersStore.update((letters) => { const updatedLetters = { ...letters }; updatedLetters[selectedLanguage] = [ ...(updatedLetters[selectedLanguage] || []), letter ]; return updatedLetters; }); }
158-161: Fix potential null reference in selectWord functionThe
selectWordfunction doesn't properly handle the case whereselectedWordcould be null after toggling.function selectWord(word) { selectedWord = selectedWord && selectedWord.word === word ? null : word; - wordIds = selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]; + wordIds = selectedWord ? (selectedWord.indexes ? selectedWord.indexes : [selectedWord.index]) : null; }
186-196: Fix non-strict comparison and use Svelte binding for DOM accessThe function is using a non-strict comparison and directly accessing the DOM with querySelector instead of using Svelte bindings.
function switchLanguage(language) { selectedLanguageStore.set(language); selectedLetter = currentAlphabet[0]; - if (selectedLanguage != vernacularLanguage) { + if (selectedLanguage !== vernacularLanguage) { fetchWords(); } - const scrollableDiv = document.querySelector('.flex-1.overflow-y-auto.bg-base-100'); - if (scrollableDiv) { - scrollableDiv.scrollTop = 0; - } + if (scrollContainer) { + scrollContainer.scrollTop = 0; + } }
200-239: Simplify scroll handling logic and add bounds checkingThe
checkIfScrolledToBottomfunction is overly complex and doesn't properly check array bounds when accessing the next letter.async function checkIfScrolledToBottom(event) { if (isFetching) return; if ( (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) || (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0) ) { let div = event.target; const threshold = 100; if (div.scrollHeight - div.scrollTop - div.clientHeight < threshold) { const currentIndex = currentAlphabet.indexOf(selectedLetter); - if (!loadedReversalLetters.has(currentAlphabet[currentIndex + 1])) { - if (currentIndex < currentAlphabet.length - 1) { + if (currentIndex < currentAlphabet.length - 1) { + const nextLetter = currentAlphabet[currentIndex + 1]; + if (!loadedReversalLetters.has(nextLetter)) { isFetching = true; - await fetchWords(currentAlphabet[currentIndex + 1]); + await fetchWords(nextLetter); isFetching = false; } } } else if ( (selectedLanguage === reversalLanguage && loadedReversalLetters.has(selectedLetter)) || selectedLanguage === vernacularLanguage ) { const allLetters = div.querySelectorAll('[id^="letter-"]'); let visibleLetter = null; + // Find the first letter that is at least partially visible allLetters.forEach((letterDiv) => { const rect = letterDiv.getBoundingClientRect(); - if (rect.top >= 0 && rect.bottom <= window.innerHeight) { + if (rect.top <= window.innerHeight && rect.bottom >= 0) { visibleLetter = letterDiv.id.split('-')[1]; } }); if (visibleLetter) { selectedLetter = visibleLetter; } } } }
297-302: Remove assignment in conditional templateUsing an assignment inside a conditional template is a bad practice and can lead to maintenance issues.
{#if selectedWord} - {#if !showBackButton} - {(showBackButton = true)} - {/if} <WordNavigationStrip currentWord={selectedWord} onSelectWord={selectWord} />
46-54: 🛠️ Refactor suggestionReplace manual store subscriptions with Svelte's auto-subscription syntax
Manual store subscriptions should be avoided in Svelte components as they don't automatically unsubscribe when the component is destroyed, potentially causing memory leaks. Use Svelte's built-in
$storesyntax instead.-// Subscribe to stores -currentReversalLettersStore.subscribe((value) => (loadedReversalLetters = new Set(value))); -currentReversalWordsStore.subscribe((value) => (reversalWordsList = value)); -vernacularLanguageStore.subscribe((value) => (vernacularLanguage = value)); -vernacularWordsStore.subscribe((value) => (vernacularWordsList = value)); - -selectedLanguageStore.subscribe((value) => { - selectedLanguage = value; -}); +$: loadedReversalLetters = new Set($currentReversalLettersStore); +$: reversalWordsList = $currentReversalWordsStore; +$: vernacularLanguage = $vernacularLanguageStore; +$: vernacularWordsList = $vernacularWordsStore; +$: selectedLanguage = $selectedLanguageStore;
🧹 Nitpick comments (3)
src/routes/lexicon/+page.svelte (3)
35-35: Convert direct reactive assignment to proper Svelte reactive declarationThis line creates a reactive declaration but uses assignment syntax. It's more idiomatic to use Svelte's reactive declaration syntax.
-$: showBackButton = selectedWord ? true : false; +$: showBackButton = !!selectedWord;
245-251: Add non-strict equality check in onMountSimilar to the issue in
switchLanguage, you're using non-strict equality comparison here.onMount(() => { - if (selectedLetter && selectedLanguage != vernacularLanguage) { + if (selectedLetter && selectedLanguage !== vernacularLanguage) { fetchWords(); } if (config.programType !== 'DAB') { goto(`${base}/text`); } });
302-310: Consider simplifying conditional stylingThe container styling is repeated in three different places with almost identical code. Consider extracting this to a common variable or component to reduce duplication.
+<script> + // At the top of your script section + $: containerClasses = "flex-1 overflow-y-auto bg-base-100 width-full"; + $: containerStyles = selectedWord + ? "background-color: var(--PrimaryColor);" + : ""; +</script> {#if selectedWord} <WordNavigationStrip currentWord={selectedWord} onSelectWord={selectWord} /> <div id="container" - class="flex-1 overflow-y-auto bg-base-100 width-full" - style="background-color: var(--PrimaryColor);" + class={containerClasses} + style={containerStyles} bind:this={scrollContainer} on:scroll={checkIfScrolledToBottom} > <LexiconXmlView {wordIds} onSelectWord={selectWord} /> </div>And apply similar changes to the other container divs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/LexiconReversalListView.svelte(1 hunks)src/lib/components/LexiconVernacularListView.svelte(1 hunks)src/lib/components/LexiconXMLView.svelte(1 hunks)src/routes/lexicon/+page.svelte(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/components/LexiconReversalListView.svelte
- src/lib/components/LexiconVernacularListView.svelte
- src/lib/components/LexiconXMLView.svelte
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/components/LexiconEntryView.svelte (1)
110-121: Consider extracting styling logic for improved maintainabilityWhile the current implementation works well, consider extracting the styling logic into a separate function to improve code readability and maintainability as the component grows.
- // I've added appropriate styling based on class name - if (className.includes('sensenumber')) { - output += ` style="color: var(--TextColor1); font-weight: bold;"`; - } else if (className.includes('vernacular')) { - output += ` style="color: var(--TextColor2);"`; - } else if (className.includes('example')) { - output += ` style="color: var(--TextColor3); font-style: italic;"`; - } else if (className.includes('definition')) { - output += ` style="color: var(--TextColor); font-weight: normal;"`; - } + // Apply appropriate styling based on class name + output += getStyleForClassName(className); // Add this function elsewhere in the script function getStyleForClassName(className) { if (className.includes('sensenumber')) { return ` style="color: var(--TextColor1); font-weight: bold;"`; } else if (className.includes('vernacular')) { return ` style="color: var(--TextColor2);"`; } else if (className.includes('example')) { return ` style="color: var(--TextColor3); font-style: italic;"`; } else if (className.includes('definition')) { return ` style="color: var(--TextColor); font-weight: normal;"`; } return ''; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/lib/components/AlphabetStrip.svelte(1 hunks)src/lib/components/LexiconEntryView.svelte(4 hunks)src/lib/components/LexiconReversalListView.svelte(2 hunks)src/lib/components/LexiconVernacularListView.svelte(2 hunks)src/lib/components/LexiconXMLView.svelte(1 hunks)src/lib/components/SearchForm.svelte(4 hunks)src/lib/components/WordNavigationStrip.svelte(2 hunks)src/routes/lexicon/search/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/components/LexiconReversalListView.svelte
- src/lib/components/AlphabetStrip.svelte
- src/lib/components/LexiconVernacularListView.svelte
- src/lib/components/WordNavigationStrip.svelte
- src/lib/components/SearchForm.svelte
- src/routes/lexicon/search/+page.svelte
- src/lib/components/LexiconXMLView.svelte
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (5)
src/lib/components/LexiconEntryView.svelte (5)
64-64: LGTM: Consistent error stylingThe error message now uses the theme's CSS variable (--TextColor6) for error text, ensuring consistent styling across the application.
102-102: LGTM: Improved link styling with CSS variablesReplacing anchor tags with styled spans maintains functionality while ensuring consistent appearance through CSS variables. This is a good approach that follows the theming system.
110-121: LGTM: Conditional styling for semantic elementsGood implementation of conditional styling based on class names using CSS variables. This ensures consistent visual hierarchy across different types of lexical content.
155-156: LGTM: Consistent separator stylingUsing the --SettingsSeparatorColor variable for horizontal rule elements ensures visual consistency between entries.
201-203: LGTM: Consistent container stylingThe pre element now correctly uses theme variables for background and text colors, ensuring the container blends well with the rest of the UI.
* Create route for lexicon page Create component for reversal view Create reusable alphabet strip component Implement initial lexicon reversal interface Add lexicon page data loading Add DAB program type route handling Add missing imports to lexicon and main routing pages lint format for page.js file for lexicon Create first reversal page draft Converted page to components Dynamically load alphabet from dictionary config Dynamically load alphabet from dictionary config Lift language state to parent component Add reversal lanuage name and letter change detection Lazy load English words and base for XML implementation Create Language Tabs Component and Parse lang Issue #685 Moved the code for the language tabs from LexiconReversalView and created a separate component. Also, parsed lang from writing systems and updated index.d.ts with displayLang Change English wording and fix selectedLetter Name vernacular language correctly and organize code Create new index.ts file for lexicon page Name vernacular language correctly and organize code Fix language tab naming convention Removed duplicate code for parsing lang Issue #685 Removed Unnecessary Code and Lexicon Folder Issue #685 Removed hardcoding Switch order of language tabs * Removed page.ts * Changed use of "slot" to "render" for navbar * List of words * Fix mobile layout * Fix lexicon auto load * Fix word id mismatch * Add margins to alphabet strip * Add first draft of vernacular letter id handling * Restore dictionary functionality while preserving styling * Fixed vernacular letter id handling * Create XML element and reorganize * Fix loading of second letter on tab change * Fix alphabet bar navigation bug * Store selectedLanguage after XMLView * Add word subtexts, indexes, and alphabet strip size change * Add word navigation component * Add XML new lines * Add multi-index XML call and new line formatting * Bug fixes * Moved vernacular query to page.js, simplified page.svelte * Revert to threshold * Fixed lazy loading glitches on firefox * Fixed sqlite fetch issue * add single entry style type * Add hyperlinks to headwords in XML * Remove unused variable and comments * show single entry styles * Added stores. Refactored lexicon code. Included reversal/vernacular mapped words in store to improve efficiency * Merge and formatting * Applied suggestions * Small changes * Small changes * miscellaneous small refactors * Format * Fix test * Fix accessibility issues * Add fallback for summary without matches * Fix comma issue. Fix duplicate listeners * Add proper type definition for vernacularWordsStore * Improve store definitions * Vernacular word hyperlink fix * Fix lint issues * Fix language tab to match native app * Remove padding * Applied feedback for language tabs * fix lint * Fix spacing * Adjust word list spacing * Fix navbar hiding on scroll and div organization * Fix more div organizaiton * Spacing fix * Implement lexicon error handling * Implement fetch logic for reversal view * Fix a potential security vulnerability * fix lint * Remove hardcoded colors * Split LexiconReversalListView into separate components. * Remove hardcoded color * Remove alphabetStrip artifact * Add proper reversal file fetch * Remove phantom endpoint call * Add grid view * Remove HEAD request in file loading * Remove unnecessary comments * Add grid view * Add grid view * Remove redundancy * convertReversalIndex create index.json * Use ReversalIndex to load reversal files * Fix wide screen gap * Fix display on wide screen * Rename AlphabetStrip * Rename LexiconReversalView * Rename LexiconXMLView --------- Co-authored-by: EthanFennell <[email protected]> Co-authored-by: Carson Kramer <[email protected]> Co-authored-by: AslanRules <[email protected]> Co-authored-by: Chris Hubbard <[email protected]>
|
Fixed branch issues in #806 |
Implementation of the Search page in DAB including:
Implementation details of the LexiconXMLView were revised to be re-used here in an efficient manner. Queries are returning results of many words quickly now.
Summary by CodeRabbit