Polish up extension with the style of the PB dictionary #2004
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR replaces legacy top-level localization keys with a modular set and adds localization infrastructure; introduces multiple new React UI components, utilities, and a hook for dictionary browsing and entry workflows; updates types to expose language options; removes obsolete menu/commands; and refactors web view and project-manager webview open/reload logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
platform.bible-extension/src/utils/fw-lite-api.ts (2)
54-58: Sanitize path segment: potential path injection via idid is interpolated directly into the URL path. Encode or validate it to prevent malformed requests.
- const path = `mini-lcm/${type}/${code}/entry/${id}`; + const path = `mini-lcm/${type}/${code}/entry/${sanitizeUrlComponent(id)}`;
70-83: Encode search query when constructing pathsearch is appended as a path segment without encoding. This will break on spaces/slashes and can route to unintended endpoints.
- if (search) path += `/${search}`; + if (search) path += `/${encodeURIComponent(search)}`;platform.bible-extension/src/types/fw-lite-extension.d.ts (1)
20-22: PartialEntry is not exported but used externallyplatform.bible-extension/src/utils/fw-lite-api.ts imports PartialEntry from 'fw-lite-extension'. Without export, TS consumers cannot import it.
- type PartialEntry = Omit<Partial<IEntry>, 'senses'> & { + export type PartialEntry = Omit<Partial<IEntry>, 'senses'> & { senses?: Partial<ISense>[]; };platform.bible-extension/src/web-views/add-word.web-view.tsx (1)
50-61: Ensure submission state resets on errors (wrap in try/catch/finally).If
addEntrythrows,isSubmittingstays true and the success path isn’t logged. Add try/catch/finally and log the error.setIsSubmitted(false); setIsSubmitting(true); - logger.info(`Adding entry: ${JSON.stringify(entry)}`); - const entryId = (await fwLiteNetworkObject.addEntry(projectId, entry))?.id; - setIsSubmitting(false); - if (entryId) { - setIsSubmitted(true); - await papi.commands.sendCommand('fwLiteExtension.displayEntry', projectId, entryId); - } else { - logger.error(`${localizedStrings['%fwLiteExtension_error_failedToAddEntry%']}`); - } + logger.info(`Adding entry: ${JSON.stringify(entry)}`); + try { + const entryId = (await fwLiteNetworkObject.addEntry(projectId, entry))?.id; + if (entryId) { + setIsSubmitted(true); + await papi.commands.sendCommand('fwLiteExtension.displayEntry', projectId, entryId); + } else { + logger.error(localizedStrings['%fwLiteExtension_error_failedToAddEntry%']); + } + } catch (e) { + logger.error(localizedStrings['%fwLiteExtension_error_failedToAddEntry%'], e); + } finally { + setIsSubmitting(false); + }platform.bible-extension/src/web-views/find-word.web-view.tsx (2)
55-66: Prevent fetch spinner from sticking on thrown errors.Wrap
getEntriesin try/catch/finally soisFetchingalways resets and errors are logged.- logger.info(`Fetching entries for ${surfaceForm}`); - setIsFetching(true); - const entries = await fwLiteNetworkObject.getEntries(projectId, { surfaceForm }); - setIsFetching(false); - setMatchingEntries(entries ?? []); + logger.info(`Fetching entries for ${surfaceForm}`); + setIsFetching(true); + try { + const entries = await fwLiteNetworkObject.getEntries(projectId, { surfaceForm }); + setMatchingEntries(entries ?? []); + } catch (e) { + logger.error('Error fetching entries', e); + } finally { + setIsFetching(false); + }
80-99: Harden add-entry flow and avoid relying on object property order.Use try/catch and derive the search term from preferred language, not
Object.values(...).pop().logger.info(`Adding entry: ${JSON.stringify(entry)}`); - const addedEntry = await fwLiteNetworkObject.addEntry(projectId, entry); - if (addedEntry) { - onSearch(Object.values<string | undefined>(addedEntry.lexemeForm).pop() ?? ''); - await papi.commands.sendCommand('fwLiteExtension.displayEntry', projectId, addedEntry.id); - } else { - logger.error(`${localizedStrings['%fwLiteExtension_error_failedToAddEntry%']}`); - } + try { + const addedEntry = await fwLiteNetworkObject.addEntry(projectId, entry); + if (addedEntry) { + const newSearch = + addedEntry.lexemeForm?.[vernacularLanguage ?? ''] ?? + addedEntry.lexemeForm?.[analysisLanguage ?? ''] ?? + ''; + onSearch(newSearch); + await papi.commands.sendCommand('fwLiteExtension.displayEntry', projectId, addedEntry.id); + } else { + logger.error(localizedStrings['%fwLiteExtension_error_failedToAddEntry%']); + } + } catch (e) { + logger.error(localizedStrings['%fwLiteExtension_error_failedToAddEntry%'], e); + }platform.bible-extension/src/web-views/find-related-words.web-view.tsx (2)
78-87: Always clear loading and handle fetch errorsIf
getEntriesrejects,isFetchingstays true and UI wedges.- logger.info(`Fetching entries for ${surfaceForm}`); - setIsFetching(true); - let entries = (await fwLiteNetworkObject.getEntries(projectId, { surfaceForm })) ?? []; - // Only consider entries and senses with at least one semantic domain. - entries = entries - .map((e) => ({ ...e, senses: e.senses.filter((s) => s.semanticDomains.length) })) - .filter((e) => e.senses.length); - setIsFetching(false); - setMatchingEntries(entries); + logger.info(`Fetching entries for ${surfaceForm}`); + setIsFetching(true); + try { + let entries = (await fwLiteNetworkObject.getEntries(projectId, { surfaceForm })) ?? []; + // Only consider entries and senses with at least one semantic domain. + entries = entries + .map((e) => ({ ...e, senses: e.senses.filter((s) => s.semanticDomains.length) })) + .filter((e) => e.senses.length); + setMatchingEntries(entries); + } catch (e) { + logger.error('Error fetching entries:', e); + } finally { + setIsFetching(false); + }
100-105: Mirror error/cleanup handling for related entriesSame loading-stuck issue if this request fails.
- logger.info(`Fetching entries in semantic domain ${semanticDomain}`); - setIsFetching(true); - const entries = await fwLiteNetworkObject.getEntries(projectId, { semanticDomain }); - setIsFetching(false); - setRelatedEntries(entries ?? []); + logger.info(`Fetching entries in semantic domain ${semanticDomain}`); + setIsFetching(true); + try { + const entries = await fwLiteNetworkObject.getEntries(projectId, { semanticDomain }); + setRelatedEntries(entries ?? []); + } catch (e) { + logger.error('Error fetching related entries:', e); + } finally { + setIsFetching(false); + }
🧹 Nitpick comments (46)
platform.bible-extension/src/utils/project-manager.ts (3)
80-84: Type the options parameter to exclude projectId since it’s injected internally.
This prevents callers from needing to (incorrectly) supply projectId and matches the implementation.Apply:
- async openWebView( + async openWebView( webViewType: WebViewType, layout?: Layout, - options?: OpenWebViewOptionsWithProjectId, + options?: Omit<OpenWebViewOptionsWithProjectId, 'projectId'>, ): Promise<boolean> {I can update any call sites/types if needed.
103-103: Prefer nullish assignment to avoid re-fetch on falsy values.
Use ??= to guard only undefined/null, not other falsy values.Apply:
- this.dataProvider ||= await projectDataProviders.get('platform.base', this.projectId); + this.dataProvider ??= await projectDataProviders.get('platform.base', this.projectId);
107-115: Logging the wrong setting key.
Both logs hardcode ProjectLanguageTag instead of using the passed setting, which makes logs misleading.Apply:
- logger.info(`Getting '${ProjectSettingKey.ProjectLanguageTag}'`); + logger.info(`Getting '${setting}'`); @@ - logger.info(`Setting '${ProjectSettingKey.ProjectLanguageTag}' to '${value}'`); + logger.info(`Setting '${setting}' to '${value}'`);platform.bible-extension/src/utils/fw-lite-api.ts (3)
93-104: Prefer partial failure over fail-open for language filteringReturning all projects on any single failure can surface irrelevant dictionaries. Use Promise.allSettled to keep successful matches while ignoring failed ones.
- try { - const matches = ( - await Promise.all( - projects.map(async (p) => - (await this.doesProjectMatchLangTag(p.code, langTag)) ? p : undefined, - ), - ) - ).filter((p) => p) as IProjectModel[]; - return matches.length ? matches : projects; - } catch { - return projects; - } + const results = await Promise.allSettled( + projects.map(async (p) => + (await this.doesProjectMatchLangTag(p.code, langTag)) ? p : undefined, + ), + ); + const matches = results + .filter((r): r is PromiseFulfilledResult<IProjectModel | undefined> => r.status === 'fulfilled') + .map((r) => r.value) + .filter((p): p is IProjectModel => !!p); + return matches.length ? matches : projects;
60-66: Locale-agnostic lowercasing and defensive accesstoLocaleLowerCase can behave unexpectedly in certain locales (e.g., Turkish). Also guard against missing vernacular.
- const tag = langTag.trim().toLocaleLowerCase().split('-')[0]; + const tag = langTag.trim().toLowerCase().split('-')[0]; if (!code || !tag) return false; const writingSystems = await this.getWritingSystems(code); - const vernLangTags = Object.keys(writingSystems.vernacular).map((v) => v.toLocaleLowerCase()); + const vernLangTags = Object.keys(writingSystems.vernacular ?? {}).map((v) => v.toLowerCase());
24-34: Safer logging of RequestInitJSON.stringify(init) may throw or omit useful info (e.g., Error, Headers). Log selective fields or fall back to String().
- if (init) { - logger.info(JSON.stringify(init)); - } + if (init) { + const { method, headers } = init; + logger.info(`method=${method ?? 'GET'}, headers=${headers ? JSON.stringify(headers) : '{}'}`); + }platform.bible-extension/src/main.ts (3)
42-42: Typo in function namelaunchFwLiteFwLiteWeb has “FwLite” duplicated. Consider renaming to launchFwLiteWeb for clarity.
- const { baseUrl, fwLiteProcess } = launchFwLiteFwLiteWeb(context); + const { baseUrl, fwLiteProcess } = launchFwLiteWeb(context);(Apply corresponding rename at definition.)
179-190: Improve error logging: JSON.stringify(Error) yields {}Use Error.message (and stack when available) to avoid swallowing details.
- const langs = await fwLiteApi - .getWritingSystems(dictionaryCode) - .catch((e) => logger.error('Error fetching writing systems:', JSON.stringify(e))); + const langs = await fwLiteApi + .getWritingSystems(dictionaryCode) + .catch((e) => + logger.error('Error fetching writing systems:', e instanceof Error ? e.message : String(e)), + ); @@ - await projectManager - .setFwAnalysisLanguage(analysisLang) - .catch((e) => logger.error('Error setting analysis language:', JSON.stringify(e))); + await projectManager + .setFwAnalysisLanguage(analysisLang) + .catch((e) => + logger.error('Error setting analysis language:', e instanceof Error ? e.message : String(e)), + );
118-133: Guard URL building against invalid entryIdgetBrowseUrl can throw for invalid entryId. Consider try/catch to log and return { success: false } rather than bubbling.
- const url = getBrowseUrl(baseUrl, dictionaryCode, entryId); - const options: BrowseWebViewOptions = { url }; - success = await projectManager.openWebView(WebViewType.Main, undefined, options); + try { + const url = getBrowseUrl(baseUrl, dictionaryCode, entryId); + const options: BrowseWebViewOptions = { url }; + success = await projectManager.openWebView(WebViewType.Main, undefined, options); + } catch (e) { + logger.error('Invalid entryId for displayEntry:', e instanceof Error ? e.message : String(e)); + }platform.bible-extension/contributions/localizedStrings.json (1)
29-31: Tighten phrasing for related words instructionsMinor copy tweak for brevity.
- "%fwLiteExtension_findRelatedWord_selectInstruction%": "Select a semantic domain for related words in that domain", + "%fwLiteExtension_findRelatedWord_selectInstruction%": "Select a semantic domain to find related words",platform.bible-extension/src/web-views/index.tsx (1)
20-119: Reduce duplication across providers (optional)The five providers largely differ by WebViewType, content, and title. Consider a small factory/helper to DRY this.
Example (outside diff for brevity):
createProvider = (type, content, title, styles) => ({ getWebView(...) { /* common checks + return */ }});platform.bible-extension/src/types/localized-string-keys.ts (1)
3-32: Use a const tuple withsatisfiesfor compile-time key validation (catch typos) and stronger typesThis prevents widening to
string[], keeps literal types, and validates each key againstLocalizeKey.Apply this diff:
-export const LOCALIZED_STRING_KEYS: LocalizeKey[] = [ +export const LOCALIZED_STRING_KEYS = [ '%fwLiteExtension_addWord_buttonAdd%', '%fwLiteExtension_addWord_buttonSubmit%', '%fwLiteExtension_addWord_title%', '%fwLiteExtension_button_cancel%', '%fwLiteExtension_dictionary_backToList%', '%fwLiteExtension_dictionary_loading%', '%fwLiteExtension_dictionary_noResults%', '%fwLiteExtension_dictionarySelect_clear%', '%fwLiteExtension_dictionarySelect_confirm%', '%fwLiteExtension_dictionarySelect_loading%', '%fwLiteExtension_dictionarySelect_noneFound%', '%fwLiteExtension_dictionarySelect_saved%', '%fwLiteExtension_dictionarySelect_saveError%', '%fwLiteExtension_dictionarySelect_saving%', '%fwLiteExtension_dictionarySelect_select%', '%fwLiteExtension_dictionarySelect_selected%', '%fwLiteExtension_entryDisplay_definition%', '%fwLiteExtension_entryDisplay_gloss%', '%fwLiteExtension_entryDisplay_headword%', '%fwLiteExtension_entryDisplay_partOfSpeech%', '%fwLiteExtension_entryDisplay_senses%', '%fwLiteExtension_error_failedToAddEntry%', '%fwLiteExtension_error_gettingNetworkObject%', '%fwLiteExtension_error_missingParam%', '%fwLiteExtension_findRelatedWord_noResultsInDomain%', '%fwLiteExtension_findRelatedWord_selectInstruction%', '%fwLiteExtension_findRelatedWord_textField%', '%fwLiteExtension_findWord_textField%', -]; +] as const satisfies readonly LocalizeKey[]; + +export type LocalizedStringKey = (typeof LOCALIZED_STRING_KEYS)[number];platform.bible-extension/src/web-views/main.web-view.tsx (1)
10-10: Localize the loading state and make the iframe fill the containerAlso consider security attributes if
urlcan be external.Apply this diff:
-import { useRef } from 'react'; +import { useRef } from 'react'; +import { useLocalizedStrings } from '@papi/frontend/react'; +import { LOCALIZED_STRING_KEYS } from '../types/localized-string-keys'; @@ const iframe = useRef<HTMLIFrameElement | null>(null); - return url ? <iframe ref={iframe} src={url} title="FieldWorks Lite" /> : <p>Loading...</p>; + const [ls] = useLocalizedStrings(LOCALIZED_STRING_KEYS); + return url ? ( + <iframe + ref={iframe} + src={url} + title="FieldWorks Lite" + className="tw-w-full tw-h-full tw-border-0" + loading="lazy" + /* Consider sandbox/referrerPolicy if remote: sandbox="allow-scripts allow-same-origin" referrerPolicy="no-referrer" */ + /> + ) : ( + <p>{ls['%fwLiteExtension_dictionary_loading%']}</p> + );platform.bible-extension/src/utils/use-is-wide-screen.tsx (1)
5-18: SSR-safe init and Safari fallback for media query listenersPrevents hydration issues when
windowis undefined and supports older Safari (addListener).Apply this diff:
-export default function useIsWideScreen() { - const [isWide, setIsWide] = useState(() => window.innerWidth >= 1024); +export default function useIsWideScreen() { + const [isWide, setIsWide] = useState(() => (typeof window !== 'undefined' ? window.innerWidth >= 1024 : false)); @@ - const mediaQuery = window.matchMedia('(min-width: 1024px)'); - - const handler = (e: MediaQueryListEvent) => setIsWide(e.matches); - mediaQuery.addEventListener('change', handler); + if (typeof window === 'undefined') return; + const mediaQuery = window.matchMedia('(min-width: 1024px)'); + const handler = (e: MediaQueryListEvent) => setIsWide(e.matches); + if (typeof mediaQuery.addEventListener === 'function') { + mediaQuery.addEventListener('change', handler); + } else { + // Safari fallback + // @ts-expect-error addListener exists in older Safari + mediaQuery.addListener(handler); + } @@ - setIsWide(mediaQuery.matches); + setIsWide(mediaQuery.matches); @@ - return () => mediaQuery.removeEventListener('change', handler); + return () => { + if (typeof mediaQuery.removeEventListener === 'function') { + mediaQuery.removeEventListener('change', handler); + } else { + // @ts-expect-error removeListener exists in older Safari + mediaQuery.removeListener(handler); + } + };platform.bible-extension/src/components/domains-display.tsx (3)
5-6: Drop unused React imports after refactorIf adopting the memo-only approach below, remove
useEffect/useState.
28-35: Replace state+effect withuseMemo; avoid non-null assertion and handle duplicates deterministicallyThis simplifies logic, avoids extra render, and removes the
!assertion.Apply this diff:
- const [sortedDomains, setSortedDomains] = useState<ISemanticDomain[]>([]); - - useEffect(() => { - const codes = [...new Set(domains.map((domain) => domain.code))].sort(); - // eslint-disable-next-line no-type-assertion/no-type-assertion - setSortedDomains(codes.map((code) => domains.find((domain) => domain.code === code)!)); - }, [domains]); + const sortedDomains = useMemo(() => { + const firstByCode = new Map<string, ISemanticDomain>(); + for (const d of domains) if (!firstByCode.has(d.code)) firstByCode.set(d.code, d); + return Array.from(firstByCode.values()).sort((a, b) => a.code.localeCompare(b.code)); + }, [domains]);
39-48: Only render a button when clickable; otherwise render a non-interactive pillImproves semantics and accessibility (no disabled interactive control when not needed).
Apply this diff:
- <button - className="tw-rounded tw-bg-accent tw-px-2 tw-py-0.5 tw-text-xs tw-accent-foreground tw-flex tw-items-center tw-gap-1" - disabled={!onClickDomain} - key={domain.code} - onClick={() => onClickDomain?.(domain)} - type="button" - > + {onClickDomain ? ( + <button + className="tw-rounded tw-bg-accent tw-px-2 tw-py-0.5 tw-text-xs tw-accent-foreground tw-flex tw-items-center tw-gap-1" + key={domain.code} + onClick={() => onClickDomain(domain)} + type="button" + > + <Network className="tw-inline tw-mr-1 tw-h-3 tw-w-3" /> + <span>{domainText(domain, analysisLanguage)}</span> + </button> + ) : ( + <span + className="tw-rounded tw-bg-accent tw-px-2 tw-py-0.5 tw-text-xs tw-accent-foreground tw-inline-flex tw-items-center tw-gap-1" + key={domain.code} + aria-label={domainText(domain, analysisLanguage)} + > + <Network className="tw-inline tw-mr-1 tw-h-3 tw-w-3" /> + <span>{domainText(domain, analysisLanguage)}</span> + </span> + )} - <Network className="tw-inline tw-mr-1 tw-h-3 tw-w-3" /> - <span>{domainText(domain, analysisLanguage)}</span> - </button>platform.bible-extension/src/components/add-new-entry-button.tsx (1)
25-39: Expose button state to AT users witharia-expandedSmall a11y win indicating the toggle state.
Apply this diff:
- return adding ? ( + return adding ? ( <div className="tw-border tw-rounded-lg tw-shadow-sm tw-p-4"> @@ - ) : ( - <Button onClick={() => setAdding(true)}> + ) : ( + <Button onClick={() => setAdding(true)} aria-expanded={false} aria-controls="add-new-entry-panel"> {localizedStrings['%fwLiteExtension_addWord_buttonAdd%']} </Button> );platform.bible-extension/src/components/dictionary-list.tsx (2)
56-58: Unify click and keyboard selection behavior (toggle on second click)Your keyboard handler toggles selection; the click handler (above) should mirror that. If you prefer centralizing, reuse
handleOptionSelect.Apply this diff:
- const handleOptionSelect = (option: ListboxOption) => { - setSelectedEntryId((prevId) => (prevId === option.id ? undefined : option.id)); - }; + const handleOptionSelect = (option: ListboxOption) => { + setSelectedEntryId((prevId) => (prevId === option.id ? undefined : option.id)); + };And at the call site (lines 106-112), either keep the toggle from the previous comment or:
- onClick={() => setSelectedEntryId(entry.id)} + onClick={() => handleOptionSelect({ id: entry.id })}
133-137: Only clear selection when Drawer closesRespect the
openboolean to avoid unexpected clearing.Apply this diff:
- <Drawer + <Drawer direction="right" open={drawerOpen} - onOpenChange={() => setSelectedEntryId(undefined)} + onOpenChange={(open) => { + if (!open) setSelectedEntryId(undefined); + }} >platform.bible-extension/src/web-views/add-word.web-view.tsx (4)
33-39: Log real error objects instead of JSON-stringifying.
JSON.stringify(e)is often{}for Errors; pass the error object.- .catch((e) => - logger.error( - `${localizedStrings['%fwLiteExtension_error_gettingNetworkObject%']}:`, - JSON.stringify(e), - ), - ); + .catch((e) => + logger.error(localizedStrings['%fwLiteExtension_error_gettingNetworkObject%'], e), + );
41-47: Clarify missing-param logs.Add a separator so the param name is readable.
- if (!projectId) logger.warn(`${errMissingParam}projectId`); - if (!fwLiteNetworkObject) logger.warn(`${errMissingParam}fwLiteNetworkObject`); + if (!projectId) logger.warn(`${errMissingParam}: projectId`); + if (!fwLiteNetworkObject) logger.warn(`${errMissingParam}: fwLiteNetworkObject`);
25-40: Avoid refetching the network object on locale changes.Re-running this effect on
localizedStringschanges is unnecessary churn.- }, [localizedStrings]); + }, []); // run once
73-75: Localize the status messages.“Adding entry…” and “Entry added!” should use localization keys for consistency.
I can add keys (e.g., %fwLiteExtension_addWord_adding%, %fwLiteExtension_addWord_added%) and wire them here—want me to push that?
platform.bible-extension/src/components/dictionary-list-wrapper.tsx (1)
30-41: Normalize layout padding/height for the items region.When
hasItemsis true,elementListrenders without the padding/height used by other states; wrap it for consistency.- {hasItems && elementList} + {hasItems && <div className="tw-flex-1 tw-p-2">{elementList}</div>}platform.bible-extension/src/components/dictionary-list-item.tsx (2)
20-31: Update the docblock to match the actual behavior.Comment mentions occurrences/Strong’s/tooltips/listbox hook that aren’t present here.
-/** - * A list item for a dictionary entry. - * - * This component is used to display a dictionary entry in a list of dictionary entries. - * - * The component renders a list item with the lemma of the dictionary entry, the number of - * occurrences in the chapter, and the Strong's codes for the dictionary entry. The component also - * renders a tooltip that displays the number of occurrences in the chapter. - * - * The component uses the `useListbox` hook from the `listbox-keyboard-navigation.util` module to - * handle keyboard navigation of the list. - */ +/** + * A list item that displays a dictionary entry’s headword, gloss, + * and optional semantic domains. Selection styling is controlled via `isSelected`. + */
69-76: Guard against missing senses.Be defensive in case
entry.sensesis empty/undefined.- domains={entry.senses.flatMap((s) => s.semanticDomains)} + domains={entry.senses?.flatMap((s) => s.semanticDomains) ?? []}platform.bible-extension/src/web-views/find-word.web-view.tsx (4)
38-45: Log real error objects instead of JSON-stringifying.Pass the error object directly for useful stack/message.
- .catch((e) => - logger.error( - `${localizedStrings['%fwLiteExtension_error_gettingNetworkObject%']}:`, - JSON.stringify(e), - ), - ); + .catch((e) => + logger.error(localizedStrings['%fwLiteExtension_error_gettingNetworkObject%'], e), + );
70-71: Clean up debounced calls on unmount.Avoid setState on unmounted component by cancelling pending debounce if supported.
const debouncedFetchEntries = useMemo(() => debounce(fetchEntries, 500), [fetchEntries]); + useEffect(() => { + return () => debouncedFetchEntries?.cancel?.(); + }, [debouncedFetchEntries]);
46-53: Clarify missing-param logs.Make the parameter name readable in warnings.
- if (!projectId) logger.warn(`${errMissingParam}projectId`); - if (!fwLiteNetworkObject) logger.warn(`${errMissingParam}fwLiteNetworkObject`); + if (!projectId) logger.warn(`${errMissingParam}: projectId`); + if (!fwLiteNetworkObject) logger.warn(`${errMissingParam}: fwLiteNetworkObject`);
30-45: Avoid refetching network object on locale changes.
localizedStringsin deps causes unnecessary reruns.- }, [localizedStrings]); + }, []);platform.bible-extension/src/components/dictionary-combo-box.tsx (2)
32-37: Log real error objects instead of JSON-stringifying.Improve diagnostics by passing the error object.
- .catch((e) => - logger.error( - localizedStrings['%fwLiteExtension_dictionarySelect_saveError%'], - JSON.stringify(e), - ), - ) + .catch((e) => logger.error(localizedStrings['%fwLiteExtension_dictionarySelect_saveError%'], e))
75-78: Reset “saved” state when selection changes.Prevents the “Saved” banner from persisting if the user picks a new dictionary.
- onChange={setSelectedDictionaryCode} + onChange={(code) => { + setSettingSaved(false); + setSelectedDictionaryCode(code); + }}platform.bible-extension/src/components/back-to-list-button.tsx (2)
34-35: Returnnullinstead ofundefinedfor no renderReact expects
nullfor “render nothing”; returningundefinedis atypical.- if (!handleBackToListButton) return undefined; + if (!handleBackToListButton) return null;
48-50: Unnecessaryjustify-betweenContainer has a single child. Use
justify-start(default) for simpler layout.- <div className="tw-mb-4 tw-flex tw-items-center tw-justify-between"> + <div className="tw-mb-4 tw-flex tw-items-center">platform.bible-extension/src/web-views/find-related-words.web-view.tsx (5)
48-53: Log the error object directly (avoid JSON.stringify pitfalls)Errors can have non-enumerable/circular fields; stringify may drop context.
- .catch((e) => - logger.error( - `${localizedStrings['%fwLiteExtension_error_gettingNetworkObject%']}:`, - JSON.stringify(e), - ), - ); + .catch((e) => + logger.error(`${localizedStrings['%fwLiteExtension_error_gettingNetworkObject%']}:`, e), + );
113-121: Cancel the debounced search on unmount to avoid setState on unmounted componentPrevents noisy logs and potential memory leaks.
const debouncedFetchEntries = useMemo(() => debounce(fetchEntries, 500), [fetchEntries]); const onSearch = useCallback( (searchQuery: string) => { setSearchTerm(searchQuery); debouncedFetchEntries(searchQuery); }, [debouncedFetchEntries], ); + useEffect(() => { + return () => { + // if debounce impl exposes cancel + (debouncedFetchEntries as any)?.cancel?.(); + }; + }, [debouncedFetchEntries]);
139-141: Pick headword deterministically usingvernacularLanguageUsing
Object.values(...).pop()relies on key order. Use the vernacular language explicitly.- onSearch(Object.values<string | undefined>(addedEntry.lexemeForm).pop() ?? ''); + onSearch(addedEntry.lexemeForm[vernacularLanguage ?? ''] ?? '');
74-76: Consider localizing this warningMinor consistency: other messages are localized.
188-207: Nested ternary reduces readabilityConsider extracting into a small render function or if/early-returns.
platform.bible-extension/src/utils/entry-display-text.ts (1)
3-31: Harden fallbacks and avoid empty/undefined stringsPrevent “undefined”/empty separators and provide graceful defaults.
export function domainText(domain: ISemanticDomain, lang = 'en'): string { - return `${domain.code}: ${domain.name[lang] || domain.name.en}`; + return `${domain.code}: ${domain.name[lang] || domain.name.en || ''}`; } export function entryGlossText(entry: IEntry, lang = 'en'): string { - return entry.senses.map((s) => senseGlossText(s, lang)).join(' | '); + return entry.senses.map((s) => senseGlossText(s, lang)).filter(Boolean).join(' | '); } export function entryHeadwordText(entry: IEntry, lang = 'en'): string { return ( entry.citationForm[lang] || entry.lexemeForm[lang] || - Object.values(entry.citationForm).filter(Boolean)[0] || - Object.values(entry.lexemeForm).filter(Boolean)[0] || + Object.values(entry.citationForm).find(Boolean) || + Object.values(entry.lexemeForm).find(Boolean) || '' ); } export function partOfSpeechText(partOfSpeech: IPartOfSpeech, lang = 'en'): string { - return partOfSpeech.name[lang] || partOfSpeech.name.en; + return partOfSpeech.name[lang] || partOfSpeech.name.en || partOfSpeech.id || ''; } export function senseDefinitionText(sense: ISense, lang = 'en'): string { - return sense.definition[lang] || Object.values(sense.definition).join('; '); + return sense.definition[lang] || Object.values(sense.definition).filter(Boolean).join('; ') || ''; } export function senseGlossText(sense: ISense, lang = 'en'): string { - return sense.gloss[lang] || Object.values(sense.gloss).join('; '); + return sense.gloss[lang] || Object.values(sense.gloss).filter(Boolean).join('; ') || ''; }platform.bible-extension/src/components/add-new-entry.tsx (2)
51-54: Use try/catch with async/await and log the error objectImproves readability and avoids stringify issues.
- await addEntry(entry) - .then(() => clearEntry()) - .catch((e) => logger.error('Error adding entry:', JSON.stringify(e))); + try { + await addEntry(entry); + clearEntry(); + } catch (e) { + logger.error('Error adding entry:', e); + }
93-99: Prevent double submit (optional)Add a submitting flag to disable buttons during the request.
- const [ready, setReady] = useState(false); + const [ready, setReady] = useState(false); + const [isSubmitting, setIsSubmitting] = useState(false); ... - async function onSubmit(): Promise<void> { + async function onSubmit(): Promise<void> { + if (isSubmitting) return; + setIsSubmitting(true); const entry = createEntry( vernacularLanguage, tempHeadword.trim(), analysisLanguage || 'en', gloss.trim(), definition.trim(), ); - try { + try { await addEntry(entry); clearEntry(); } catch (e) { logger.error('Error adding entry:', e); + } finally { + setIsSubmitting(false); } } ... - <Button disabled={!ready} onClick={() => onSubmit()}> + <Button disabled={!ready || isSubmitting} onClick={() => onSubmit()}> {localizedStrings['%fwLiteExtension_addWord_buttonSubmit%']} </Button> - <Button onClick={clearEntry}> + <Button disabled={isSubmitting} onClick={clearEntry}> {localizedStrings['%fwLiteExtension_button_cancel%']} </Button>platform.bible-extension/src/components/dictionary-entry-display.tsx (3)
39-42: Docstring is outdatedMentions Hebrew/Strong’s specifics not shown here. Update to reflect current UI.
87-99: Simplify sense iteration
sensesis already an array;Object.values(...).flat()is unnecessary.- {Object.values(dictionaryEntry.senses) - .flat() - .filter((sense) => sense !== undefined) - .map((sense, senseIndex) => ( + {dictionaryEntry.senses.map((sense, senseIndex) => (
123-131: Add accessible name to “scroll to top” buttonImproves screen reader support.
<Button variant="secondary" size="icon" className="tw-fixed tw-bottom-4 tw-right-4 tw-z-20" onClick={onClickScrollToTop} + aria-label={localizedStrings['%fwLiteExtension_button_scrollToTop%'] ?? 'Scroll to top'} + title={localizedStrings['%fwLiteExtension_button_scrollToTop%'] ?? 'Scroll to top'} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
platform.bible-extension/contributions/localizedStrings.json(1 hunks)platform.bible-extension/contributions/menus.json(0 hunks)platform.bible-extension/src/components/add-new-entry-button.tsx(1 hunks)platform.bible-extension/src/components/add-new-entry.tsx(2 hunks)platform.bible-extension/src/components/back-to-list-button.tsx(1 hunks)platform.bible-extension/src/components/dictionary-combo-box.tsx(2 hunks)platform.bible-extension/src/components/dictionary-entry-display.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list-item.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list-wrapper.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list.tsx(1 hunks)platform.bible-extension/src/components/domains-display.tsx(1 hunks)platform.bible-extension/src/components/entry-card.tsx(0 hunks)platform.bible-extension/src/main.ts(4 hunks)platform.bible-extension/src/types/fw-lite-extension.d.ts(1 hunks)platform.bible-extension/src/types/localized-string-keys.ts(1 hunks)platform.bible-extension/src/utils/entry-display-text.ts(1 hunks)platform.bible-extension/src/utils/fw-lite-api.ts(1 hunks)platform.bible-extension/src/utils/project-manager.ts(4 hunks)platform.bible-extension/src/utils/use-is-wide-screen.tsx(1 hunks)platform.bible-extension/src/web-views/add-word.web-view.tsx(4 hunks)platform.bible-extension/src/web-views/find-related-words.web-view.tsx(8 hunks)platform.bible-extension/src/web-views/find-word.web-view.tsx(6 hunks)platform.bible-extension/src/web-views/index.tsx(6 hunks)platform.bible-extension/src/web-views/main.web-view.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- platform.bible-extension/contributions/menus.json
- platform.bible-extension/src/components/entry-card.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/types/fw-lite-extension.d.ts:4-22
Timestamp: 2025-07-31T16:00:49.635Z
Learning: In the sillsdev/languageforge-lexbox repository, the platform.bible-extension is intentionally tightly coupled with the frontend's dotnet-types. The relative imports from `../../../frontend/viewer/src/lib/dotnet-types/index.js` in the extension's type declarations are by design, not a maintainability issue that needs to be addressed.
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/main.ts:239-246
Timestamp: 2025-07-31T19:10:41.178Z
Learning: In the sillsdev/languageforge-lexbox repository, user imnasnainaec prefers to defer code improvements when there are related TODO comments indicating planned refactoring work, choosing to bundle related changes together rather than making incremental improvements that would need to be modified again during the larger refactoring.
📚 Learning: 2025-07-31T17:31:59.999Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/utils/fw-lite-api.ts:48-53
Timestamp: 2025-07-31T17:31:59.999Z
Learning: In the sillsdev/languageforge-lexbox platform.bible-extension, the FwLiteApi.doesProjectMatchLanguage() method uses JSON.stringify() on writingSystems.vernacular data as an intentional temporary placeholder ("stand-in") until proper language code access can be implemented. This is not a code quality issue but a deliberate temporary solution.
Applied to files:
platform.bible-extension/src/utils/fw-lite-api.tsplatform.bible-extension/src/main.ts
📚 Learning: 2025-07-31T16:00:49.635Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/types/fw-lite-extension.d.ts:4-22
Timestamp: 2025-07-31T16:00:49.635Z
Learning: In the sillsdev/languageforge-lexbox repository, the platform.bible-extension is intentionally tightly coupled with the frontend's dotnet-types. The relative imports from `../../../frontend/viewer/src/lib/dotnet-types/index.js` in the extension's type declarations are by design, not a maintainability issue that needs to be addressed.
Applied to files:
platform.bible-extension/src/types/fw-lite-extension.d.tsplatform.bible-extension/src/main.ts
📚 Learning: 2025-07-31T19:33:04.864Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/webpack.config.ts:10-12
Timestamp: 2025-07-31T19:33:04.864Z
Learning: In the sillsdev/languageforge-lexbox repository's platform.bible-extension, the webpack configuration already enforces proper build order using webpack's `dependencies` array. The configWebView has `name: 'webView'` and configMain has `dependencies: ['webView']`, ensuring the WebView build completes before the main build starts.
Applied to files:
platform.bible-extension/src/utils/project-manager.tsplatform.bible-extension/src/web-views/index.tsx
📚 Learning: 2025-07-31T19:33:04.864Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/webpack.config.ts:10-12
Timestamp: 2025-07-31T19:33:04.864Z
Learning: In the sillsdev/languageforge-lexbox repository's platform.bible-extension, the webpack configuration already properly enforces build order using webpack's `dependencies` array. The configWebView has `name: 'webView'` and configMain has `dependencies: ['webView']`, ensuring the WebView build completes before the main build starts. This is the correct way to handle build dependencies in webpack multi-compiler setups.
Applied to files:
platform.bible-extension/src/web-views/index.tsx
🔇 Additional comments (23)
platform.bible-extension/src/utils/project-manager.ts (2)
1-1: Switch to named imports from @papi/backend looks good.
Using explicit imports improves tree-shaking and clarity. No issues spotted.
48-51: DictionarySelect: float layout addition is sensible.
The float window size is a nice UX touch. This relies on the openWebView options-spread being robust; see my fix below to avoid a crash when options is undefined.platform.bible-extension/src/main.ts (3)
110-113: LGTM: consistent baseUrl usage in browse flow
129-132: LGTM: consistent baseUrl usage in display flow
182-182: Confirm intended analysis language selection orderChanged from last (.pop()) to first ([0]). If the array is ordered by preference descending, [0] is correct; if ascending, this flips behavior.
Would you like a quick repo scan to locate where IWritingSystems.analysis is produced to confirm the order?
platform.bible-extension/src/types/fw-lite-extension.d.ts (4)
51-57: LGTM: expose OpenWebViewOptionsWithProjectId and BrowseWebViewOptions
59-63: LGTM: new DictionaryLanguages surface
64-68: LGTM: compose dictionary info via Partial
70-72: Verify downstream types compile with new exportsEnsure web-views and services import the updated exported interfaces without breaking type resolution.
Run TypeScript compile locally and confirm no errors referencing WordWebViewOptions/OpenWebViewOptionsWithDictionaryInfo.
platform.bible-extension/contributions/localizedStrings.json (2)
5-21: LGTM: expanded, clearer UI strings for new flows
41-45: LGTM: consistent WebView titles aligned with new providersplatform.bible-extension/src/web-views/index.tsx (6)
7-8: LGTM: styles split between main and tailwind bundles
35-37: LGTM: localized title for main WebView
55-57: LGTM: add-word provider uses tailwind and localized title
75-77: LGTM: dictionary-select provider uses tailwind and localized title
95-97: LGTM: find-word provider uses tailwind and localized title
115-117: LGTM: find-related-words provider uses tailwind and localized titleplatform.bible-extension/src/types/localized-string-keys.ts (1)
3-32: Quick check: ensure every key exists in localizedStrings.jsonRun this to flag any missing keys.
#!/usr/bin/env bash set -euo pipefail # Find the keys file and the localized strings file keys_file="platform.bible-extension/src/types/localized-string-keys.ts" json_file=$(fd -a localizedStrings.json | rg -m1 'platform\.bible-extension/.*/localizedStrings\.json' || true) if [[ -z "${json_file:-}" ]]; then echo "ERROR: platform.bible-extension/.../localizedStrings.json not found" >&2 exit 1 fi echo "Checking keys from: $keys_file" echo "Against localized strings file: $json_file" echo missing=0 # Extract all %...% tokens from the keys file while IFS= read -r key; do if ! rg -q --fixed-strings "$key" "$json_file"; then echo "Missing key: $key" ((missing++)) || true fi done < <(rg -o "%fwLiteExtension_[^%]+%" "$keys_file" | sort -u) if (( missing == 0 )); then echo "All keys present." else echo "$missing key(s) missing." exit 2 fiplatform.bible-extension/src/components/dictionary-list.tsx (1)
60-65: Verify listbox roles/ids matchuseListboxexpectationsEnsure each option element rendered in
DictionaryListItemhas role="option" and anidmatching theoptionsids soaria-activedescendantworks.#!/usr/bin/env bash # Inspect DictionaryListItem for role and id wiring rg -nC3 -g '!**/dist/**' -g '!**/build/**' 'DictionaryListItem|role=.option.|aria-selected|id=' platform.bible-extension/src | sed -n '1,200p'platform.bible-extension/src/web-views/add-word.web-view.tsx (1)
69-72: Confirm empty-string fallbacks are valid API.Passing
''for languages may be semantically different from undefined. VerifyAddNewEntryhandles this as intended.platform.bible-extension/src/components/dictionary-list-item.tsx (1)
45-57: ARIA: ensure parent provides listbox semantics.This renders
role="option"withtabIndex={-1}and suppresses key events. Confirm it’s always used within a parent implementing listbox keyboard handling andaria-activedescendant.platform.bible-extension/src/web-views/find-word.web-view.tsx (1)
116-119: Confirm empty-string fallbacks are valid API.Verify
AddNewEntryButtonand downstream logic treat''languages as “unset”.platform.bible-extension/src/components/back-to-list-button.tsx (1)
37-44: ConfirmListboxOptionshape ({ id }only) is correctIf
ListboxOptionexpects other fields (e.g.,label/value), this click payload may break consumers.Run to locate the type and usages:
#!/bin/bash rg -nP -C2 '\bListboxOption\b|handleBackToListButton\(' --type ts --type tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
platform.bible-extension/src/web-views/find-related-words.web-view.tsx (2)
78-87: Ensure loading state clears on errors (use try/finally) in fetchEntriesIf
getEntriesthrows,setIsFetching(false)isn’t reached, leaving the UI stuck in loading.Apply this diff:
logger.info(`Fetching entries for ${surfaceForm}`); setIsFetching(true); - let entries = (await fwLiteNetworkObject.getEntries(projectId, { surfaceForm })) ?? []; - // Only consider entries and senses with at least one semantic domain. - entries = entries - .map((e) => ({ ...e, senses: e.senses.filter((s) => s.semanticDomains.length) })) - .filter((e) => e.senses.length); - setIsFetching(false); - setMatchingEntries(entries); + try { + let entries = (await fwLiteNetworkObject.getEntries(projectId, { surfaceForm })) ?? []; + // Only consider entries and senses with at least one semantic domain. + entries = entries + .map((e) => ({ ...e, senses: e.senses.filter((s) => s.semanticDomains.length) })) + .filter((e) => e.senses.length); + setMatchingEntries(entries); + } catch (e) { + logger.error('Error fetching entries', JSON.stringify(e)); + } finally { + setIsFetching(false); + }Optionally localize the error message for consistency.
100-105: Also wrap fetchRelatedEntries with try/finallySame stuck-loading risk here if the call throws.
Apply this diff:
logger.info(`Fetching entries in semantic domain ${semanticDomain}`); setIsFetching(true); - const entries = await fwLiteNetworkObject.getEntries(projectId, { semanticDomain }); - setIsFetching(false); - setRelatedEntries(entries ?? []); + try { + const entries = await fwLiteNetworkObject.getEntries(projectId, { semanticDomain }); + setRelatedEntries(entries ?? []); + } catch (e) { + logger.error('Error fetching related entries', JSON.stringify(e)); + } finally { + setIsFetching(false); + }platform.bible-extension/src/web-views/add-word.web-view.tsx (1)
50-60: Ensure submitting state clears on errors (use try/finally) in addEntryIf
addEntrythrows,setIsSubmitting(false)isn’t hit, leaving the UI stuck.Apply this diff:
- setIsSubmitted(false); - setIsSubmitting(true); - logger.info(`Adding entry: ${JSON.stringify(entry)}`); - const entryId = (await fwLiteNetworkObject.addEntry(projectId, entry))?.id; - setIsSubmitting(false); - if (entryId) { - setIsSubmitted(true); - await papi.commands.sendCommand('fwLiteExtension.displayEntry', projectId, entryId); - } else { - logger.error(`${localizedStrings['%fwLiteExtension_error_failedToAddEntry%']}`); - } + setIsSubmitted(false); + setIsSubmitting(true); + logger.info(`Adding entry: ${JSON.stringify(entry)}`); + try { + const entryId = (await fwLiteNetworkObject.addEntry(projectId, entry))?.id; + if (entryId) { + setIsSubmitted(true); + await papi.commands.sendCommand('fwLiteExtension.displayEntry', projectId, entryId); + } else { + logger.error(`${localizedStrings['%fwLiteExtension_error_failedToAddEntry%']}`); + } + } catch (e) { + logger.error(`${localizedStrings['%fwLiteExtension_error_failedToAddEntry%']}`, JSON.stringify(e)); + } finally { + setIsSubmitting(false); + }platform.bible-extension/src/web-views/find-word.web-view.tsx (1)
61-66: Ensure isFetching is reset on failure (wrap in try/catch/finally)Current code can leave loading stuck and swallow errors.
- setIsFetching(true); - const entries = await fwLiteNetworkObject.getEntries(projectId, { surfaceForm }); - setIsFetching(false); - setMatchingEntries(entries ?? []); + setIsFetching(true); + try { + const entries = await fwLiteNetworkObject.getEntries(projectId, { surfaceForm }); + setMatchingEntries(entries ?? []); + } catch (e) { + logger.error('Error fetching entries', e); + } finally { + setIsFetching(false); + }
♻️ Duplicate comments (1)
platform.bible-extension/src/utils/project-manager.ts (1)
85-93: Clarify log messages to distinguish reload from open.The log at line 87 says "Opening..." but the code may reload an existing WebView instead (lines 89-91). This was previously flagged and a suggestion was provided to log "Reloaded..." on successful reload and "Opening..." only when actually opening a new WebView.
🧹 Nitpick comments (16)
platform.bible-extension/src/utils/use-is-wide-screen.tsx (1)
5-16: Guard againstwindowbeing undefined.Directly touching
windowin the initializer and effect will throw in SSR or non-browser tests. Add a guard so the hook no-ops safely whenwindowisn’t available.- const [isWide, setIsWide] = useState(() => window.innerWidth >= 1024); + const [isWide, setIsWide] = useState( + () => (typeof window !== 'undefined' ? window.innerWidth >= 1024 : false), + ); useEffect(() => { - // Matches Tailwind css lg breakpoint - const mediaQuery = window.matchMedia('(min-width: 1024px)'); + if (typeof window === 'undefined' || typeof window.matchMedia !== 'function') return; + + // Matches Tailwind css lg breakpoint + const mediaQuery = window.matchMedia('(min-width: 1024px)');platform.bible-extension/src/web-views/main.web-view.tsx (1)
2-2: Remove unused ref and prefer localized Loading text
useRefand theiframeref are unused. Drop both and thereact-hooksdisable if no other hooks remain.- Consider localizing "Loading..." to match the rest of the extension’s i18n. You can also use a Platform.Bible UI component for consistency. Based on learnings
Apply this diff to remove the unused ref:
-import { useRef } from 'react'; +// No hooks needed @@ -globalThis.webViewComponent = function fwLiteMainWindow({ url }: BrowseWebViewOptions) { - // eslint-disable-next-line no-null/no-null - const iframe = useRef<HTMLIFrameElement | null>(null); +globalThis.webViewComponent = function fwLiteMainWindow({ url }: BrowseWebViewOptions) { @@ - return url ? <iframe ref={iframe} src={url} title="FieldWorks Lite" /> : <p>Loading...</p>; + return url ? <iframe src={url} title="FieldWorks Lite" /> : <p>Loading...</p>; }Also applies to: 8-10
platform.bible-extension/src/web-views/find-related-words.web-view.tsx (4)
113-121: Cancel pending debounced searches on unmount to prevent state updates after unmountClean up the debounced function when the component unmounts or when the debounced callback changes to avoid late updates.
If
platform-bible-utilsdebounce exposescancelorclear, add:
- A cleanup effect:
- useEffect(() => () => debouncedFetchEntries.cancel?.() ?? debouncedFetchEntries.clear?.(), [debouncedFetchEntries])
Please verify the debounce API in platform-bible-utils and wire the appropriate cleanup.
139-140: Pick headword deterministically using language preference orderUsing
Object.values(...).pop()is nondeterministic across languages. PrefervernacularLanguage, thenanalysisLanguage, then first available.Apply this diff:
- onSearch(Object.values<string | undefined>(addedEntry.lexemeForm).pop() ?? ''); + const headword = + (vernacularLanguage && addedEntry.lexemeForm[vernacularLanguage]) || + (analysisLanguage && addedEntry.lexemeForm[analysisLanguage]) || + Object.values(addedEntry.lexemeForm).find(Boolean) || + ''; + onSearch(headword);
208-210: Make hasItems reflect the currently displayed listWhen showing related entries, base
hasItemsonrelatedEntries, notmatchingEntries.Apply this diff:
- isLoading={isFetching} - hasItems={!!matchingEntries?.length} + isLoading={isFetching} + hasItems={selectedDomain ? !!relatedEntries?.length : !!matchingEntries?.length}
72-76: Localize the “No word provided for search” warning (optional)For consistency with other messages, consider localizing this warning.
Confirm whether a suitable key exists in
LOCALIZED_STRING_KEYS; if so, use it here.platform.bible-extension/src/web-views/add-word.web-view.tsx (1)
73-75: Localize user-visible status text (optional)Consider localizing "Adding entry to FieldWorks..." and "Entry added!" to align with the rest of the UI.
If keys already exist in
LOCALIZED_STRING_KEYS, wire them here; otherwise, add new keys.platform.bible-extension/src/web-views/find-word.web-view.tsx (3)
55-59: Localize the “no word provided” messageUse a localized string for this warning for consistency with the rest.
92-99: Prefer vernacularLanguage when deriving new search term after addUse the vernacular language headword (when known) instead of Object.values(...).pop(); update deps.
- onSearch(Object.values<string | undefined>(addedEntry.lexemeForm).pop() ?? ''); + const newHeadword = vernacularLanguage + ? addedEntry.lexemeForm[vernacularLanguage] ?? '' + : Object.values<string | undefined>(addedEntry.lexemeForm).pop() ?? ''; + onSearch(newHeadword); @@ - [fwLiteNetworkObject, localizedStrings, onSearch, projectId], + [fwLiteNetworkObject, localizedStrings, onSearch, projectId, vernacularLanguage],
70-71: Cancel debounced calls on unmountAvoid stray requests and state updates after unmount if debounce supports cancel().
const debouncedFetchEntries = useMemo(() => debounce(fetchEntries, 500), [fetchEntries]); + useEffect(() => { + return () => debouncedFetchEntries?.cancel?.(); + }, [debouncedFetchEntries]);Also applies to: 78-78
platform.bible-extension/src/components/dictionary-list-wrapper.tsx (1)
40-41: Wrap list in a scrolling container to keep header stickyWithout a flex-1/overflow container for the list, the sticky header may not behave consistently.
- {hasItems && elementList} + {hasItems && <div className="tw-flex-1 tw-overflow-auto tw-p-2">{elementList}</div>}platform.bible-extension/src/components/add-new-entry.tsx (3)
51-54: Log localized error and pass the error objectAvoid JSON.stringify on Error; use the existing localized failure message.
- await addEntry(entry) - .then(() => clearEntry()) - .catch((e) => logger.error('Error adding entry:', JSON.stringify(e))); + await addEntry(entry) + .then(() => clearEntry()) + .catch((e) => + logger.error(localizedStrings['%fwLiteExtension_error_failedToAddEntry%'] ?? 'Error adding entry', e), + );
62-90: Use unique ids to avoid collisions if multiple forms existGenerate ids with useId and append to htmlFor/id.
-import { type ReactElement, useCallback, useEffect, useState } from 'react'; +import { type ReactElement, useCallback, useEffect, useId, useState } from 'react'; @@ - const [localizedStrings] = useLocalizedStrings(LOCALIZED_STRING_KEYS); + const [localizedStrings] = useLocalizedStrings(LOCALIZED_STRING_KEYS); + const uid = useId(); @@ - <Label htmlFor="newEntryHeadword"> + <Label htmlFor={`newEntryHeadword-${uid}`}> @@ - <Input - id="newEntryHeadword" + <Input + id={`newEntryHeadword-${uid}`} @@ - <Label htmlFor="newEntryGloss"> + <Label htmlFor={`newEntryGloss-${uid}`}> @@ - <Input id="newEntryGloss" onChange={(e) => setGloss(e.target.value)} value={gloss} /> + <Input id={`newEntryGloss-${uid}`} onChange={(e) => setGloss(e.target.value)} value={gloss} /> @@ - <Label htmlFor="newEntryDefinition"> + <Label htmlFor={`newEntryDefinition-${uid}`}> @@ - <Input - id="newEntryDefinition" + <Input + id={`newEntryDefinition-${uid}`}
92-96: Minor: remove arrow wrapper in onClickCleaner and avoids re-creating a function each render.
- <Button disabled={!ready} onClick={() => onSubmit()}> + <Button disabled={!ready} onClick={onSubmit}>platform.bible-extension/contributions/localizedStrings.json (2)
27-28: Avoid double-colon in error_gettingNetworkObjectIf you keep the code as-is, drop the trailing colon here. Otherwise, the code change I suggested removes the extra colon.
- "%fwLiteExtension_error_gettingNetworkObject%": "Error getting network object:", + "%fwLiteExtension_error_gettingNetworkObject%": "Error getting network object",
14-15: Unify ellipsis spacingMinor copy edit for consistency with other “Loading...” strings.
- "%fwLiteExtension_dictionarySelect_loading%": "Loading dictionaries ...", + "%fwLiteExtension_dictionarySelect_loading%": "Loading dictionaries...",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
platform.bible-extension/contributions/localizedStrings.json(1 hunks)platform.bible-extension/contributions/menus.json(0 hunks)platform.bible-extension/src/components/add-new-entry-button.tsx(1 hunks)platform.bible-extension/src/components/add-new-entry.tsx(2 hunks)platform.bible-extension/src/components/back-to-list-button.tsx(1 hunks)platform.bible-extension/src/components/dictionary-combo-box.tsx(2 hunks)platform.bible-extension/src/components/dictionary-entry-display.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list-item.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list-wrapper.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list.tsx(1 hunks)platform.bible-extension/src/components/domains-display.tsx(1 hunks)platform.bible-extension/src/components/entry-card.tsx(0 hunks)platform.bible-extension/src/main.ts(5 hunks)platform.bible-extension/src/types/fw-lite-extension.d.ts(2 hunks)platform.bible-extension/src/types/localized-string-keys.ts(1 hunks)platform.bible-extension/src/utils/entry-display-text.ts(1 hunks)platform.bible-extension/src/utils/fw-lite-api.ts(1 hunks)platform.bible-extension/src/utils/project-manager.ts(4 hunks)platform.bible-extension/src/utils/use-is-wide-screen.tsx(1 hunks)platform.bible-extension/src/web-views/add-word.web-view.tsx(4 hunks)platform.bible-extension/src/web-views/find-related-words.web-view.tsx(8 hunks)platform.bible-extension/src/web-views/find-word.web-view.tsx(6 hunks)platform.bible-extension/src/web-views/index.tsx(6 hunks)platform.bible-extension/src/web-views/main.web-view.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- platform.bible-extension/contributions/menus.json
- platform.bible-extension/src/components/entry-card.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- platform.bible-extension/src/components/domains-display.tsx
- platform.bible-extension/src/types/localized-string-keys.ts
- platform.bible-extension/src/utils/entry-display-text.ts
- platform.bible-extension/src/web-views/index.tsx
- platform.bible-extension/src/utils/fw-lite-api.ts
- platform.bible-extension/src/components/dictionary-list.tsx
- platform.bible-extension/src/components/back-to-list-button.tsx
- platform.bible-extension/src/components/dictionary-list-item.tsx
- platform.bible-extension/src/components/dictionary-entry-display.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T16:03:23.277Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#2004
File: platform.bible-extension/src/utils/project-manager.ts:85-93
Timestamp: 2025-10-13T16:03:23.277Z
Learning: In JavaScript/TypeScript, spreading falsy values (undefined, null) in object literals is safe and does not throw runtime errors. For example, `{...undefined}` evaluates to `{}`.
Applied to files:
platform.bible-extension/src/utils/project-manager.ts
🔇 Additional comments (13)
platform.bible-extension/src/utils/project-manager.ts (3)
1-1: LGTM! Import refactor improves clarity.Switching to named imports from
@papi/backendmakes dependencies explicit and aligns with the backend API structure.
48-51: LGTM! Float configuration is appropriate.The
floatSizeandtypeparameters properly configure the dictionary selector as a floating modal with sensible dimensions.
103-103: LGTM! Data provider access updated correctly.Using the named import
projectDataProvidersis consistent with the refactored imports and maintains the same functionality.platform.bible-extension/src/main.ts (4)
42-43: LGTM! Cleaner destructuring pattern.Directly destructuring
baseUrlandfwLiteProcessfromlaunchFwLiteWebis more readable and explicit than using a holder object pattern.
110-110: LGTM! Consistent usage of destructured baseUrl.The direct usage of
baseUrlis clean and consistent with the refactored destructuring pattern.Also applies to: 129-129
242-274: LGTM! Well-structured launch function.The
launchFwLiteWebfunction is clean and well-organized:
- Proper privilege and platform validation
- Good process lifecycle management with logging
- Clean return structure that matches the destructuring pattern
- TODO comment appropriately documents the hardcoded URL limitation
182-182: Approve first analysis language selection: Using[0]aligns with existing defaultAnalysis conventions and avoids mutation; no downstream code relies on the old side effect.platform.bible-extension/src/components/add-new-entry-button.tsx (1)
1-40: Well-structured toggle and localized labelGood state toggle, prop pass-through, and localization usage. Matches the PB style components. Based on learnings
platform.bible-extension/src/components/dictionary-combo-box.tsx (1)
1-93: Good localization coverage and clear save flowLocalized placeholders/messages, proper disabled states, and tidy save handling. UI matches PB components. Based on learnings
platform.bible-extension/src/web-views/find-word.web-view.tsx (1)
113-121: Verify AddNewEntryButton behavior when languages are missingYou pass empty strings for analysisLanguage/vernacularLanguage when not provided. Confirm the button disables or guards to prevent creating entries with '' as a language key.
platform.bible-extension/src/components/add-new-entry.tsx (1)
44-50: Is the 'en' fallback intended for analysisLanguage?Defaulting to 'en' can mis-attribute gloss/definition when the project uses another analysis language. Consider requiring a valid code or blocking submit until present.
platform.bible-extension/src/types/fw-lite-extension.d.ts (2)
51-53: Confirm projectId optionalityMany flows require a project id; if practically required, consider making it required here or create a separate Required variant.
59-63: DictionaryLanguages: do you allow empty strings?UI passes empty strings in some cases. Either make these optional in consumer-facing options or enforce valid non-empty codes to prevent invalid entries.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform.bible-extension/src/web-views/find-related-words.web-view.tsx (2)
63-89: Ensure isFetching is cleared on errors; wrap in try/catch/finallyPrevents stuck loading and surfaces errors.
const fetchEntries = useCallback( async (untrimmedSurfaceForm: string) => { if (!projectId || !fwLiteNetworkObject) { const errMissingParam = localizedStrings['%fwLiteExtension_error_missingParam%']; if (!projectId) logger.warn(`${errMissingParam}projectId`); if (!fwLiteNetworkObject) logger.warn(`${errMissingParam}fwLiteNetworkObject`); return; } const surfaceForm = untrimmedSurfaceForm.trim(); if (!surfaceForm) { logger.warn('No word provided for search'); return; } logger.info(`Fetching entries for ${surfaceForm}`); - setIsFetching(true); - let entries = (await fwLiteNetworkObject.getEntries(projectId, { surfaceForm })) ?? []; - // Only consider entries and senses with at least one semantic domain. - entries = entries - .map((e) => ({ ...e, senses: e.senses.filter((s) => s.semanticDomains.length) })) - .filter((e) => e.senses.length); - setIsFetching(false); - setMatchingEntries(entries); + setIsFetching(true); + try { + let entries = (await fwLiteNetworkObject.getEntries(projectId, { surfaceForm })) ?? []; + // Only consider entries and senses with at least one semantic domain. + entries = entries + .map((e) => ({ ...e, senses: e.senses.filter((s) => s.semanticDomains.length) })) + .filter((e) => e.senses.length); + setMatchingEntries(entries); + } catch (e) { + logger.error('Failed to fetch entries', e); + } finally { + setIsFetching(false); + } }, [fwLiteNetworkObject, localizedStrings, projectId], );
91-107: Mirror try/catch/finally in fetchRelatedEntriesSame reliability concern applies.
const fetchRelatedEntries = useCallback( async (semanticDomain: string) => { if (!projectId || !fwLiteNetworkObject) { const errMissingParam = localizedStrings['%fwLiteExtension_error_missingParam%']; if (!projectId) logger.warn(`${errMissingParam}projectId`); if (!fwLiteNetworkObject) logger.warn(`${errMissingParam}fwLiteNetworkObject`); return; } logger.info(`Fetching entries in semantic domain ${semanticDomain}`); - setIsFetching(true); - const entries = await fwLiteNetworkObject.getEntries(projectId, { semanticDomain }); - setIsFetching(false); - setRelatedEntries(entries ?? []); + setIsFetching(true); + try { + const entries = await fwLiteNetworkObject.getEntries(projectId, { semanticDomain }); + setRelatedEntries(entries ?? []); + } catch (e) { + logger.error('Failed to fetch related entries', e); + } finally { + setIsFetching(false); + } }, [fwLiteNetworkObject, localizedStrings, projectId], );
♻️ Duplicate comments (1)
platform.bible-extension/src/utils/project-manager.ts (1)
85-93: Consider clarifying log messages to distinguish reload from open.The log on Line 87 states "Opening" before attempting to reload an existing WebView (Line 89), which can be misleading when a reload succeeds.
Per previous discussion, consider logging "Reloaded ..." when successfully reloading and "Opening ..." only when actually opening:
const webViewId = this.webViewIds[webViewType]; const newOptions = { ...options, projectId: this.projectId }; - logger.info(`Opening ${webViewType} WebView for project ${this.projectId}`); - logger.info(`WebView options: ${JSON.stringify(newOptions)}`); - if (webViewId && (await webViews.reloadWebView(webViewType, webViewId, newOptions))) { - return true; - } - this.webViewIds[webViewType] = await webViews.openWebView(webViewType, layout, newOptions); + if (webViewId) { + const reloaded = await webViews.reloadWebView(webViewType, webViewId, newOptions); + if (reloaded) { + logger.info(`Reloaded ${webViewType} WebView for project ${this.projectId}`); + return true; + } + } + logger.info(`Opening ${webViewType} WebView for project ${this.projectId}`); + logger.info(`WebView options: ${JSON.stringify(newOptions)}`); + this.webViewIds[webViewType] = await webViews.openWebView(webViewType, layout, newOptions);
🧹 Nitpick comments (10)
platform.bible-extension/src/utils/fw-lite-api.ts (1)
93-104: Log the error before returning the fallback.The try/catch improves resilience, but silently swallowing errors can make debugging difficult. Consider logging the error to provide visibility into failures.
Apply this diff to add error logging:
try { const matches = ( await Promise.all( projects.map(async (p) => (await this.doesProjectMatchLangTag(p.code, langTag)) ? p : undefined, ), ) ).filter((p) => p) as IProjectModel[]; return matches.length ? matches : projects; - } catch { + } catch (error) { + logger.error('Error filtering projects by language tag:', error); return projects; }platform.bible-extension/src/utils/use-is-wide-screen.tsx (1)
5-5: Optional: Redundant initial state calculation.The initial state on Line 5 uses
window.innerWidth >= 1024, but Line 15 immediately overwrites it withmediaQuery.matcheson mount, making the lazy initializer unnecessary.Consider simplifying to:
- const [isWide, setIsWide] = useState(() => window.innerWidth >= 1024); + const [isWide, setIsWide] = useState(false);Or remove the redundant
setIsWideon Line 15 if you want to keep the initializer:useEffect(() => { // Matches Tailwind css lg breakpoint const mediaQuery = window.matchMedia('(min-width: 1024px)'); const handler = (e: MediaQueryListEvent) => setIsWide(e.matches); mediaQuery.addEventListener('change', handler); - // Set initial state - setIsWide(mediaQuery.matches); - return () => mediaQuery.removeEventListener('change', handler); }, []);Note: Since this is copied from the source implementation (per the comment), you may prefer to maintain consistency with the original.
Also applies to: 15-15
platform.bible-extension/src/components/dictionary-list-wrapper.tsx (1)
30-39: Add aria-live/role for loading and empty statesImprove SR feedback by marking status regions and loading state.
- {isLoading && ( - <div className="tw-flex-1 tw-p-2 tw-space-y-4"> + {isLoading && ( + <div + className="tw-flex-1 tw-p-2 tw-space-y-4" + role="status" + aria-live="polite" + aria-busy="true" + > <Label>{localizedStrings['%fwLiteExtension_dictionary_loading%']}</Label> </div> )} - {!hasItems && !isLoading && ( - <div className="tw-m-4 tw-flex tw-justify-center"> + {!hasItems && !isLoading && ( + <div className="tw-m-4 tw-flex tw-justify-center" role="status" aria-live="polite"> <Label>{localizedStrings['%fwLiteExtension_dictionary_noResults%']}</Label> </div> )}platform.bible-extension/src/web-views/find-related-words.web-view.tsx (4)
40-55: Avoid refetching network object on locale changes; log error objects, not JSON.stringifyRun once on mount; pass the error object directly.
useEffect(() => { papi.networkObjects .get<IEntryService>('fwliteextension.entryService') // eslint-disable-next-line promise/always-return .then((networkObject) => { logger.info('Got network object:', networkObject); setFwLiteNetworkObject(networkObject); }) - .catch((e) => - logger.error( - `${localizedStrings['%fwLiteExtension_error_gettingNetworkObject%']}:`, - JSON.stringify(e), - ), - ); - }, [localizedStrings]); + .catch((e) => + logger.error(`${localizedStrings['%fwLiteExtension_error_gettingNetworkObject%']}:`, e), + ); + }, []);
113-114: Consider cancelling the debounced search on unmountPrevents late setState after unmount and reduces wasted work.
const debouncedFetchEntries = useMemo(() => debounce(fetchEntries, 500), [fetchEntries]); + useEffect(() => { + return () => { + // If the debounce utility exposes cancel/clear, call it + // Optional chaining avoids runtime error if unsupported + (debouncedFetchEntries as any)?.cancel?.(); + }; + }, [debouncedFetchEntries]);
72-76: Localize this warningAlign with the rest of localized logs.
Replace the hard-coded string:
- logger.warn('No word provided for search');
With a localized string key, e.g., logger.warn(localizedStrings['%fwLiteExtension_warning_noWordProvided%']).
139-141: Prefer vernacularLanguage when deriving the new headwordAvoids relying on object property order.
- onSearch(Object.values<string | undefined>(addedEntry.lexemeForm).pop() ?? ''); + const lexeme = + addedEntry.lexemeForm[vernacularLanguage ?? ''] ?? + Object.values(addedEntry.lexemeForm).find((v) => !!v) ?? + ''; + onSearch(lexeme);platform.bible-extension/src/utils/entry-display-text.ts (1)
3-31: Use nullish coalescing and first-available fallbacks; avoid extra array scansMore robust when values are empty strings and when 'en' is absent. Also reduce work with find().
export function domainText(domain: ISemanticDomain, lang = 'en'): string { - return `${domain.code}: ${domain.name[lang] || domain.name.en}`; + const name = + domain.name[lang] ?? + domain.name.en ?? + Object.values(domain.name).find((v) => !!v) ?? + ''; + return `${domain.code}: ${name}`; } export function entryGlossText(entry: IEntry, lang = 'en'): string { return entry.senses.map((s) => senseGlossText(s, lang)).join(' | '); } export function entryHeadwordText(entry: IEntry, lang = 'en'): string { return ( - entry.citationForm[lang] || - entry.lexemeForm[lang] || - Object.values(entry.citationForm).filter(Boolean)[0] || - Object.values(entry.lexemeForm).filter(Boolean)[0] || + entry.citationForm[lang] ?? + entry.lexemeForm[lang] ?? + Object.values(entry.citationForm).find((v) => !!v) ?? + Object.values(entry.lexemeForm).find((v) => !!v) ?? '' ); } export function partOfSpeechText(partOfSpeech: IPartOfSpeech, lang = 'en'): string { - return partOfSpeech.name[lang] || partOfSpeech.name.en; + return ( + partOfSpeech.name[lang] ?? + partOfSpeech.name.en ?? + Object.values(partOfSpeech.name).find((v) => !!v) ?? + '' + ); } export function senseDefinitionText(sense: ISense, lang = 'en'): string { - return sense.definition[lang] || Object.values(sense.definition).join('; '); + return sense.definition[lang] ?? Object.values(sense.definition).filter(Boolean).join('; '); } export function senseGlossText(sense: ISense, lang = 'en'): string { - return sense.gloss[lang] || Object.values(sense.gloss).join('; '); + return sense.gloss[lang] ?? Object.values(sense.gloss).filter(Boolean).join('; '); }platform.bible-extension/src/components/dictionary-entry-display.tsx (1)
119-127: Add accessible name to the scroll-to-top buttonProvide an aria-label (localized if available).
<div> <Button variant="secondary" size="icon" className="tw-fixed tw-bottom-4 tw-right-4 tw-z-20" onClick={onClickScrollToTop} + aria-label={localizedStrings['%fwLiteExtension_action_scrollToTop%'] ?? 'Scroll to top'} > <ChevronUpIcon /> </Button> </div>platform.bible-extension/src/components/add-new-entry.tsx (1)
51-54: Simplify async flow; log error object directlyUse try/catch; avoid JSON.stringify on errors.
- await addEntry(entry) - .then(() => clearEntry()) - .catch((e) => logger.error('Error adding entry:', JSON.stringify(e))); + try { + await addEntry(entry); + clearEntry(); + } catch (e) { + logger.error('Error adding entry', e); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
platform.bible-extension/contributions/localizedStrings.json(1 hunks)platform.bible-extension/contributions/menus.json(0 hunks)platform.bible-extension/src/components/add-new-entry-button.tsx(1 hunks)platform.bible-extension/src/components/add-new-entry.tsx(2 hunks)platform.bible-extension/src/components/back-to-list-button.tsx(1 hunks)platform.bible-extension/src/components/dictionary-combo-box.tsx(2 hunks)platform.bible-extension/src/components/dictionary-entry-display.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list-item.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list-wrapper.tsx(1 hunks)platform.bible-extension/src/components/dictionary-list.tsx(1 hunks)platform.bible-extension/src/components/domains-display.tsx(1 hunks)platform.bible-extension/src/components/entry-card.tsx(0 hunks)platform.bible-extension/src/main.ts(5 hunks)platform.bible-extension/src/types/fw-lite-extension.d.ts(2 hunks)platform.bible-extension/src/types/localized-string-keys.ts(1 hunks)platform.bible-extension/src/utils/entry-display-text.ts(1 hunks)platform.bible-extension/src/utils/fw-lite-api.ts(1 hunks)platform.bible-extension/src/utils/project-manager.ts(4 hunks)platform.bible-extension/src/utils/use-is-wide-screen.tsx(1 hunks)platform.bible-extension/src/web-views/add-word.web-view.tsx(4 hunks)platform.bible-extension/src/web-views/find-related-words.web-view.tsx(8 hunks)platform.bible-extension/src/web-views/find-word.web-view.tsx(6 hunks)platform.bible-extension/src/web-views/index.tsx(6 hunks)platform.bible-extension/src/web-views/main.web-view.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- platform.bible-extension/contributions/menus.json
- platform.bible-extension/src/components/entry-card.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- platform.bible-extension/src/main.ts
- platform.bible-extension/contributions/localizedStrings.json
- platform.bible-extension/src/components/dictionary-combo-box.tsx
- platform.bible-extension/src/components/add-new-entry-button.tsx
- platform.bible-extension/src/components/back-to-list-button.tsx
- platform.bible-extension/src/types/localized-string-keys.ts
- platform.bible-extension/src/components/domains-display.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T16:03:23.277Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#2004
File: platform.bible-extension/src/utils/project-manager.ts:85-93
Timestamp: 2025-10-13T16:03:23.277Z
Learning: In JavaScript/TypeScript, spreading falsy values (undefined, null) in object literals is safe and does not throw runtime errors. For example, `{...undefined}` evaluates to `{}`.
Applied to files:
platform.bible-extension/src/utils/project-manager.ts
🔇 Additional comments (6)
platform.bible-extension/src/web-views/main.web-view.tsx (1)
1-11: LGTM! Clean simplification.The refactor to directly render the iframe or loading message is clear and removes unnecessary complexity.
platform.bible-extension/src/web-views/index.tsx (1)
7-8: LGTM! Consistent localization key updates.The migration from
mainStylestomainCssStyles, introduction oftailwindCssStyles, and updated localization keys (e.g.,fwLiteExtension_webViewTitle_*) are applied consistently across all web view providers.Also applies to: 35-36, 55-56, 75-76, 95-96, 115-116
platform.bible-extension/src/components/dictionary-list-item.tsx (1)
44-57: LGTM! Proper listbox semantics.The
<li>element correctly usesrole="option",aria-selected, andtabIndex={-1}for keyboard-navigable listbox behavior.platform.bible-extension/src/web-views/add-word.web-view.tsx (1)
3-3: LGTM! Proper localization integration.The localization hook is correctly integrated with
useLocalizedStrings, all error messages and warnings now use localized strings, and dependency arrays are properly updated. The prop name changes (analysisLang→analysisLanguage,vernacularLang→vernacularLanguage) align with the broader API surface updates.Also applies to: 7-7, 17-17, 33-39, 44-46, 59-59, 62-62, 66-72
platform.bible-extension/src/web-views/find-word.web-view.tsx (1)
3-3: LGTM! Comprehensive localization and UI refactor.The localization integration follows the same pattern as
add-word.web-view.tsx, with proper error handling and dependency management. The UI refactor withDictionaryListWrapper,DictionaryList, andAddNewEntryButtoncreates a cleaner, more modular structure aligned with the broader PR objectives.Also applies to: 8-11, 21-21, 38-44, 49-51, 67-67, 83-85, 95-95, 98-98, 102-134
platform.bible-extension/src/types/fw-lite-extension.d.ts (1)
20-72: Good modularization of dictionary/webview option typings.Breaking out
PartialEntry,DictionaryLanguages, and the layered webview option interfaces keeps the API surface tidy and matches the component usage patterns. Reads well—no issues from my side.
2 participants
Refactor components toward the style and structure of https://github.com/paranext/paranext-core/tree/main/extensions/src/platform-lexical-tools