Feature/display reversals/685#802
Conversation
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
…ppbuilder-pwa into feature/display-reversals/685
WalkthroughThis set of changes introduces a comprehensive, interactive lexicon browsing experience with multi-language support, dynamic data loading, and detailed entry views. New Svelte components for language tabs, alphabet navigation, vernacular and reversal word lists, entry detail views, and navigation strips are added. The lexicon data management is refactored using new Svelte stores and a database initialization helper. The lexicon page is rewritten to support incremental loading, smooth navigation, and responsive UI updates based on user interactions. Supporting backend and configuration changes include new type definitions, manifest file generation, and test updates to accommodate expanded features and data structures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LexiconPage
participant LexiconStores
participant Components
participant Database
User->>LexiconPage: Loads page
LexiconPage->>Database: initializeDatabase()
Database-->>LexiconStores: Sets up SQL.js and DB
LexiconPage->>LexiconStores: Fetches initial data (vernacular, reversal)
LexiconStores-->>LexiconPage: Provides words, alphabets, indexes
LexiconPage->>Components: Passes data and handlers
User->>Components: Switches language / Selects letter / Selects word
Components->>LexiconPage: onSwitchLanguage / onLetterChange / onSelectWord
LexiconPage->>LexiconStores: Updates selection, fetches more data if needed
LexiconStores-->>LexiconPage: Updated data
LexiconPage->>Components: Updates props
Components->>Database: (if viewing entry) Query for XML data
Database-->>Components: Returns XML entry
Components->>Components: Parse and display entry, handle links
User->>Components: Navigates with navigation strip
Components->>LexiconPage: onSelectWord
LexiconPage->>LexiconStores: Updates current word
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
src/lib/components/LexiconLanguageTabs.svelte (2)
2-5: Add TypeScript types to component props.Consider adding TypeScript type annotations to improve type safety and documentation.
- 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;
8-27: Improve accessibility of language tab buttons.The language tabs would benefit from proper ARIA attributes to improve accessibility for screen readers.
<div class="flex flex-wrap bg-[#e1bee8] p-2 mb-4"> <button on:click={() => onSwitchLanguage(vernacularLanguage)} + role="tab" + aria-selected={selectedLanguage === vernacularLanguage} + aria-controls={`${vernacularLanguage}-panel`} class="px-4 py-2 text-base font-bold text-black uppercase border-b-4 border-transparent cursor-pointer mr-2 mb-2 rounded-md hover:bg-gray-200 {selectedLanguage === vernacularLanguage ? 'bg-[#bb9ac2] border-black' : ''}" > {vernacularLanguage} </button> <button on:click={() => onSwitchLanguage(reversalLanguage)} + role="tab" + aria-selected={selectedLanguage === reversalLanguage} + aria-controls={`${reversalLanguage}-panel`} class="px-4 py-2 text-base font-bold text-black uppercase border-b-4 border-transparent cursor-pointer mr-2 mb-2 rounded-md hover:bg-gray-200 {selectedLanguage === reversalLanguage ? 'bg-[#bb9ac2] border-black' : ''}" > {reversalLanguage} </button> </div>src/lib/components/AlphabetStrip.svelte (1)
7-22: Consider adding an aria-pressed attribute for accessibility
When marking the active letter button, you can add anaria-pressedattribute to help screen readers understand which button is currently selected:<button ... - class="..." + class="..." + aria-pressed={activeLetter === letter} on:click={() => onLetterSelect(letter)} >src/routes/lexicon/+page.js (1)
32-94: Remove or revise console.log calls for production
Lines 42 and 48 contain console logs that may be noisy in production. Consider limiting them to debug builds or removing them entirely to avoid cluttering logs.src/lib/components/LexiconReversalView.svelte (2)
14-16: Potential conflict with reactive statements forcurrentLetter.You initialize
currentLettertoalphabet[0]here, but lines 33–35 also reassigncurrentLettertoalphabet[0]ifalphabetexists and has a length. This can create confusion or unwanted side effects ifalphabetchanges. Consider consolidating to a single assignment path for clarity.- let currentLetter = alphabet[0]; ... $: if (alphabet && alphabet.length > 0) { - currentLetter = alphabet[0]; + if (!currentLetter) { + currentLetter = alphabet[0]; + } }
18-25:loadingnever reset to false.You set
loading = truewhen loading data for letters other than the first, but never reset it once the data load completes. This leads to a perpetual loading state. Consider resettingloading = falseafter the data is loaded.async function loadReversalData(letter) { if (letter === alphabet[0] && Object.keys(initialData).length > 0) { reversalData = initialData; return; } loading = true; + // Simulate data load + // ... + loading = false; }src/lib/components/LexiconReversalListView.svelte (1)
38-45: Use caution with console.log in production code.Including
console.logfor missing indices is helpful for debugging, but consider replacing it with a more formalized error handling or logging strategy to avoid clutter in production logs.src/lib/components/WordNavigationStrip.svelte (1)
12-30: Potential performance concern with largewordsList.The inline callback within
findIndexhas branching logic that may become expensive for large lists. Consider extracting this logic into a dedicated helper function for readability and performance.$: currentIndex = wordsList.findIndex(word => { - // multiple if-else checks - ... + return matchesCurrentWord(word, currentWord); }); +function matchesCurrentWord(word, current) { + if (typeof word !== 'object') return false; + // replicate the same matching logic + return ... +}src/routes/lexicon/+page.svelte (1)
175-175: Consider awaitingfetchWords()for state consistency.
fetchWords()is called withoutawaitafter resetting language-based states. Depending on the app logic, you might want to ensure new data is loaded before continuing.function switchLanguage(language) { ... - fetchWords(); + await fetchWords(); ... }src/lib/components/LexiconXMLView.svelte (2)
14-37: Optimize repeated database loading.Currently, each call to
queryXmlByWordIdreinitializes the database by fetching and parsingdata.sqlite. This can cause significant performance overhead if the function is invoked frequently (e.g., after each component update). Consider storing a sharedSQL.Databaseinstance at module level or using a lazy-loading approach to prevent repeated loading and parsing of the same file.Example approach:
-let db = null; - -async function queryXmlByWordId(wordId) { - const SQL = await initSqlJs({ - locateFile: (file) => `${base}/wasm/sql-wasm.wasm` - }); - - const response = await fetch(`${base}/data.sqlite`); - const buffer = await response.arrayBuffer(); - db = new SQL.Database(new Uint8Array(buffer)); - ... +let dbPromise; + +function getDatabaseInstance() { + if (!dbPromise) { + dbPromise = (async () => { + const SQL = await initSqlJs({ + locateFile: (file) => `${base}/wasm/sql-wasm.wasm`, + }); + const response = await fetch(`${base}/data.sqlite`); + const buffer = await response.arrayBuffer(); + return new SQL.Database(new Uint8Array(buffer)); + })(); + } + return dbPromise; +} + +async function queryXmlByWordId(wordId) { + const db = await getDatabaseInstance(); ... }
141-165: Avoid attaching new event listeners after each update.The
attachEventListenersfunction runs on every Svelte update, potentially leading to multiple click handlers on the same<span>elements. Each re-render can accumulate listeners, causing memory leaks or duplicated event triggers. Consider removing old listeners, or run this only once after content changes, ensuring you don’t re-bind events multiple times to the same elements.Example approach (prevent reattachment if identical content):
let previousXmlData = ''; afterUpdate(() => { if (xmlData !== previousXmlData) { previousXmlData = xmlData; attachEventListeners(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/index.d.ts(1 hunks)convert/convertConfig.ts(6 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/LexiconXMLView.svelte(1 hunks)src/lib/components/WordNavigationStrip.svelte(1 hunks)src/routes/+page.svelte(1 hunks)src/routes/lexicon/+page.js(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)src/routes/lexicon/+page.ts(0 hunks)src/routes/quiz/[collection]/[id]/+page.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/routes/lexicon/+page.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/routes/lexicon/+page.js (1)
src/routes/quiz/[collection]/[id]/+page.js (2)
load(5-33)response(20-20)
src/routes/quiz/[collection]/[id]/+page.js (1)
src/routes/lexicon/+page.js (2)
load(9-94)response(38-38)
🪛 GitHub Actions: Test
src/lib/components/LexiconReversalListView.svelte
[warning] 15-15: Visible, non-interactive elements with a click event must be accompanied by a keyboard event handler. Consider whether an interactive element such as <button type="button"> or <a> might be more appropriate.
[warning] 15-15: <div> with a click handler must have an ARIA role.
[warning] 32-32: Visible, non-interactive elements with a click event must be accompanied by a keyboard event handler. Consider whether an interactive element such as <button type="button"> or <a> might be more appropriate.
[warning] 32-32: <div> with a click handler must have an ARIA role.
🪛 GitHub Actions: Lint
src/lib/components/LexiconReversalListView.svelte
[warning] 15-15: Visible, non-interactive elements with a click event must be accompanied by a keyboard event handler. Consider whether an interactive element such as '' or '' might be more appropriate.
[warning] 15-15: '
[warning] 32-32: Visible, non-interactive elements with a click event must be accompanied by a keyboard event handler. Consider whether an interactive element such as '
[warning] 32-32: '
🔇 Additional comments (11)
config/index.d.ts (1)
288-294: LGTM: Clean addition ofsingleEntryStylesproperty.The addition of the
singleEntryStylesoptional property to theDictionaryConfigtype is well-structured and follows the existing patterns in the codebase for style configurations.src/lib/components/AlphabetStrip.svelte (1)
1-5: Typed exports look good
The usage of typedexport letvariables in lines 1–5 is straightforward and aligns with TypeScript best practices.src/routes/lexicon/+page.js (2)
1-3: Imports are appropriate
These imports forbase,config, andinitSqlJsare correct and will be used in the load function below.
5-8: SQL.js initialization is straightforward
Specifying thelocateFilecallback for thesql-wasm.wasmfile aligns well with recommended usage.src/routes/quiz/[collection]/[id]/+page.js (1)
5-33: Handle potential empty alphabet scenario
Currently, an empty alphabet array will pass theif (!writingSystem?.alphabet)check but causealphabet[0]to beundefined. Consider verifying that the alphabet has at least one character:if (!writingSystem?.alphabet || !writingSystem.alphabet.length) { throw new Error('Writing system alphabet not found or is empty'); }src/lib/components/LexiconReversalView.svelte (1)
27-31: Check if callbacks are defined before invocation.Ensure
onLetterChangeis a valid callback before calling it to avoid runtime errors if the parent component does not supply the function.function handleLetterSelect(letter) { currentLetter = letter; - onLetterChange(letter); + if (typeof onLetterChange === 'function') { + onLetterChange(letter); + } await loadReversalData(letter); }src/lib/components/WordNavigationStrip.svelte (1)
38-54: Guard the callback.
onSelectWordis called directly. EnsureonSelectWordis a function to avoid runtime issues if it's not provided by the parent component.if (previousWord) { ... - onSelectWord(wordObject); + if (typeof onSelectWord === 'function') { + onSelectWord(wordObject); + } }src/routes/lexicon/+page.svelte (1)
218-218: Infinite scrolling letter tracking caution.When updating
selectedLetterbased on visible elements, you may cause reactive chains that update the DOM and re-trigger scroll events. Double-check that this logic won’t create a loop or unintended resets.src/lib/components/LexiconXMLView.svelte (3)
1-5: No immediate issues in imports and initial setup.Your usage of
$app/pathsandsql.jsappears correct. Overall, these lines look good.
6-13: Well-structured exported props.The exported props are clear and explicitly named. This helps maintain clarity at the component’s interface.
168-168: Duplicate XSS concern from lines 39-119.Using
{@html xmlData}for display retains the same XSS exposure ifxmlDatais ever untrusted or user-provided. Ensure you sanitize or reliably trust the XML data before injecting it here.
| function formatXmlByClass(xmlString) { | ||
| if (!xmlString) return ''; | ||
|
|
||
| const parser = new DOMParser(); | ||
| const xmlDoc = parser.parseFromString(xmlString, 'text/xml'); | ||
|
|
||
| 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 isDefinitionOrGloss = className.includes('definitionorgloss'); | ||
|
|
||
| let parentContainsSenseNumber = | ||
| parentHasSenseNumber || | ||
| [...node.parentNode.children].some( | ||
| (child) => | ||
| child.getAttribute && | ||
| (child.getAttribute('class') || '').includes('sensenumber') | ||
| ); | ||
|
|
||
| const blockClasses = [ | ||
| 'sensenumber', | ||
| 'minimallexreferences', | ||
| 'sharedgrammaticalinfo' | ||
| ]; | ||
|
|
||
| if (blockClasses.some((cls) => className.includes(cls))) { | ||
| output += '\n'; | ||
| } | ||
|
|
||
| if (isDefinitionOrGloss && !parentContainsSenseNumber) { | ||
| output += '\n'; | ||
| } | ||
|
|
||
| if (node.tagName === 'a' && node.hasAttribute('href')) { | ||
| 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 wordObject = vernacularWordsList.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 | ||
|
|
||
| let linkText = node.textContent.trim(); | ||
|
|
||
| // If the text inside the link matches the homonym index, use the homonym index as the text | ||
| if (linkText === String(homonymIndex)) { | ||
| linkText = homonymIndex.toString(); | ||
| } | ||
|
|
||
| output += `<span class="clickable" data-word="${word}" data-index="${index}" data-homonym="${homonymIndex}">${linkText}</span>`; | ||
| return output; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| 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}>`; | ||
| } | ||
| } | ||
|
|
||
| return output; | ||
| } | ||
|
|
||
| return processNode(xmlDoc.documentElement); | ||
| } |
There was a problem hiding this comment.
Potential cross-site scripting (XSS) risk when parsing XML for direct HTML display.
Because untrusted XML content is inserted directly into the DOM via {@html xmlData}, maliciously crafted data could potentially include harmful scripts or tags. If the XML is delivered from a source that might be manipulated, it’s safer to sanitize or whitelist the permitted HTML elements/attributes before injecting into the DOM.
For example, you might integrate a sanitizer library:
-import { afterUpdate, onMount } from 'svelte';
+import DOMPurify from 'dompurify';
...
let xmlData = '';
...
function formatXmlByClass(xmlString) {
...
}
async function updateXmlData() {
let rawXml = await fetchXml(...);
- xmlData = formatXmlByClass(rawXml);
+ xmlData = DOMPurify.sanitize(formatXmlByClass(rawXml));
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@chrisvire Do we want to implement this sanitizer library?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@chrisvire Do we want to implement this sanitizer library?
let's ask Chris today first.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/lib/components/LexiconReversalView.svelte (1)
28-37: 🛠️ Refactor suggestionAdd spacing between language tabs and alphabet strip.
According to the PR comments from chrisvire, there is a need for improved spacing between UI elements. Currently, there's no padding between the language tabs and the letter buttons.
<div class="flex flex-col flex-none"> <LexiconLanguageTabs {reversalLanguage} {selectedLanguage} {onSwitchLanguage} {vernacularLanguage} /> + <div class="mb-2"></div> <!-- Add spacing between tabs and alphabet strip --> <AlphabetStrip {alphabet} activeLetter={currentLetter} onLetterSelect={handleLetterSelect} /> </div>src/routes/lexicon/+page.svelte (3)
73-73:⚠️ Potential issueFix alphabet reference in sort function.
The sort function uses
currentAlphabetreactive variable which could lead to inconsistencies as the reactive value might not be updated yet.- const alphabet = currentAlphabet; + const alphabet = selectedLanguage === reversalLanguage ? alphabets.reversal : alphabets.vernacular;
193-204: 🛠️ Refactor suggestionImprove language switching implementation.
The language switching logic uses inconsistent equality operators (
!=vs===) and directly manipulates the DOM for scrolling. Use Svelte's binding approach instead.function switchLanguage(language) { selectedReversalLanguageStore.set(language); selectedLanguage = 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; + } }
285-289: 🛠️ Refactor suggestionAdd padding to improve UI layout.
According to the PR comments, padding is needed between UI elements to improve the visual layout.
<div - class="flex-1 overflow-y-auto bg-base-100" + class="flex-1 overflow-y-auto bg-base-100 px-2" bind:this={scrollContainer} on:scroll={checkIfScrolledToBottom} >
🧹 Nitpick comments (2)
src/lib/components/LexiconReversalView.svelte (1)
13-25: Add loading state handling.The component tracks currentLetter changes but doesn't provide any visual feedback during these state changes. Given that letter selection likely triggers data loading operations (as seen in the parent component), it would improve user experience to add loading state indicators.
<script> import AlphabetStrip from './AlphabetStrip.svelte'; import LexiconLanguageTabs from './LexiconLanguageTabs.svelte'; export let alphabet = []; export let selectedLanguage; export let vernacularLanguage; export let reversalLanguage; export let selectedLetter; export let onSwitchLanguage; export let onLetterChange; + export let loading = false; let currentLetter = alphabet[0]; async function handleLetterSelect(letter) { currentLetter = letter; onLetterChange(letter); } $: if (alphabet && alphabet.length > 0) { currentLetter = alphabet[0]; } $: if (selectedLetter !== currentLetter) { currentLetter = selectedLetter; } </script>And in the template:
<div class="flex flex-col flex-none"> <LexiconLanguageTabs {reversalLanguage} {selectedLanguage} {onSwitchLanguage} {vernacularLanguage} /> <div class="mb-2"></div> <!-- Add spacing between tabs and alphabet strip --> <AlphabetStrip {alphabet} activeLetter={currentLetter} onLetterSelect={handleLetterSelect} /> + + {#if loading} + <div class="flex justify-center p-4"> + <div class="loading loading-spinner"></div> + </div> + {/if} </div>src/routes/lexicon/+page.svelte (1)
273-283: Connect loading state to LexiconReversalView.The LexiconReversalView component should receive the loading state to display a loading indicator during data fetching.
{#if !selectedWord} <LexiconReversalView alphabet={currentAlphabet} {selectedLanguage} {vernacularLanguage} {reversalLanguage} {selectedLetter} onSwitchLanguage={switchLanguage} onLetterChange={handleLetterChange} + loading={isFetching} /> {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/LexiconReversalView.svelte(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
🔇 Additional comments (1)
src/lib/components/LexiconReversalView.svelte (1)
1-1: Clarify component name.The component is named
LexiconReversalViewbut it appears to be handling both reversal and vernacular view functionality through language tabs. Consider renaming it to something more generic likeLexiconLanguageViewto better reflect its dual purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/routes/lexicon/+page.svelte (6)
73-73: Consistent use of alphabet reference needed.The sort function is using
currentAlphabetwhich is a reactive value and may lead to inconsistencies if it hasn't been updated when this code runs.Instead, directly reference the correct alphabet:
-const alphabet = currentAlphabet; +const alphabet = selectedLanguage === reversalLanguage ? alphabets.reversal : alphabets.vernacular;
85-149: Improve file loading approach to prevent 404 console errors.The current approach of checking for files until one is not found works but generates 404 errors in the console that can confuse users diagnosing issues.
Consider implementing one of these approaches:
- Pre-fetch a metadata file containing available files per letter
- Use a count parameter in your API that provides the number of files per letter
- Implement error silencing for expected 404s
Let's check how prevalent this pattern is in the codebase:
#!/bin/bash # Search for similar file loading patterns to see how they're handled elsewhere rg -A 3 "fetch.*HEAD" --glob "*.{js,ts,svelte}"
197-206: Refine language switching implementation.This implementation contains two issues:
- Inconsistent equality operators (
!=vs===)- Direct DOM manipulation instead of using Svelte bindings
Apply these improvements:
function switchLanguage(language) { selectedReversalLanguageStore.set(language); selectedLanguage = 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; + } }
277-287: Implement DaisyUI tabs for language selection per design feedback.According to PR feedback from chrisvire, the language selector should use DaisyUI tabs rather than buttons, with proper spacing between UI elements.
The
LexiconReversalViewcomponent should be updated to use DaisyUI tabs with the "dy-" prefix:#!/bin/bash # Check for existing DaisyUI tab implementations in the codebase rg -A 3 "dy-tabs" --glob "*.{svelte,html}"
289-293: Add padding between UI elements per design feedback.According to PR feedback, padding is needed between UI elements to improve the visual layout.
-<div - class="flex-1 overflow-y-auto bg-base-100" +<div + class="flex-1 overflow-y-auto bg-base-100 px-2"
211-250: 🛠️ Refactor suggestionImplement debouncing for scroll event handler.
The
checkIfScrolledToBottomfunction runs on every scroll event which can impact performance. Implementing debouncing would make this more efficient.Import debounce from lodash-es (or implement your own) and apply it to the scroll handler:
import { onMount, tick } from 'svelte'; +import { debounce } from 'lodash-es'; // Then replace the existing implementation with: +const debouncedScrollCheck = debounce(async (event) => { + // Move the entire checkIfScrolledToBottom implementation here +}, 200); +function checkIfScrolledToBottom(event) { + debouncedScrollCheck(event); +}
🧹 Nitpick comments (1)
src/routes/lexicon/+page.svelte (1)
256-256: Use consistent equality operators.There's an inconsistent use of equality operators across the codebase.
-if (selectedLetter && selectedLanguage != vernacularLanguage) { +if (selectedLetter && selectedLanguage !== vernacularLanguage) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/LexiconReversalView.svelte(1 hunks)src/lib/components/LexiconXMLView.svelte(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/components/LexiconReversalView.svelte
- src/lib/components/LexiconXMLView.svelte
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
convert/convertReverseIndex.ts (1)
61-65: Consider documenting case sensitivity differences.The code uses different case conventions -
entriesByLetteruses the original letter case whilefileIndexByLetteruses lowercase keys. Though valid for filename consistency, this could be clearer with documentation.const entryLetter = firstLetter ?? latestLetter; +// Use lowercase for filenames to ensure consistency across operating systems const fileEntryLetter = entryLetter.toLowerCase(); if (!entriesByLetter[entryLetter]) { entriesByLetter[entryLetter] = []; + // Initialize file index array using lowercase letter for filesystem consistency fileIndexByLetter[fileEntryLetter] = []; // Initialize file index for the letter }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
convert/convertReverseIndex.ts(4 hunks)src/lib/lexicon/index.ts(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)src/routes/lexicon/+page.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib/lexicon/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/lexicon/+page.ts
- src/routes/lexicon/+page.svelte
🔇 Additional comments (5)
convert/convertReverseIndex.ts (5)
52-52: Good addition for tracking chunk files by letter.Creating a file index by letter will facilitate efficient retrieval of reversal word chunks in the UI, enabling better performance for lexicon browsing.
71-72: Appropriate caching of lowercase letter.Good practice to cache the lowercase letter for filename generation, which avoids redundant transformations and ensures consistency.
106-106: Using consistent filename conventions.Using lowercase in filenames and a structured numbering format ensures filesystem compatibility across different operating systems.
114-115: Proper tracking of chunk files in the index.Correctly tracking each chunk file in the letter-based index ensures the client can efficiently locate and load relevant data chunks.
124-128: Good addition of the index manifest file.Creating an index.json manifest allows the client application to efficiently determine which chunk files contain data for a specific letter, supporting the incremental loading UI pattern mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/routes/lexicon/+page.svelte (5)
74-74: Usealphabets.reversalinstead ofcurrentAlphabetin sort function.The
currentAlphabetreactive variable is used within the sort function, which can lead to inconsistencies if the reactive value hasn't been updated yet.-const alphabet = currentAlphabet; +const alphabet = selectedLanguage === reversalLanguage ? alphabets.reversal : alphabets.vernacular;
182-191: Improve language switching implementation.The language switching logic uses loose equality (
!=) in one conditional but strict equality (===) elsewhere, and there's direct DOM manipulation for scrolling which should use Svelte bindings instead.function switchLanguage(language) { selectedReversalLanguageStore.set(language); selectedLanguage = 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; + } }
196-235: Debounce scroll event handler for better performance.The
checkIfScrolledToBottomfunction runs on every scroll event, which can impact performance. Implementing debouncing would make it more efficient.+ import { debounce } from 'lodash-es'; // ... - async function checkIfScrolledToBottom(event) { - if (isFetching) return; - - // Rest of the function - } + const debouncedScrollCheck = debounce(async (event) => { + if (isFetching) return; + + // Move the entire function body here + }, 200); + + function checkIfScrolledToBottom(event) { + debouncedScrollCheck(event); + }If lodash is not available, implement a simple debounce function:
function debounce(func, wait) { let timeout; return function(...args) { clearTimeout(timeout); timeout = setTimeout(() => func.apply(this, args), wait); }; }
274-278: Add padding between UI elements per design feedback.According to the PR comments from chrisvire, padding is needed between UI elements to improve the visual layout.
<div - class="flex-1 overflow-y-auto bg-base-100" + class="flex-1 overflow-y-auto bg-base-100 px-2" bind:this={scrollContainer} on:scroll={checkIfScrolledToBottom} >
263-272: Implement DaisyUI tabs for language selection following design feedback.According to the PR comments, the language selector should use DaisyUI tabs rather than buttons. Please ensure the
LexiconReversalViewcomponent implements this recommendation.<LexiconReversalView alphabet={currentAlphabet} {selectedLanguage} {vernacularLanguage} {reversalLanguage} {selectedLetter} onSwitchLanguage={switchLanguage} onLetterChange={handleLetterChange} + class="mb-2" <!-- Add margin bottom to create spacing --> />The
LexiconLanguageTabscomponent insideLexiconReversalViewshould use DaisyUI tabs with thedy-prefix as mentioned in the PR comments.
🧹 Nitpick comments (1)
src/routes/lexicon/+page.svelte (1)
110-116: Improve error logging message.The current error message doesn't provide enough context about which word is being processed, making debugging more difficult.
-console.log(`Index ${entry.index} not found in vernacularWordsList`); +console.warn(`Index ${entry.index} for word "${word}" not found in vernacularWordsList`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/AlphabetStrip.svelte(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/AlphabetStrip.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)
240-247: LGTM on onMount logic.The onMount implementation correctly initializes data fetching and handles redirection when the program type isn't 'DAB'.
45-51: LGTM on store subscriptions.Clean implementation of store subscriptions with appropriate default values and fallbacks.
|
|
||
| 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); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in loadLetterData.
The current implementation only processes responses with status 200 (ok) but doesn't handle errors or log failures when fetching reversal data files.
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();
// Rest of the processing remains the same
} else {
+ console.warn(`Failed to load reversal file ${file} with status: ${response.status}`);
}
+ } catch (error) {
+ console.error(`Error fetching reversal file ${file}:`, error.message);
+ }
}
// Rest of the function remains the same
}📝 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 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); | |
| } | |
| }); | |
| } | |
| } | |
| 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` | |
| ); | |
| 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.warn(`Failed to load reversal file ${file} with status: ${response.status}`); | |
| } | |
| } catch (error) { | |
| console.error(`Error fetching reversal file ${file}:`, error.message); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/components/LexiconListViewHeader.svelte (2)
1-26: Component structure and logic look good, with minor reactivity concerns.The overall component structure is well organized with clear prop definitions and logical flow. The component properly integrates the language tabs and alphabet strip. However, there is a potential reactivity issue with the current letter selection logic.
The reactive statements on lines 20-25 might cause unexpected behavior. When the alphabet changes,
currentLetteris always reset to the first letter, which could override user selection. Consider modifying this logic to only initializecurrentLetterwhen it's undefined or not in the new alphabet:-$: if (alphabet && alphabet.length > 0) { - currentLetter = alphabet[0]; -} +$: if (alphabet && alphabet.length > 0 && (!currentLetter || !alphabet.includes(currentLetter))) { + currentLetter = alphabet[0]; +}
5-11: Add TypeScript type definitions for props.The component lacks TypeScript type definitions for its props, which would improve code maintainability and provide better IDE support.
Consider adding TypeScript types to your props:
<script lang="ts"> import LexiconAlphabetStrip from './LexiconAlphabetStrip.svelte'; import LexiconLanguageTabs from './LexiconLanguageTabs.svelte'; - export let alphabet = []; + export let alphabet: string[] = []; - export let selectedLanguage; + export let selectedLanguage: string; - export let vernacularLanguage; + export let vernacularLanguage: string; - export let reversalLanguage; + export let reversalLanguage: string; - export let selectedLetter; + export let selectedLetter: string; - export let onSwitchLanguage; + export let onSwitchLanguage: (language: string) => void; - export let onLetterChange; + export let onLetterChange: (letter: string) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/LexiconAlphabetStrip.svelte(1 hunks)src/lib/components/LexiconListViewHeader.svelte(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/LexiconAlphabetStrip.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/lexicon/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (1)
src/lib/components/LexiconListViewHeader.svelte (1)
28-33: Consider implementing language selection as tabs per PR feedback.According to the PR comments, it was recommended that "the language selector should be implemented as tabs rather than buttons" and specifically mentions using Daisy UI tab components with the "dy-" prefix.
Please verify that the
LexiconLanguageTabscomponent properly implements this recommendation using Daisy UI tabs with the "dy-" prefix. You might want to check the implementation of this component to ensure it meets the requirements mentioned in the PR feedback.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/routes/lexicon/+page.svelte (2)
181-191:⚠️ Potential issueUse strict comparison and the existing
scrollContainerbindingA previous review already highlighted this but the code still uses loose comparison and manual DOM query:
-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; +}
86-134: 🛠️ Refactor suggestion
loadLetterDatalacks error handling and can break the whole fetch chainIf any
fetchinside the loop throws (network issue, JSON parse error) the function bubbles the exception and none of the awaitedPromise.allcalls resolve, leaving the UI stuck.Wrap the per‑file logic in
try/catchand continue processing the remaining files:for (const file of files) { + try { const reversalFile = `${base}/reversal/${defaultReversalKey}/${file}`; const response = await fetch(reversalFile); if (response.ok) { const data = await response.json(); … } + } catch (e) { + console.warn(`Failed to load ${file}:`, e); + } }
🧹 Nitpick comments (1)
src/routes/lexicon/+page.svelte (1)
56-57: Remove debugconsole.logor gate it behind dev flag
console.log('Loading letter data:', letter);will clutter production logs.
Consider removing or wrapping withif (import.meta.env.DEV).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/LexiconEntryView.svelte(1 hunks)src/routes/lexicon/+page.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (1)
src/routes/lexicon/+page.svelte (1)
70-78:currentAlphabetreference inside sort can be stale
currentAlphabetis a reactive declaration outside the function; if the user switches languages between the asyncfetchWordscall and the sort, you might sort with the old alphabet.Capture the alphabet before sorting:
const alphabet = selectedLanguage === reversalLanguage ? alphabets.reversal : alphabets.vernacular; updatedWords[selectedLanguage] = (updatedWords[selectedLanguage] || []).sort( (a, b) => alphabet.indexOf(a.word[0].toLowerCase()) - alphabet.indexOf(b.word[0].toLowerCase()) );
| afterUpdate(() => { | ||
| updateXmlData(); | ||
| applyStyles(); | ||
| attachEventListeners(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
afterUpdate triggers an expensive DB fetch every render
afterUpdate runs on every component update; you call updateXmlData() (async) each time, causing needless DB queries and re‑render loops.
Prefer a reactive statement that depends only on selectedWord / vernacularWordsList:
$: if (selectedWord) {
await updateXmlData();
await tick(); // wait for DOM update
applyStyles();
attachEventListeners();
}This fires exactly when required and avoids thrashing.
| function formatXmlByClass(xmlString) { | ||
| if (!xmlString) return ''; | ||
|
|
||
| const parser = new DOMParser(); | ||
| const xmlDoc = parser.parseFromString(xmlString, 'text/xml'); | ||
|
|
||
| // Check if parsing failed | ||
| const parseError = xmlDoc.querySelector('parsererror'); | ||
| if (parseError) { | ||
| console.error('XML parsing error:', parseError.textContent); | ||
| return `<span class="text-error">Error parsing XML: Invalid format</span>`; | ||
| } | ||
|
|
||
| function processNode(node, parentHasSenseNumber = false) { |
There was a problem hiding this comment.
Potential XSS – XML → HTML is injected without sanitisation
formatXmlByClass blindly reconstructs HTML from the parsed XML and injects it with {@html}.
If the XML contains <script> or dangerous attributes (on*, javascript: URLs, etc.) they will be executed.
Please run the output through an HTML sanitizer (e.g. DOMPurify) or explicitly strip/escape unsafe tags and attributes before exposing them to the DOM.
| for (let stl of singleEntryStyles) { | ||
| for (let elm of document.querySelectorAll(stl.name)) { | ||
| elm.style = convertStyle(stl.properties); | ||
| } | ||
| } |
There was a problem hiding this comment.
Inline‑style assignment is wrong – use style.cssText or setAttribute
elm.style is a CSSStyleDeclaration, not a string. Assigning a string to it is a no‑op in most browsers.
-elm.style = convertStyle(stl.properties);
+elm.style.cssText = convertStyle(stl.properties); // or elm.setAttribute('style', …)Without this fix none of the configured styles are actually applied.
📝 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.
| for (let stl of singleEntryStyles) { | |
| for (let elm of document.querySelectorAll(stl.name)) { | |
| elm.style = convertStyle(stl.properties); | |
| } | |
| } | |
| for (let stl of singleEntryStyles) { | |
| for (let elm of document.querySelectorAll(stl.name)) { | |
| - elm.style = convertStyle(stl.properties); | |
| + elm.style.cssText = convertStyle(stl.properties); // or elm.setAttribute('style', …) | |
| } | |
| } |
| async function updateXmlData() { | ||
| if ( | ||
| !selectedWord || | ||
| (!selectedWord.index && (!selectedWord.indexes || selectedWord.indexes.length === 0)) | ||
| ) { | ||
| xmlData = ''; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
selectedWord.index may legally be 0 – current check treats it as “missing”
!selectedWord.index is falsy for 0, so an entry whose index is zero will never be shown.
Use an explicit null/undefined test instead:
-if (
- !selectedWord ||
- (!selectedWord.index && (!selectedWord.indexes || selectedWord.indexes.length === 0))
-)
+if (
+ !selectedWord ||
+ (selectedWord.index == null && (!selectedWord.indexes || selectedWord.indexes.length === 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.
| async function updateXmlData() { | |
| if ( | |
| !selectedWord || | |
| (!selectedWord.index && (!selectedWord.indexes || selectedWord.indexes.length === 0)) | |
| ) { | |
| xmlData = ''; | |
| return; | |
| } | |
| async function updateXmlData() { | |
| if ( | |
| !selectedWord || | |
| (selectedWord.index == null && (!selectedWord.indexes || selectedWord.indexes.length === 0)) | |
| ) { | |
| xmlData = ''; | |
| return; | |
| } |
| async function queryXmlByWordId(wordId) { | ||
| try { | ||
| const SQL = await initSqlJs({ | ||
| locateFile: (file) => `${base}/wasm/sql-wasm.wasm` | ||
| }); | ||
|
|
||
| const response = await fetch(`${base}/data.sqlite`); | ||
| if (!response.ok) { | ||
| throw new Error( | ||
| `Failed to fetch database: ${response.status} ${response.statusText}` | ||
| ); | ||
| } | ||
| const buffer = await response.arrayBuffer(); | ||
| const db = new SQL.Database(new Uint8Array(buffer)); | ||
| if (!db) { | ||
| console.error('Database not initialized'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid re‑initialising sql.js and re‑downloading the DB on every word lookup
initSqlJs and the fetch for data.sqlite are executed inside queryXmlByWordId, which means a full WASM initialisation and DB download happens for each word ID (and for every component update).
This is extremely expensive (multi‑MB download + WASM compile) and becomes noticeable as soon as a user clicks through several entries.
Create a module‑level (or store‑level) singleton that initialises SQL and keeps a Database instance open for the life‑time of the session, then reuse it:
-let SQL;
-let db;
+let SQL; // undefined until first call
+let db; // cache DB instance
async function queryXmlByWordId(wordId) {
try {
- const SQL = await initSqlJs({ locateFile: … });
- const response = await fetch(`${base}/data.sqlite`);
- …
- const db = new SQL.Database(new Uint8Array(buffer));
+ if (!SQL) {
+ SQL = await initSqlJs({ locateFile: (f) => `${base}/wasm/${f}` });
+ const response = await fetch(`${base}/data.sqlite`);
+ const buffer = await response.arrayBuffer();
+ db = new SQL.Database(new Uint8Array(buffer));
+ }This reduces network traffic, CPU time, and memory dramatically.
Committable suggestion skipped: line range outside the PR's diff.
| 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; |
There was a problem hiding this comment.
Guard against undefined word lists before accessing .length
reversalWordsList and vernacularWordsList are populated asynchronously via store subscriptions, so they are undefined during the first few renders.
Calling .length on undefined throws.
- (selectedLanguage === reversalLanguage && reversalWordsList.length > 0) ||
- (selectedLanguage === vernacularLanguage && vernacularWordsList.length > 0)
+ (selectedLanguage === reversalLanguage && (reversalWordsList?.length ?? 0) > 0) ||
+ (selectedLanguage === vernacularLanguage && (vernacularWordsList?.length ?? 0) > 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.
| 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; | |
| async function checkIfScrolledToBottom(event) { | |
| if (isFetching) return; | |
| if ( | |
| (selectedLanguage === reversalLanguage && (reversalWordsList?.length ?? 0) > 0) || | |
| (selectedLanguage === vernacularLanguage && (vernacularWordsList?.length ?? 0) > 0) | |
| ) { | |
| let div = event.target; | |
| const threshold = 100; | |
| // ... | |
| } | |
| // rest of function... | |
| } |
5 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores