fix: resolve performance issues causing UI lag#93
Open
eweren wants to merge 8 commits intotolgee:mainfrom
Open
fix: resolve performance issues causing UI lag#93eweren wants to merge 8 commits intotolgee:mainfrom
eweren wants to merge 8 commits intotolgee:mainfrom
Conversation
…n keys - Fix memory leak in useSelectedNodes (missing dep array, debounce dual events) - Stabilize ListItem handlers with useCallback, consolidate race-prone effects - Memoize props in Index, NamespaceSelect, ProjectSettings, AutocompleteSelect - Wrap KeyInput and NodeRow with memo to prevent unnecessary re-renders - Set reasonable React Query cache/stale defaults (was 0/0) - Reduce Connect search debounce from 1000ms to 300ms - Cache figma.listAvailableFontsAsync() per formatText call - Optimize getConflictingNodes from O(n²) to O(n) using Map - Optimize getPushChanges screenshot lookup from O(n×m×k) to O(n) - Eliminate redundant node traversal in getScreenshots - Combine 6 parent traversals into single getAllParents() in preformatKey - Fix stale closures in Dialog, Popover, and ResizeHandle - Extract inline styles in NodeList virtualization - Disable virtualization in Push Changes view to fix blank space issue - Precompute toLowerCase/highlight data in AutocompleteSelect - Remove layout thrashing from AutocompleteSelect useLayoutEffect Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a unified NodeParents API, caches available fonts, switches several linear scans to map/group lookups, applies memoization/useCallback/useMemo and debounce optimizations across UI, upgrades lint/format tooling and TypeScript config, and includes many stylistic trailing-comma edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ui/components/Dialog/Dialog.tsx (1)
13-22:⚠️ Potential issue | 🟠 MajorAdding
[onClose]fixes the stale closure but introduces a performance regression.The caller in
Router.tsxpasses an inline arrow function (onClose={() => setRoute("index")}), which creates a new reference on every render. With[onClose]in the dependency array, this causes the effect to re-run on every parent render—re-focusing the element and re-attaching the keydown listener repeatedly.Two options to fix:
Option A (preferred): Use a ref to always call the latest
onClosewithout re-running the effect:Proposed fix
-import { useEffect, useRef } from "preact/hooks"; +import { useEffect, useRef, useCallback } from "preact/hooks"; import styles from "./Dialog.css"; type Props = { onClose: () => void; children: ComponentChildren; }; export const Dialog = ({ children, onClose }: Props) => { const ref = useRef<HTMLDivElement>(null); + const onCloseRef = useRef(onClose); + onCloseRef.current = onClose; useEffect(() => { const handler = (e: KeyboardEvent) => { if (e.code === "Escape") { - onClose(); + onCloseRef.current(); } }; ref.current?.focus(); ref.current?.addEventListener("keydown", handler); return () => ref.current?.removeEventListener("keydown", handler); - }, [onClose]); + }, []);Option B: Wrap
onClosewithuseCallbackinRouter.tsx:const handleDialogClose = useCallback(() => setRoute("index"), [setRoute]); // ... <Dialog onClose={handleDialogClose}>{dialogPage}</Dialog>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/Dialog/Dialog.tsx` around lines 13 - 22, The effect re-runs on every render because the inline onClose changes; instead keep the effect stable by using a mutable ref for the latest onClose and reading that inside the keydown handler: create a latestOnCloseRef and update latestOnCloseRef.current = onClose in a small effect that runs on every onClose change, then in the existing useEffect that sets focus and attaches handler (leave its dependency array empty) have the handler call latestOnCloseRef.current() when Escape is pressed; update references to ref, handler, and onClose accordingly in the Dialog component so the listener and focus are not re-attached on every parent render.src/ui/views/StringDetails/StringDetails.tsx (1)
439-457:⚠️ Potential issue | 🔴 Critical
TextboxNumericdoes not accept anonChangeprop in v3.2.0.
TextboxNumericfrom@create-figma-plugin/ui@3.2.0does not have anonChangeprop (it was removed in the v3 breaking changes). UseonValueInputinstead, which receives(newValue: string) => void:🐛 Proposed fix
<TextboxNumeric data-cy="string_details_plural_paramter_value" placeholder={tolgeeValue.parameter} value={currentNode.pluralParamValue ?? ""} - onChange={({ currentTarget }) => { + onValueInput={(newValue) => { setNeedsSubmission(true); - if (currentTarget.value !== "") { - currentNode.pluralParamValue = currentTarget.value; + if (newValue !== "") { + currentNode.pluralParamValue = newValue; const newNode: NodeInfo = { ...currentNode, - pluralParamValue: currentTarget.value, + pluralParamValue: newValue, }; if (translation != null) { setCurrentNode(newNode); } } }} variant="border" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/StringDetails/StringDetails.tsx` around lines 439 - 457, TextboxNumeric's onChange handler is invalid in v3.2.0; replace the onChange usage on the TextboxNumeric component with onValueInput: change the handler on TextboxNumeric (where currentNode.pluralParamValue is read/updated and setNeedsSubmission, setCurrentNode, translation, and tolgeeValue.parameter are referenced) to use onValueInput: accept the newValue: string parameter, call setNeedsSubmission(true), if newValue !== "" update currentNode.pluralParamValue and create newNode with pluralParamValue: newValue, then if translation != null call setCurrentNode(newNode); keep the placeholder and variant props unchanged.src/ui/components/Popover/Popover.tsx (1)
63-75:⚠️ Potential issue | 🟡 MinorCapture the trigger element before subscribing.
This effect subscribes to one DOM node but unsubscribes from whatever
popoverTrigger.currentpoints to at cleanup time. While the ref is unlikely to be reassigned in the current code, capturing the element ensures add/remove are paired to the same node, making the effect resilient to future changes. This is also a common React pattern for event listener setup.Suggested change
useEffect(() => { - if (!popoverTrigger.current || !dropdownRef.current) { + const trigger = popoverTrigger.current; + if (!trigger || !dropdownRef.current) { return; } - popoverTrigger.current.addEventListener("click", open); + trigger.addEventListener("click", open); window.addEventListener("resize", computePositionAndSet); return () => { - popoverTrigger.current?.removeEventListener("click", open); + trigger.removeEventListener("click", open); window.removeEventListener("resize", computePositionAndSet); }; }, [popoverTrigger, open, computePositionAndSet]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/Popover/Popover.tsx` around lines 63 - 75, The effect should capture the trigger element before subscribing so add/remove use the same node: inside the useEffect that references popoverTrigger, dropdownRef, open, computePositionAndSet, read const trigger = popoverTrigger.current (and return early if null) then call trigger.addEventListener("click", open) and keep window.addEventListener("resize", computePositionAndSet); in the cleanup remove the listener from that captured trigger (trigger.removeEventListener("click", open)) and remove the window resize listener, ensuring add/remove are paired to the same DOM element even if popoverTrigger.current changes.
🧹 Nitpick comments (4)
src/ui/components/ResizeHandle/ResizeHandle.tsx (1)
25-31: Consider memoizinghandleMouseDownfor consistency.While
handleMouseDownis used as a JSX event handler (not a document listener) and doesn't suffer from the same stale closure problem, wrapping it inuseCallbackwould maintain consistency with the other handlers and prevent unnecessary re-renders of child components if this component is refactored later.♻️ Optional refactor for consistency
- const handleMouseDown = (e: MouseEvent) => { + const handleMouseDown = useCallback((e: MouseEvent) => { e.preventDefault(); setIsResizing(true); setStartPos({ x: e.clientX, y: e.clientY }); const currentSize = sizeStack[sizeStack.length - 1] || DEFAULT_SIZE; setStartSize(currentSize); - }; + }, [sizeStack]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/ResizeHandle/ResizeHandle.tsx` around lines 25 - 31, The handleMouseDown function should be memoized with React.useCallback to match other handlers and avoid unnecessary re-renders; wrap the existing handleMouseDown implementation in useCallback (referencing handleMouseDown, setIsResizing, setStartPos, setStartSize, sizeStack, DEFAULT_SIZE) and include the correct dependency array (dependencies: sizeStack and the state setters if not stable) so its identity remains stable across renders.src/tools/getPushChanges.ts (1)
35-51: Good algorithmic optimization from O(n×m×k) to O(n×m + k).The Map-based precomputation for screenshot lookups is a solid improvement. The use of
\0as a separator ensures unambiguous composite keys even for edge cases likekey="ab", ns="c"vskey="a", ns="bc".Potential duplicate screenshots per key: If a single
FrameScreenshotcontains multiple nodes with identicalkeyandnsvalues, the same screenshot will be added to the list multiple times. This causes downstream processing to create duplicateKeyScreenshotDtoentries with identicaluploadedImageIdand positions, wasting bandwidth and potentially causing redundant operations on the server.Prevent duplicates by tracking seen keys during insertion:
♻️ Optional fix to prevent duplicate screenshot entries
const screenshotsByKey = new Map<string, FrameScreenshot[]>(); screenshots.forEach((screenshot) => { + const seenKeys = new Set<string>(); screenshot.keys.forEach((node) => { const mapKey = `${node.key}\0${hasNamespacesEnabled ? (node.ns || "") : ""}`; + if (seenKeys.has(mapKey)) return; + seenKeys.add(mapKey); let list = screenshotsByKey.get(mapKey); if (!list) { list = []; screenshotsByKey.set(mapKey, list); } list.push(screenshot); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/getPushChanges.ts` around lines 35 - 51, The current population of screenshotsByKey can add the same FrameScreenshot multiple times if a FrameScreenshot's screenshot.keys contains duplicate nodes with identical key/ns; inside the screenshots.forEach loop (where screenshot.keys.forEach is used) add a per-screenshot Set (e.g., seenMapKeys) and compute the same mapKey (`${node.key}\0${hasNamespacesEnabled ? (node.ns || "") : ""}`) — only push the screenshot into screenshotsByKey for that mapKey if it hasn't been seen in seenMapKeys yet; keep the existing mapKey construction and getKeyScreenshots function unchanged otherwise.src/ui/views/StringDetails/StringDetails.tsx (1)
446-446: Remove direct mutation of state object.Line 446 directly mutates
currentNode.pluralParamValuebefore creating a new object on line 447-450. This is redundant since the spread creates a new object with the updated value anyway, and direct mutation of state/props is an anti-pattern that can cause subtle bugs.♻️ Remove redundant mutation
onChange={({ currentTarget }) => { setNeedsSubmission(true); if (currentTarget.value !== "") { - currentNode.pluralParamValue = currentTarget.value; const newNode: NodeInfo = { ...currentNode, pluralParamValue: currentTarget.value, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/StringDetails/StringDetails.tsx` at line 446, Remove the direct mutation of currentNode.pluralParamValue; do not assign to currentNode before creating the new object—delete the statement that sets currentNode.pluralParamValue = currentTarget.value and rely on the subsequent spread/update that builds a new node object (the code that spreads currentNode and sets pluralParamValue to currentTarget.value) so state is updated immutably within the StringDetails handler.src/ui/views/Push/Changes.tsx (1)
27-30: Avoid usingInfinityas the virtualization off-switch.This relies on
NodeList's currentitems.length <= minVisibleRowsimplementation and still feedsInfinityinto the height calculation logic. An explicitvirtualized={false}path inNodeListwould be clearer and less brittle.Also applies to: 51-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/Push/Changes.tsx` around lines 27 - 30, The code currently disables virtualization by passing minVisibleRows={Infinity} to NodeList (e.g., the NodeList instance using rowHeight and items={changes.newKeys.map(...)}) which is brittle; update NodeList usage to explicitly opt out of virtualization by adding virtualized={false} and remove or stop passing minVisibleRows={Infinity} (use a sensible number or omit it) and apply the same change to the other NodeList instance later in the file that uses the same Infinity pattern so both places explicitly disable virtualization instead of relying on Infinity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx`:
- Around line 131-137: The highlightedOptions mapping currently only carries raw
opt, matchIndex and isExact so rendering always falls back to the raw string
(bypassing displayValue and the "<none>" fallback); update the mapping in the
AutocompleteSelect component (and the similar block around the other render
path) to include the computed displayValue (e.g., compute displayValue =
getDisplayValue(opt) or the existing displayValue logic, treating empty strings
as "<none>") and use that displayValue when rendering options and when computing
matchIndex/isExact (or allow matchIndex = -1 if appropriate) so custom labels
and the empty-string fallback are applied in the main results list.
- Around line 101-119: filteredOptions and the highlight logic should use the
same normalized, trimmed query as exactMatch: replace uses of lowerInputValue
with lowerInputTrimmed inside the options.filter call in the filteredOptions
useMemo (and update its dependency list accordingly) so filtering matches
trimmed input and multi-select exclusion (isMultiSelect / multiValues) remains;
also update the highlight logic to use lowerInputTrimmed for matching and use
trimmedInputValue.length for the bolded range calculation so highlights align
with the filtered/exact-match behavior.
In `@src/ui/components/NamespaceSelect/NamespaceSelect.tsx`:
- Around line 25-37: The memoized allNamespaces currently uses .filter(Boolean)
which removes the empty-string namespace ("") — update the useMemo for
allNamespaces (the useMemo block that builds the Set from namespaces and value)
to preserve "" by removing the .filter(Boolean) call (or replace it with a
filter that only removes null/undefined if you prefer), leaving the .sort(...)
as-is since it already handles empty strings correctly so users can switch back
to the default/<none> namespace.
In `@src/ui/components/NodeList/NodeList.tsx`:
- Around line 115-128: The useMemo hooks for spacerStyle and translateStyle
(which depend on totalHeight and offsetY) are being called only on the
virtualized path, violating hook rules when items.length crosses minVisibleRows;
move the useMemo calls for spacerStyle and translateStyle to always run on every
render (i.e., before the early return that handles short lists in the NodeList
component) or replace them with unconditional computed values that use the same
dependencies (totalHeight, offsetY) so the hooks count never changes between
renders.
In `@src/ui/Plugin.tsx`:
- Around line 18-19: The useSelectedNodes hook returns stale cached data on
remount because it inherits global query defaults (staleTime: 30s, cacheTime:
5m); update the hook (useSelectedNodes in src/ui/hooks/useSelectedNodes.ts) to
pass explicit query options staleTime: 0 and cacheTime: 0 when creating its
React Query (useQuery/useSuspenseQuery) so the hook always refetches on mount,
matching other ephemeral hooks like PageSetup/CreateCopy/ProjectSettings.
In `@src/ui/views/Settings/ProjectSettings.tsx`:
- Around line 125-128: The computed namespaces array (const namespaces =
useMemo(...)) currently falls back to ["" ] only when the source is undefined,
so an actual empty array from namespacesLoadable.data._embedded.namespaces
yields [] and removes the default option; update the useMemo logic (the
namespaces computation) to check the mapped result's length and return [""] when
the mapped array is empty (e.g., compute const result =
namespacesLoadable.data?._embedded?.namespaces?.map(...) ?? []; return
result.length ? result : ["" ]), ensuring the default empty namespace is
preserved.
---
Outside diff comments:
In `@src/ui/components/Dialog/Dialog.tsx`:
- Around line 13-22: The effect re-runs on every render because the inline
onClose changes; instead keep the effect stable by using a mutable ref for the
latest onClose and reading that inside the keydown handler: create a
latestOnCloseRef and update latestOnCloseRef.current = onClose in a small effect
that runs on every onClose change, then in the existing useEffect that sets
focus and attaches handler (leave its dependency array empty) have the handler
call latestOnCloseRef.current() when Escape is pressed; update references to
ref, handler, and onClose accordingly in the Dialog component so the listener
and focus are not re-attached on every parent render.
In `@src/ui/components/Popover/Popover.tsx`:
- Around line 63-75: The effect should capture the trigger element before
subscribing so add/remove use the same node: inside the useEffect that
references popoverTrigger, dropdownRef, open, computePositionAndSet, read const
trigger = popoverTrigger.current (and return early if null) then call
trigger.addEventListener("click", open) and keep
window.addEventListener("resize", computePositionAndSet); in the cleanup remove
the listener from that captured trigger (trigger.removeEventListener("click",
open)) and remove the window resize listener, ensuring add/remove are paired to
the same DOM element even if popoverTrigger.current changes.
In `@src/ui/views/StringDetails/StringDetails.tsx`:
- Around line 439-457: TextboxNumeric's onChange handler is invalid in v3.2.0;
replace the onChange usage on the TextboxNumeric component with onValueInput:
change the handler on TextboxNumeric (where currentNode.pluralParamValue is
read/updated and setNeedsSubmission, setCurrentNode, translation, and
tolgeeValue.parameter are referenced) to use onValueInput: accept the newValue:
string parameter, call setNeedsSubmission(true), if newValue !== "" update
currentNode.pluralParamValue and create newNode with pluralParamValue: newValue,
then if translation != null call setCurrentNode(newNode); keep the placeholder
and variant props unchanged.
---
Nitpick comments:
In `@src/tools/getPushChanges.ts`:
- Around line 35-51: The current population of screenshotsByKey can add the same
FrameScreenshot multiple times if a FrameScreenshot's screenshot.keys contains
duplicate nodes with identical key/ns; inside the screenshots.forEach loop
(where screenshot.keys.forEach is used) add a per-screenshot Set (e.g.,
seenMapKeys) and compute the same mapKey (`${node.key}\0${hasNamespacesEnabled ?
(node.ns || "") : ""}`) — only push the screenshot into screenshotsByKey for
that mapKey if it hasn't been seen in seenMapKeys yet; keep the existing mapKey
construction and getKeyScreenshots function unchanged otherwise.
In `@src/ui/components/ResizeHandle/ResizeHandle.tsx`:
- Around line 25-31: The handleMouseDown function should be memoized with
React.useCallback to match other handlers and avoid unnecessary re-renders; wrap
the existing handleMouseDown implementation in useCallback (referencing
handleMouseDown, setIsResizing, setStartPos, setStartSize, sizeStack,
DEFAULT_SIZE) and include the correct dependency array (dependencies: sizeStack
and the state setters if not stable) so its identity remains stable across
renders.
In `@src/ui/views/Push/Changes.tsx`:
- Around line 27-30: The code currently disables virtualization by passing
minVisibleRows={Infinity} to NodeList (e.g., the NodeList instance using
rowHeight and items={changes.newKeys.map(...)}) which is brittle; update
NodeList usage to explicitly opt out of virtualization by adding
virtualized={false} and remove or stop passing minVisibleRows={Infinity} (use a
sensible number or omit it) and apply the same change to the other NodeList
instance later in the file that uses the same Infinity pattern so both places
explicitly disable virtualization instead of relying on Infinity.
In `@src/ui/views/StringDetails/StringDetails.tsx`:
- Line 446: Remove the direct mutation of currentNode.pluralParamValue; do not
assign to currentNode before creating the new object—delete the statement that
sets currentNode.pluralParamValue = currentTarget.value and rely on the
subsequent spread/update that builds a new node object (the code that spreads
currentNode and sets pluralParamValue to currentTarget.value) so state is
updated immutably within the StringDetails handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9610d8d5-2338-457b-9170-6b217e9a966b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
package.jsonsrc/main/endpoints/formatText.tssrc/main/endpoints/getScreenshots.tssrc/main/endpoints/preformatKey.tssrc/main/utils/nodeParents.tssrc/tools/getConflictingNodes.tssrc/tools/getPushChanges.tssrc/ui/Plugin.tsxsrc/ui/components/AutocompleteSelect/AutocompleteSelect.tsxsrc/ui/components/Dialog/Dialog.tsxsrc/ui/components/NamespaceSelect/NamespaceSelect.tsxsrc/ui/components/NodeList/NodeList.tsxsrc/ui/components/NodeList/NodeRow.tsxsrc/ui/components/Popover/Popover.tsxsrc/ui/components/ResizeHandle/ResizeHandle.tsxsrc/ui/hooks/useSelectedNodes.tssrc/ui/views/Connect/Connect.tsxsrc/ui/views/Index/Index.tsxsrc/ui/views/Index/KeyInput.tsxsrc/ui/views/Index/ListItem.tsxsrc/ui/views/Push/Changes.tsxsrc/ui/views/Settings/ProjectSettings.tsxsrc/ui/views/Settings/PushSection.tsxsrc/ui/views/Settings/StringsEditor.tsxsrc/ui/views/StringDetails/StringDetails.tsxtsconfig.json
💤 Files with no reviewable changes (2)
- src/ui/views/Settings/StringsEditor.tsx
- src/ui/views/Settings/PushSection.tsx
- Fix hooks rule violation in NodeList (useMemo after early return) - Preserve empty namespace option in NamespaceSelect (remove .filter(Boolean)) - Add staleTime/cacheTime: 0 to useSelectedNodes for fresh data on remount - Use consistent trimmed input for filtering and matching in AutocompleteSelect - Use displayValue labels in highlight rendering path in AutocompleteSelect - Handle empty namespace array fallback in ProjectSettings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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 (1)
src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx (1)
81-92:⚠️ Potential issue | 🟠 MajorReposition the dropdown when its height changes.
With Line 92 depending only on
showDropdown, the placement is locked to the first render. In single-select, focusing a prefilled value opens a short menu; clearing the query can then expand it without flipping upward near the bottom of the viewport, so part of the list ends up off-screen. Re-run this measurement when the dropdown resizes instead of only when it opens.💡 Suggested fix
useLayoutEffect(() => { - if (!showDropdown || !inputRef.current) return; - const inputRect = inputRef.current.getBoundingClientRect(); - const dropdownHeight = dropdownRef.current?.offsetHeight || 180; - const spaceBelow = window.innerHeight - inputRect.bottom - 49; // padding of actionbar - const spaceAbove = inputRect.top; - if (spaceBelow < dropdownHeight && spaceAbove > dropdownHeight) { - setDropdownDirection("up"); - } else { - setDropdownDirection("down"); - } + if (!showDropdown || !inputRef.current || !dropdownRef.current) return; + + const updateDirection = () => { + const inputRect = inputRef.current!.getBoundingClientRect(); + const dropdownHeight = dropdownRef.current!.offsetHeight || 180; + const spaceBelow = window.innerHeight - inputRect.bottom - 49; // padding of actionbar + const spaceAbove = inputRect.top; + + setDropdownDirection( + spaceBelow < dropdownHeight && spaceAbove > dropdownHeight ? "up" : "down" + ); + }; + + updateDirection(); + + const observer = new ResizeObserver(updateDirection); + observer.observe(dropdownRef.current); + + return () => observer.disconnect(); }, [showDropdown]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx` around lines 81 - 92, The placement effect for the dropdown (useLayoutEffect) only runs when showDropdown changes so it never re-measures on height changes; update it to re-run when the dropdown resizes by observing dropdownRef.current with a ResizeObserver (or by attaching a resize listener) while the menu is open and recomputing the measurement using inputRef.getBoundingClientRect() and dropdownRef.current.offsetHeight, then calling setDropdownDirection("up"|"down") accordingly; ensure you create and disconnect the ResizeObserver in the effect cleanup to avoid leaks and keep the existing logic in useLayoutEffect that checks spaceBelow and spaceAbove.
🧹 Nitpick comments (1)
src/ui/hooks/useSelectedNodes.ts (1)
15-18: Add explicit initial value touseReffor Preact 11 compatibility.Line 15 calls
useRef<ReturnType<typeof setTimeout>>()without an initial value. In Preact 11,useRefrequires an initial value (the no-argument overload was removed). The explicitundefinedinitialization also clarifies the timer lifecycle.Proposed change
- const refetchTimerRef = useRef<ReturnType<typeof setTimeout>>(); + const refetchTimerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/hooks/useSelectedNodes.ts` around lines 15 - 18, The useRef call for refetchTimerRef must include an explicit initial value for Preact 11 compatibility; update the declaration of refetchTimerRef (used by debouncedRefetch and cleared via clearTimeout) to provide an initial undefined (e.g., useRef<ReturnType<typeof setTimeout> | undefined>(undefined)) so the ref type and lifecycle are explicit and the no-argument overload is not used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx`:
- Around line 107-116: filteredOptions currently excludes selected values using
raw multiValues.includes(...) which fails when casing differs; normalize
selected values the same way option lookup is normalized (e.g.,
lowercased/trimmed) before comparisons. Update the filteredOptions computation
to create a normalized set/array (from multiValues via v =>
v.toLowerCase().trim()) and use that for filtering (replace multiValues.includes
with normalizedSet.has/ includes), and apply the same normalization when
performing duplicate checks anywhere else (e.g., selection handler(s) that check
multiValues for duplicates) so comparisons are consistent with option matching.
---
Outside diff comments:
In `@src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx`:
- Around line 81-92: The placement effect for the dropdown (useLayoutEffect)
only runs when showDropdown changes so it never re-measures on height changes;
update it to re-run when the dropdown resizes by observing dropdownRef.current
with a ResizeObserver (or by attaching a resize listener) while the menu is open
and recomputing the measurement using inputRef.getBoundingClientRect() and
dropdownRef.current.offsetHeight, then calling setDropdownDirection("up"|"down")
accordingly; ensure you create and disconnect the ResizeObserver in the effect
cleanup to avoid leaks and keep the existing logic in useLayoutEffect that
checks spaceBelow and spaceAbove.
---
Nitpick comments:
In `@src/ui/hooks/useSelectedNodes.ts`:
- Around line 15-18: The useRef call for refetchTimerRef must include an
explicit initial value for Preact 11 compatibility; update the declaration of
refetchTimerRef (used by debouncedRefetch and cleared via clearTimeout) to
provide an initial undefined (e.g., useRef<ReturnType<typeof setTimeout> |
undefined>(undefined)) so the ref type and lifecycle are explicit and the
no-argument overload is not used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fefe08d4-875d-4451-90a3-ed5332efa02b
📒 Files selected for processing (5)
src/ui/components/AutocompleteSelect/AutocompleteSelect.tsxsrc/ui/components/NamespaceSelect/NamespaceSelect.tsxsrc/ui/components/NodeList/NodeList.tsxsrc/ui/hooks/useSelectedNodes.tssrc/ui/views/Settings/ProjectSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/components/NodeList/NodeList.tsx
- src/ui/views/Settings/ProjectSettings.tsx
- src/ui/components/NamespaceSelect/NamespaceSelect.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/ui/hooks/useInterpolatedTranslation.ts (1)
149-173:⚠️ Potential issue | 🟠 MajorPublish the recomputed interpolation through state here.
This effect now only mutates
interpolatedTranslation.current, so the hook never rerenders after the recompute. That leavestranslationDiffersFromNodestale insrc/ui/components/NodeList/NodeRow.tsx:21-58, andplaceholders/previewTextcan stay empty or outdated insrc/ui/views/StringDetails/StringDetails.tsx:196-204. The effect also closes overparamsValues,node?.pluralParamValue,config?.language, andselectedPluralVariant, but those inputs are missing from the dependency list, so switching to another node with the same raw text can keep the previous interpolation.💡 Possible fix
useEffect(() => { - const newCharacters = getOrUpdateInterpolatedTranslation({ - paramsValues: paramsValues || {}, - currentTranslation: newTranslation, - })?.replace(/<[^>]*>/g, ""); + const newCharacters = getOrUpdateInterpolatedTranslation( + { + paramsValues: paramsValues || {}, + pluralParamValue: node?.pluralParamValue, + currentTranslation: newTranslation, + }, + true, + )?.replace(/<[^>]*>/g, ""); if (newCharacters != null && newCharacters !== previousCharacters) { interpolatedTranslation.current = newCharacters; } - }, [rawTranslation, node?.isPlural]); + }, [ + newTranslation, + previousCharacters, + paramsValues, + node?.isPlural, + node?.pluralParamValue, + config?.language, + selectedPluralVariant, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/hooks/useInterpolatedTranslation.ts` around lines 149 - 173, The effect that recomputes interpolation currently only mutates interpolatedTranslation.current so the hook never re-renders and downstream values (translationDiffersFromNode, placeholders, previewText) stay stale; change the effect in useInterpolatedTranslation to set a state value (e.g., setInterpolatedTranslationState) with the cleaned string returned by getOrUpdateInterpolatedTranslation instead of only writing to interpolatedTranslation.current, and include all missing dependencies (paramsValues, node?.pluralParamValue, config?.language, selectedPluralVariant, rawTranslation, node?.isPlural) in the effect dependency list so switching nodes or plural variants triggers recomputation; update translationDiffersFromNode to read from the new state value rather than interpolatedTranslation.current.src/ui/views/Settings/ProjectSettings.tsx (1)
193-236:⚠️ Potential issue | 🟠 MajorDrive this branch from the same credentials as the namespace query, and clear hidden state.
namespacesLoadableis fetched with the propapiKey/apiUrl, butuseHasNamespacesEnabled()insrc/ui/hooks/useHasNamespacesEnabled.ts:1-29reads global client state instead. During project switches or before global state catches up, this can hideNamespaceSelectfor the wrong project. When that happens,settings.namespaceis never reset, soonChangekeeps emitting a namespace the user can no longer clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/Settings/ProjectSettings.tsx` around lines 193 - 236, use the same per-project credentials when checking namespace availability and clear any stored namespace when namespaces become disabled: update useHasNamespacesEnabled to accept apiKey/apiUrl (or add a per-project boolean derived from namespacesLoadable keyed by apiKey/apiUrl) so the UI decision uses the same credentials as namespacesLoadable, and in ProjectSettings.tsx watch that per-project flag (the renamed/extended hook or derived value) and call setSettings(s => ({ ...s!, namespace: "" })) when namespaces become disabled so settings.namespace is reset and the NamespaceSelect state can’t persist an invalid value; refer to useHasNamespacesEnabled, namespacesLoadable, NamespaceSelect, settings.namespace, apiKey/apiUrl and setSettings to locate where to change the logic.src/ui/client/useQueryApi.ts (1)
40-47:⚠️ Potential issue | 🟠 MajorInclude config identity (apiUrl/apiKey) in the query cache key.
Line 41 keys queries by
[url, path, query]only, but the fetcher depends onconfig.apiUrland the project derived fromconfig.apiKey(line 44-45). With the non-zero React Query defaults (staleTime: 30s,cacheTime: 5min), switching API keys or servers can reuse cached data from a previous workspace until the cache expires. Addconfig.apiUrlandconfig.apiKeyto the query key at line 41, or add automatic cache invalidation in GlobalState when CONFIG_CHANGE is received (currently only Settings.tsx clears the cache, and CONFIG_CHANGE events from the main process bypass this).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/client/useQueryApi.ts` around lines 40 - 47, The query key used in useQuery (in useQueryApi) only includes [url, path, query] but the fetcher also depends on config.apiUrl and config.apiKey; update the query key in useQueryApi to include config.apiUrl and config.apiKey (or otherwise derive a stable identity from config) so that swapping API keys or servers won't return stale cached results; locate the useQuery call inside useQueryApi and add config.apiUrl and config.apiKey to its key array (or trigger cache invalidation on CONFIG_CHANGE if you prefer central invalidation in GlobalState) so the cache is correctly scoped per config.src/ui/views/Pull/Pull.tsx (1)
68-70:⚠️ Potential issue | 🟠 MajorMissing
hasNamespacesEnabledin dependency array causes stale diff.The
useEffecttriggerscomputeDiff()whenselectedNodes.dataorlangchanges, buthasNamespacesEnabledis not included. SinceuseHasNamespacesEnabledreturnsfalsewhile queries are pending (per the hook at lines 1-29 inuseHasNamespacesEnabled.ts), the initial diff computation may bypass namespace-aware lookup. When the query resolves andhasNamespacesEnabledbecomestrue, the diff won't recompute.🐛 Proposed fix
useEffect(() => { computeDiff(); - }, [selectedNodes.data, lang]); + }, [selectedNodes.data, lang, hasNamespacesEnabled]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/Pull/Pull.tsx` around lines 68 - 70, The effect that calls computeDiff() currently depends on selectedNodes.data and lang but misses hasNamespacesEnabled, causing a stale diff when useHasNamespacesEnabled (from useHasNamespacesEnabled.ts) flips from false to true; update the dependency array for the useEffect that calls computeDiff() (the hook referencing computeDiff, selectedNodes.data, and lang) to also include hasNamespacesEnabled so computeDiff re-runs when namespace support status changes.
🧹 Nitpick comments (7)
src/createFormatIcu.ts (1)
41-43: Formatting change looks good; consider.some()for a minor semantic/performance improvement.The trailing comma addition is fine. Since this PR focuses on performance, note that
.some()would be more appropriate here than.find()— it short-circuits on the first truthy result and directly returns a boolean, avoiding the extra coercion in the!expression.♻️ Optional: use `.some()` instead of `.find()`
- const ignoreTag = !Object.values(params || {}).find( - (p) => typeof p === "function", - ); + const ignoreTag = !Object.values(params || {}).some( + (p) => typeof p === "function", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/createFormatIcu.ts` around lines 41 - 43, Replace the use of Object.values(...).find(...) when computing ignoreTag with Object.values(...).some(...) so the check short-circuits and directly returns a boolean; specifically update the ignoreTag assignment in src/createFormatIcu.ts (the ignoreTag variable) to use .some instead of .find and remove the extra boolean coercion (keep the negation as needed) to improve performance and clarity.src/ui/views/CreateCopy/CreateCopy.tsx (1)
129-141: Good UI improvement; consider extracting the inline style for consistency.Wrapping the languages list in a scrollable container is a solid fix to prevent layout overflow. However, the inline style object
{ maxHeight: 200, overflowY: "auto" }creates a new reference on each render. Given the PR's goal of extracting inline styles to avoid per-render allocations (as done in NodeList), consider hoisting this to a constant outside the component.♻️ Suggested refactor
+const languageListStyle = { maxHeight: 200, overflowY: "auto" } as const; + export const CreateCopy: FunctionComponent = () => {Then use it:
- <div style={{ maxHeight: 200, overflowY: "auto" }}> + <div style={languageListStyle}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/CreateCopy/CreateCopy.tsx` around lines 129 - 141, Extract the inline style object { maxHeight: 200, overflowY: "auto" } used on the languages container into a top-level constant (e.g., LANGUAGES_LIST_STYLE) declared outside the CreateCopy component, preserving the same properties and types; then replace the inline style on the div that renders languages?.map(...) with style={LANGUAGES_LIST_STYLE} so the map/Checkbox/handleToggleLanguage rendering uses the hoisted constant instead of reallocating the object each render.src/ui/client/client.ts (1)
38-44: Avoid rebuilding the accumulator inflattenParams.Line 38 creates a new object for every entry, and nested params add another spread on top. That makes this helper allocation-heavy on the request path, which works against the PR’s performance goal. A mutable local accumulator keeps the flattening work linear.
♻️ Suggested refactor
const flattenParams = ( params: Params | null | undefined, ): Record<string, string | string[]> => { - if (params) { - return Object.entries(params).reduce( - (acc, [key, value]) => - Array.isArray(value) || typeof value !== "object" - ? { ...acc, [key]: value } - : { ...acc, ...flattenParams(value) }, - {}, - ); - } - return {}; + const result: Record<string, string | string[]> = {}; + if (!params) { + return result; + } + + for (const [key, value] of Object.entries(params)) { + if (Array.isArray(value) || typeof value !== "object") { + if (value != null) { + result[key] = value; + } + } else { + Object.assign(result, flattenParams(value)); + } + } + + return result; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/client/client.ts` around lines 38 - 44, flattenParams currently rebuilds the accumulator on every entry (using object spreads in the reduce), causing heavy allocations; change flattenParams to use a single mutable accumulator object instead of spreading: iterate Object.entries(params) (or switch to a for loop), and for each [key,value] assign primitive/array values directly (acc[key] = value), and for nested objects call flattenParams(value) and copy its properties into the same acc (e.g., loop over the returned object's keys and assign acc[nestedKey] = nestedValue or use Object.assign) before returning acc; keep the function name flattenParams and the accumulator variable (acc) so the change is local and preserves behavior.src/ui/client/useQueryApi.ts (1)
88-111: AddinvalidatePrefixandqueryClientto dependency arrays to prevent stale closures.The
mutateandmutateAsynccallbacks depend only onmutation.mutate*, but their implementations callcustomOptions()which closes overinvalidatePrefixandqueryClient. If these props change after the first render, the callbacks will use stale values. While the ESLint rule is disabled in this project, wrappingcustomOptionsinuseCallbackwith the appropriate dependencies improves maintainability:🔧 Suggested fix
- const customOptions = (options: MutationOptions<any, any, any, any>) => ({ - ...options, - onSuccess: (...args: any) => { - if (invalidatePrefix !== undefined) { - invalidateUrlPrefix(queryClient, invalidatePrefix); - } - // `@ts-ignore` - options?.onSuccess?.(...args); - }, - }); + const customOptions = useCallback( + (options: MutationOptions<any, any, any, any>) => ({ + ...options, + onSuccess: (...args: any) => { + if (invalidatePrefix !== undefined) { + invalidateUrlPrefix(queryClient, invalidatePrefix); + } + // `@ts-ignore` + options?.onSuccess?.(...args); + }, + }), + [invalidatePrefix, queryClient], + ); const mutate = useCallback<typeof mutation.mutate>( (variables, options) => { return mutation.mutate(variables, customOptions(options as any)); }, - [mutation.mutate], + [customOptions, mutation.mutate], ); const mutateAsync = useCallback<typeof mutation.mutateAsync>( (variables, options) => { return mutation.mutateAsync(variables, customOptions(options as any)); }, - [mutation.mutateAsync], + [customOptions, mutation.mutateAsync], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/client/useQueryApi.ts` around lines 88 - 111, The mutate and mutateAsync callbacks close over customOptions which itself uses queryClient and invalidatePrefix, causing stale closures; wrap customOptions in useCallback with dependencies [queryClient, invalidatePrefix, options?.onSuccess] (or at least [queryClient, invalidatePrefix]) and include queryClient and invalidatePrefix in the dependency arrays for the useCallback calls that create mutate and mutateAsync (i.e., the arrays for mutate and mutateAsync should include mutation.mutate/mutateAsync plus queryClient and invalidatePrefix) so the callbacks always use current values of queryClient and invalidatePrefix.src/ui/components/NodeList/NodeRow.tsx (2)
34-37: Inline style objects defeat memo optimization.The
styleobject at lines 34-37 is created fresh each render, producing a new reference that bypassesmemo(). Consider extracting to a constant or usinguseMemo.♻️ Example fix using useMemo
+ import { useMemo } from "preact/hooks"; export const NodeRow = memo( ({ node, action, keyComponent, nsComponent, showNamespace = true, compact, onClick }: Props) => { const showText = node.characters || !compact || action; + const containerStyle = useMemo( + () => ({ + gridTemplateColumns: action ? "1fr 1fr auto" : "1fr 1fr", + cursor: onClick ? "pointer" : "default", + }), + [action, onClick] + ); // ... in JSX: - style={{ - gridTemplateColumns: action ? "1fr 1fr auto" : "1fr 1fr", - cursor: onClick ? "pointer" : "default", - }} + style={containerStyle}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/NodeList/NodeRow.tsx` around lines 34 - 37, The inline style object in the NodeRow component is recreated every render (gridTemplateColumns based on prop action and cursor based on onClick), which breaks React.memo; update NodeRow to memoize or hoist the style so its reference is stable — e.g., use useMemo inside NodeRow to compute the style object from action and onClick (or create two static style constants and pick between them) and then pass that memoized/hoisted object to the element instead of constructing it inline.
21-22:memo()is partially ineffective due to inline callbacks in parent.Per the context snippet from
NodeList.tsx(lines 70-87),NodeRowreceives:
onClickas an inline arrow() => onClick?.(item)action,keyComponent,nsComponentfrom callbacks invoked each renderThese create new references every render, causing
NodeRowto always re-render despitememo(). Consider memoizing these inNodeListusinguseCallbackor restructuring to pass stable references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/NodeList/NodeRow.tsx` around lines 21 - 22, NodeRow is being re-rendered every time because parent creates new function/element references for props action, keyComponent, nsComponent and the inline onClick (() => onClick?.(item)); in the parent component (NodeList) wrap those callbacks in useCallback (and any derived components in useMemo) so they are stable across renders, replace the inline onClick arrow with a stable handler (e.g., const handleClick = useCallback(() => onClick?.(item), [onClick, itemId]) or bind using item id) and pass the memoized action/keyComponent/nsComponent to NodeRow to make React.memo effective.src/main/endpoints/preformatKey.ts (1)
34-42: Low-risk ReDoS from user-controlled key format.The static analyzer flags
separatorBeforePlaceholder(derived fromkeyFormat) being interpolated into a regex. While typical separators (.,-,_) are safe, unusual key formats could theoretically introduce regex special characters.The risk is low given the expected usage pattern, but consider escaping if stricter guarantees are needed:
♻️ Optional: Escape regex special characters
+function escapeRegex(str: string) { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} // In replacePlaceholder: - newFormat = newFormat.replace( - new RegExp(`${separatorBeforePlaceholder}${placeholder}`, "g"), - "", - ); + newFormat = newFormat.replace( + new RegExp(`${escapeRegex(separatorBeforePlaceholder)}${escapeRegex(placeholder)}`, "g"), + "", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/endpoints/preformatKey.ts` around lines 34 - 42, The regex uses user-derived values separatorBeforePlaceholder and placeholder when calling newFormat.replace via new RegExp(...), which can lead to ReDoS or unintended regex behavior; add an escape routine (e.g. escapeRegex) and apply it to separatorBeforePlaceholder and placeholder before constructing the RegExp in both branches (the empty-value branch and the replaceString branch) so the RegExp is built from escaped literals; update any references in this file (preformatKey.ts) where new RegExp(`${separatorBeforePlaceholder}${placeholder}`...) or new RegExp(`${placeholder}`...) is created to use the escaped versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.js`:
- Around line 36-40: The jest/expect-expect rule is unknown unless
eslint-plugin-jest is installed and registered; install the plugin (e.g., add
eslint-plugin-jest to devDependencies) and register it in the ESLint config used
here (add "jest" to the plugins array and/or extend "plugin:jest/recommended")
so the rule "jest/expect-expect" is recognized and the config no longer errors;
update the ESLint config where the rule list lives to include the plugin and/or
recommended preset.
In `@src/ui/hooks/useConnectedNodes.ts`:
- Around line 9-13: The query key for useQuery in useConnectedNodes currently
uses only [getConnectedNodesEndpoint.name], causing different calls with
different props to share the same cache; update the key to include a stable,
serializable representation of props (e.g., specific identifying fields or a
derived stable object) so cache entries are per-props (e.g., change the key to
[getConnectedNodesEndpoint.name, /* stableProps */]); leave the fetcher as
delayed(() => getConnectedNodesEndpoint.call(props)) and keep the select
transform unchanged, but ensure you trim non-serializable values out of the key.
In `@src/ui/views/Index/ListItem.tsx`:
- Around line 63-75: The effect in the ListItem component compares
debouncedNamespace against (node.ns ?? defaultNamespace) but omits
defaultNamespace from the useEffect dependency array; update the dependency list
for that useEffect (the one using debouncedKeyName, debouncedNamespace, node.id,
node.connected) to also include defaultNamespace so the comparison and the
setNodesDataMutation.mutate call use the up-to-date defaultNamespace value.
In `@src/ui/views/Settings/ProjectSettings.tsx`:
- Around line 138-144: The sort currently moves the empty namespace to the end
so settings.namespace initialized from namespaces?.[0] will pick a real
namespace by default; fix by changing the sort logic in the ns.sort callback so
the empty string (""/implicit default) stays at the front (e.g., treat a === ""
as smallest and b === "" as largest), or alternatively change the initialization
of settings.namespace (where namespaces?.[0] is used) to explicitly prefer ""
when present before persisting via onChange; update the comparison in the
ns.sort block and/or the namespace default assignment to ensure the implicit
empty namespace remains first and is not persisted without user action.
---
Outside diff comments:
In `@src/ui/client/useQueryApi.ts`:
- Around line 40-47: The query key used in useQuery (in useQueryApi) only
includes [url, path, query] but the fetcher also depends on config.apiUrl and
config.apiKey; update the query key in useQueryApi to include config.apiUrl and
config.apiKey (or otherwise derive a stable identity from config) so that
swapping API keys or servers won't return stale cached results; locate the
useQuery call inside useQueryApi and add config.apiUrl and config.apiKey to its
key array (or trigger cache invalidation on CONFIG_CHANGE if you prefer central
invalidation in GlobalState) so the cache is correctly scoped per config.
In `@src/ui/hooks/useInterpolatedTranslation.ts`:
- Around line 149-173: The effect that recomputes interpolation currently only
mutates interpolatedTranslation.current so the hook never re-renders and
downstream values (translationDiffersFromNode, placeholders, previewText) stay
stale; change the effect in useInterpolatedTranslation to set a state value
(e.g., setInterpolatedTranslationState) with the cleaned string returned by
getOrUpdateInterpolatedTranslation instead of only writing to
interpolatedTranslation.current, and include all missing dependencies
(paramsValues, node?.pluralParamValue, config?.language, selectedPluralVariant,
rawTranslation, node?.isPlural) in the effect dependency list so switching nodes
or plural variants triggers recomputation; update translationDiffersFromNode to
read from the new state value rather than interpolatedTranslation.current.
In `@src/ui/views/Pull/Pull.tsx`:
- Around line 68-70: The effect that calls computeDiff() currently depends on
selectedNodes.data and lang but misses hasNamespacesEnabled, causing a stale
diff when useHasNamespacesEnabled (from useHasNamespacesEnabled.ts) flips from
false to true; update the dependency array for the useEffect that calls
computeDiff() (the hook referencing computeDiff, selectedNodes.data, and lang)
to also include hasNamespacesEnabled so computeDiff re-runs when namespace
support status changes.
In `@src/ui/views/Settings/ProjectSettings.tsx`:
- Around line 193-236: use the same per-project credentials when checking
namespace availability and clear any stored namespace when namespaces become
disabled: update useHasNamespacesEnabled to accept apiKey/apiUrl (or add a
per-project boolean derived from namespacesLoadable keyed by apiKey/apiUrl) so
the UI decision uses the same credentials as namespacesLoadable, and in
ProjectSettings.tsx watch that per-project flag (the renamed/extended hook or
derived value) and call setSettings(s => ({ ...s!, namespace: "" })) when
namespaces become disabled so settings.namespace is reset and the
NamespaceSelect state can’t persist an invalid value; refer to
useHasNamespacesEnabled, namespacesLoadable, NamespaceSelect,
settings.namespace, apiKey/apiUrl and setSettings to locate where to change the
logic.
---
Nitpick comments:
In `@src/createFormatIcu.ts`:
- Around line 41-43: Replace the use of Object.values(...).find(...) when
computing ignoreTag with Object.values(...).some(...) so the check
short-circuits and directly returns a boolean; specifically update the ignoreTag
assignment in src/createFormatIcu.ts (the ignoreTag variable) to use .some
instead of .find and remove the extra boolean coercion (keep the negation as
needed) to improve performance and clarity.
In `@src/main/endpoints/preformatKey.ts`:
- Around line 34-42: The regex uses user-derived values
separatorBeforePlaceholder and placeholder when calling newFormat.replace via
new RegExp(...), which can lead to ReDoS or unintended regex behavior; add an
escape routine (e.g. escapeRegex) and apply it to separatorBeforePlaceholder and
placeholder before constructing the RegExp in both branches (the empty-value
branch and the replaceString branch) so the RegExp is built from escaped
literals; update any references in this file (preformatKey.ts) where new
RegExp(`${separatorBeforePlaceholder}${placeholder}`...) or new
RegExp(`${placeholder}`...) is created to use the escaped versions.
In `@src/ui/client/client.ts`:
- Around line 38-44: flattenParams currently rebuilds the accumulator on every
entry (using object spreads in the reduce), causing heavy allocations; change
flattenParams to use a single mutable accumulator object instead of spreading:
iterate Object.entries(params) (or switch to a for loop), and for each
[key,value] assign primitive/array values directly (acc[key] = value), and for
nested objects call flattenParams(value) and copy its properties into the same
acc (e.g., loop over the returned object's keys and assign acc[nestedKey] =
nestedValue or use Object.assign) before returning acc; keep the function name
flattenParams and the accumulator variable (acc) so the change is local and
preserves behavior.
In `@src/ui/client/useQueryApi.ts`:
- Around line 88-111: The mutate and mutateAsync callbacks close over
customOptions which itself uses queryClient and invalidatePrefix, causing stale
closures; wrap customOptions in useCallback with dependencies [queryClient,
invalidatePrefix, options?.onSuccess] (or at least [queryClient,
invalidatePrefix]) and include queryClient and invalidatePrefix in the
dependency arrays for the useCallback calls that create mutate and mutateAsync
(i.e., the arrays for mutate and mutateAsync should include
mutation.mutate/mutateAsync plus queryClient and invalidatePrefix) so the
callbacks always use current values of queryClient and invalidatePrefix.
In `@src/ui/components/NodeList/NodeRow.tsx`:
- Around line 34-37: The inline style object in the NodeRow component is
recreated every render (gridTemplateColumns based on prop action and cursor
based on onClick), which breaks React.memo; update NodeRow to memoize or hoist
the style so its reference is stable — e.g., use useMemo inside NodeRow to
compute the style object from action and onClick (or create two static style
constants and pick between them) and then pass that memoized/hoisted object to
the element instead of constructing it inline.
- Around line 21-22: NodeRow is being re-rendered every time because parent
creates new function/element references for props action, keyComponent,
nsComponent and the inline onClick (() => onClick?.(item)); in the parent
component (NodeList) wrap those callbacks in useCallback (and any derived
components in useMemo) so they are stable across renders, replace the inline
onClick arrow with a stable handler (e.g., const handleClick = useCallback(() =>
onClick?.(item), [onClick, itemId]) or bind using item id) and pass the memoized
action/keyComponent/nsComponent to NodeRow to make React.memo effective.
In `@src/ui/views/CreateCopy/CreateCopy.tsx`:
- Around line 129-141: Extract the inline style object { maxHeight: 200,
overflowY: "auto" } used on the languages container into a top-level constant
(e.g., LANGUAGES_LIST_STYLE) declared outside the CreateCopy component,
preserving the same properties and types; then replace the inline style on the
div that renders languages?.map(...) with style={LANGUAGES_LIST_STYLE} so the
map/Checkbox/handleToggleLanguage rendering uses the hoisted constant instead of
reallocating the object each render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cb01c50-c94f-474a-bdc0-6e71cda74bdb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (69)
eslint.config.jspackage.jsonsrc/createFormatIcu.tssrc/main/endpoints/copyPage.tssrc/main/endpoints/editorType.tssrc/main/endpoints/formatText.tssrc/main/endpoints/getScreenshots.tssrc/main/endpoints/highlightNode.tssrc/main/endpoints/preformatKey.tssrc/main/endpoints/setNodesData.tssrc/main/endpoints/updateNodes.tssrc/main/main.tssrc/main/utils/createEndpoint.tssrc/main/utils/delayed.tssrc/main/utils/nodeParents.tssrc/main/utils/nodeTools.tssrc/main/utils/pages.tssrc/main/utils/settingsTools.tssrc/main/utils/textFormattingTools.tssrc/tools/createProvider.tsxsrc/tools/getConflictingNodes.tssrc/tools/getPullChanges.tssrc/tools/getPushChanges.tssrc/ui/client/client.tssrc/ui/client/decodeApiKey.tssrc/ui/client/types.tssrc/ui/client/useQueryApi.tssrc/ui/components/AutocompleteSelect/AutocompleteSelect.tsxsrc/ui/components/Editor/Editor.tsxsrc/ui/components/Editor/TranslationPlurals.tsxsrc/ui/components/KeyOptionsButton/KeyOptionsButton.tsxsrc/ui/components/NamespaceSelect/NamespaceSelect.tsxsrc/ui/components/NodeList/NodeList.tsxsrc/ui/components/NodeList/NodeRow.tsxsrc/ui/components/ResizeHandle/ResizeHandle.tsxsrc/ui/components/Shared/HtmlText.tsxsrc/ui/hooks/useAllTranslations.tssrc/ui/hooks/useConnectedMutation.tssrc/ui/hooks/useConnectedNodes.tssrc/ui/hooks/useCopyPage.tssrc/ui/hooks/useEditorMode.tssrc/ui/hooks/useFigmaNotify.tssrc/ui/hooks/useFormatText.tssrc/ui/hooks/useHasNamespacesEnabled.tssrc/ui/hooks/useHighlightNodeMutation.tssrc/ui/hooks/useInterpolatedTranslation.tssrc/ui/hooks/usePrefilledKey.tssrc/ui/hooks/useSelectedNodes.tssrc/ui/hooks/useSetNodesDataMutation.tssrc/ui/hooks/useTranslation.tssrc/ui/hooks/useUpdateNodesMutation.tssrc/ui/hooks/useWindowSize.tssrc/ui/state/GlobalState.tssrc/ui/styles.csssrc/ui/views/Connect/Connect.tsxsrc/ui/views/CopyView/CopyView.tsxsrc/ui/views/CreateCopy/CreateCopy.tsxsrc/ui/views/Index/Index.tsxsrc/ui/views/Index/ListItem.tsxsrc/ui/views/Pull/Pull.tsxsrc/ui/views/Push/Push.tsxsrc/ui/views/Settings/ProjectSettings.tsxsrc/ui/views/Settings/PushSection.tsxsrc/ui/views/Settings/Settings.tsxsrc/ui/views/Settings/StringsEditor.tsxsrc/ui/views/Settings/StringsSection.tsxsrc/ui/views/StringDetails/StringDetails.tsxsrc/utilities.tssrc/web/main.ts
💤 Files with no reviewable changes (3)
- src/ui/views/Settings/PushSection.tsx
- src/ui/components/Shared/HtmlText.tsx
- src/ui/hooks/useHasNamespacesEnabled.ts
✅ Files skipped from review due to trivial changes (17)
- src/utilities.ts
- src/ui/hooks/useAllTranslations.ts
- src/ui/hooks/useHighlightNodeMutation.ts
- src/ui/components/KeyOptionsButton/KeyOptionsButton.tsx
- src/ui/hooks/useFigmaNotify.ts
- src/ui/hooks/useConnectedMutation.ts
- src/ui/hooks/useCopyPage.ts
- src/ui/hooks/useTranslation.ts
- src/ui/views/CopyView/CopyView.tsx
- src/main/endpoints/editorType.ts
- src/tools/createProvider.tsx
- src/ui/hooks/useWindowSize.ts
- src/main/utils/pages.ts
- src/ui/hooks/useSetNodesDataMutation.ts
- src/ui/views/Settings/Settings.tsx
- src/main/utils/nodeTools.ts
- src/main/main.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/ui/views/Connect/Connect.tsx
- src/ui/components/NamespaceSelect/NamespaceSelect.tsx
- src/ui/components/ResizeHandle/ResizeHandle.tsx
- src/ui/views/StringDetails/StringDetails.tsx
- package.json
- src/main/endpoints/getScreenshots.ts
- src/ui/hooks/useSelectedNodes.ts
- src/ui/views/Settings/StringsEditor.tsx
- Introduced normalized comparison for already selected values in multi-select mode. - Updated filtering logic to utilize a Set for performance optimization. - Ensured consistent casing and trimming for input values during comparisons.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
10-10: Consider separating lint check from lint fix.Running
--fixby default in the maineslintscript may be surprising to developers who expect a lint check without auto-modifications. A common pattern is to have separate scripts:
"lint": "eslint --ext .ts,.tsx,.js,.jsx src"for CI/checks"lint:fix": "eslint --fix --ext .ts,.tsx,.js,.jsx src"for auto-fixingThis prevents accidental modifications when developers just want to check for issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 10, The current package.json "eslint" script runs eslint with --fix which auto-modifies files; change it to a non-fixing check and add a separate fixing script: update the "eslint" script (the "eslint" key in package.json scripts) to remove the --fix flag so it runs only eslint --ext .ts,.tsx,.js,.jsx src, and add a new "eslint:fix" or "lint:fix" script that includes --fix (e.g., eslint --fix --ext .ts,.tsx,.js,.jsx src) so CI/quick checks use the non-mutating command and developers can explicitly run the fixer when desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/views/Settings/ProjectSettings.tsx`:
- Around line 197-200: The tooltip currently passes projectId:
getProjectIdFromApiKey(apiKey) ?? 0 into namespaceHelpTextSetUp which produces
links like /projects/0/... when the API key can't be decoded; change the logic
to first compute const projectId = getProjectIdFromApiKey(apiKey) (no fallback
to 0) and then either pass projectId (which may be undefined) into
namespaceHelpTextSetUp or conditionally render a non-linked/degraded tooltip
when projectId is falsy; apply the same change to both places that call
namespaceHelpTextSetUp (the calls around lines 197-200 and 214-217) so invalid
project IDs are not turned into concrete /projects/0 URLs.
- Around line 138-143: The comparator passed to ns.sort never returns 0 for the
implicit-default case, causing inconsistent ordering for duplicate empty
namespaces; update the comparator used in ns.sort so it first checks if both a
and b are empty/falsy and returns 0, then handle the single-empty cases (return
-1/1) and finally fall back to a.localeCompare(b) — reference the ns.sort
comparator to make this change.
---
Nitpick comments:
In `@package.json`:
- Line 10: The current package.json "eslint" script runs eslint with --fix which
auto-modifies files; change it to a non-fixing check and add a separate fixing
script: update the "eslint" script (the "eslint" key in package.json scripts) to
remove the --fix flag so it runs only eslint --ext .ts,.tsx,.js,.jsx src, and
add a new "eslint:fix" or "lint:fix" script that includes --fix (e.g., eslint
--fix --ext .ts,.tsx,.js,.jsx src) so CI/quick checks use the non-mutating
command and developers can explicitly run the fixer when desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49baf922-ee7d-4d6d-ad44-f9693915cff1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
eslint.config.jspackage.jsonsrc/ui/components/NodeList/NodeRow.tsxsrc/ui/hooks/useConnectedNodes.tssrc/ui/views/Index/ListItem.tsxsrc/ui/views/Pull/Pull.tsxsrc/ui/views/Settings/ProjectSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ui/components/NodeList/NodeRow.tsx
- src/ui/views/Pull/Pull.tsx
- src/ui/hooks/useConnectedNodes.ts
- eslint.config.js
Comment on lines
+197
to
+200
| {namespaceHelpTextSetUp({ | ||
| apiUrl, | ||
| projectId: getProjectIdFromApiKey(apiKey) ?? 0, | ||
| })} |
Contributor
There was a problem hiding this comment.
Avoid linking to /projects/0/... when the API key can't be decoded.
getProjectIdFromApiKey(apiKey) ?? 0 turns an unknown project into a concrete but invalid URL. Please keep the tooltip text, but suppress or degrade the link until a real projectId is available.
Also applies to: 214-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/views/Settings/ProjectSettings.tsx` around lines 197 - 200, The
tooltip currently passes projectId: getProjectIdFromApiKey(apiKey) ?? 0 into
namespaceHelpTextSetUp which produces links like /projects/0/... when the API
key can't be decoded; change the logic to first compute const projectId =
getProjectIdFromApiKey(apiKey) (no fallback to 0) and then either pass projectId
(which may be undefined) into namespaceHelpTextSetUp or conditionally render a
non-linked/degraded tooltip when projectId is falsy; apply the same change to
both places that call namespaceHelpTextSetUp (the calls around lines 197-200 and
214-217) so invalid project IDs are not turned into concrete /projects/0 URLs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useSelectedNodes— missing dependency array caused event listeners to pile up; now debounces dual DOCUMENT_CHANGE + SELECTION_CHANGE events into a single refetchuseCallback, consolidate two race-prone mutation effects into a single debounced effecttoLowerCase, highlight data)memoto prevent unnecessary re-renderscacheTime/staleTimewas0/0, now5min/30sfigma.listAvailableFontsAsync()performatTextcall instead of per rangegetConflictingNodesO(n²)→O(n),getPushChangesscreenshot lookup O(n×m×k)→O(n), eliminate redundant node traversal ingetScreenshots, combine 6 parent traversals into singlegetAllParents()inpreformatKeyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Improvements
Refactor
Chores